Handling of dormant shortcodes is inelegant

[markjaquith]

Our handling of dormant shortcodes is inelegant. They spit out the raw shortcode. Better, would be for use to keep track of a historical record of which shortcodes have been registered, and if it's one of those "dormant" shortcodes (used to be registered, but no longer is), have it gracefully degrade, not show the shortcode, and return its contents if it's a wrapper shortcode.

This would require an option to track the shortcodes that have been used. Whenever a new one comes around, we add it to the array.

Eventually, we could deprecate the use of non-registered shortcodes, get rid of this solution, and simplify our regex to be generic (which would scale better).

[scribu]

I'm not sure what you mean by "deprecate the use of non-registered shortcodes", but I agree that being able to parse any shortcode without it being registered would be ideal.

[trevogre]

I think that I did that wrong. I'll have to lookup how to submit a patch and see if I did that right.

The uploaded patch does the first two parts of this. Adds an option to store every shortcode that is added with modifications to add_shortcode. It stores the array with each shortcode pointing to a blank shortcode that returns only the content enclosed in the shortcode.

This option is then loaded into $shortcode_tags when the file is loaded and each found run of add_shortcode replaces the blank function with it shortcode if the shortcode still exists.

I hope this passes the elegance test. The only thing that concerns me is running update_option repeatedly. I'm not sure if that is doing a database call every time it runs but I would assume that it does.

[scribu]

Replying to trevogre:

I think that I did that wrong. I'll have to lookup how to submit a patch and see if I did that right.

​http://core.trac.wordpress.org/wiki#HowtoSubmitPatches

[trevogre]

No, I think that is right, but the code was wrong forgot some braces on the if statement. The second upload should work as a patch.

So I assume the process is what I just did, upload the patch to the ticket and then wait for a person with commit access to review it, improve it, laugh at it and dump it or include it?

The tutorials are just shy of complete. It takes you through creating the patch but not the part about submitting them for review.

[scribu]

Yes, shortcodes.2.php is indeed a patch, so you should name it appropriately: shortcodes.diff or shortcodes.patch.

More info: ​http://codex.wordpress.org/Reporting_Bugs#Patching_Bugs

[trevogre]

It seems a bit drastic to just show no shortcode to someone previewing a page, I'll have to look and see if there is any functions to detect preview mode, maybe it makes sense to keep the [nonworkingshortcode] outputting in preview mode. Or to have some span print out that shows where the code should be if you view source. I just think it might really confuse people that are expecting to test thier content and see that output.

It might be better to allow the theme dev to set the default shortcode handling, put in the new code, but have it still spit out the [] by default. Mabye a register_default_shortcode function that you can use in your functions.php or a plugin to set the handling of all unregistered shortcodes.

I guess I'll change the extension to .patch, is there a difference between .diff and .patch on some systems?

[scribu]

is there a difference between .diff and .patch on some systems?

Nope.

[aaroncampbell]

A couple thoughts:

1. It seems like a preview is "a look at what this will really look like" so I think it should be the same as what a user would see after it's published.
2. The patch doesn't work. Each time a shortocde is added the option is updated to the CURRENT list of shortcodes, which means anything that USED to exist but no longer does isn't in the option. I used the following code added to functions.php, added [testshortcode] to a post, viewed it, removed the code below, loaded the post again and it worked, then loaded the post again and the shortcode shows again (so it only works for one page load).

function testshortcode( $attr, $content = '' ) {
	return "Shortcode is working";
}
add_shortcode( 'testshortcode', 'testshortcode' );

[aaroncampbell]

Talked to trevgore in IRC. I recommended something like this:
On init retrieve option and register shortcodes. Since each shortcode tag can only have 1 function attached anything registered after will overwrite. Example code:

$old_shortcodes = get_option( 'old_shortcodes', array() );
foreach ( $old_shortcodes as $sc ) {
	if ( $sc_not_registered_already )
		add_shortcode( $sc, 'unregistered_shortcode' );
}

And then on shutdown load the option and merge the list of old shortcodes with the current set and store the list back in the option.

I think trevgore is going to work on another patch.

[trevogre]

That should be better. I also added the ability to register a default shortcode. With register_default_shortcode($func). That may be a bad name as it's the default for unregistered shortcodes.

I used the suggestion to register the used shortcodes at shutdown.

I didn't put in anything to fire on init because the $shortcode_tags is initialized with an array on load of the shortcodes.php file currently. So I thought that the option needed to be loaded at the earliest point possible so that it would happen before all add_shortcode calls.

It would be easy to wrap that in init if that makes more sense but I don't know the answer to that.

I'm not sure about the comment that it only works for one page load. I don't see that, but I'm going to poke at it some more and see if I can't make that happen.

[aaroncampbell]

18558.diff​ is a first pass at processing ALL shortcodes (not just registered ones) and passing any that aren't registered through a generic unregistered_shortcode() which just returns the content. I'm sure it needs more work on the regex, but I threw a decent number of tests at it an it worked. It basically considers the shortcode name to be the from right after the opening [ until the first space or ]

One of the things I had to do was add the ability for do_shortcode to be able to process a single tag. The reason is that the embed shortcode had a hack where it unregistered all shortcodes, ran do_shortcode() on the content, then re-registered all the shortcodes...just so that the embed shortcode could run before the rest. Now it can just call do_shortcode( $content, 'embed' ). We may want to consider making the second argument an array so you could process a group of shortcodes, but I'm not sure it's necessary.

[trevogre]

That's probably a good idea, but I see some issues with it.

I didn't consider that because it seems like a feature to me to always show something on output of a newly entered shortcode that doesn't exist. So that when you preview you see that you got the shortcode wrong, or need to install the shortcode. The real feature enhancement seemed to be when a shortcode disappeared that you may have used in a ton of places. It would be a pain to go and clean your site to remove it everywhere and better to have it just not appear.

Also, someone might want to use [] in their text, and so you are going to make them into completely special characters instead of special characters only in explicit circumstances.
That could be very bad for backwards compatibility.

It seems to me that that could/should be an option somewhere. Treat all text contained in [] as shortcodes.

Combine that with the ability to set a default shortcode handler and you have a feature.

You could write some interesting catch all default shortcode handlers with a bunch of switches. So maybe your default shortcode handler in your theme emulates a bunch of existing shortcodes that you might not have turned on.

One thing that might be interesting is to global the current running shortcode so that you know which shortcode name is running. $current_shortcode_rendering. Then your default shortcode could switch its behavior based upon the shortcode name.

[jdgrimes]

Replying to trevogre:

One thing that might be interesting is to global the current running shortcode so that you know which shortcode name is running. $current_shortcode_rendering.

+1. I would like to see that improvement. We have current_filter(), so why not current_shortcode()?

[miqrogroove]

I'm working on a roadmap proposal to fix this going forward with better shortcode syntax.

[miqrogroove]

#12760 was marked as a duplicate.

[miqrogroove]

At the 9 September meeting, participants agreed there is no way to fix this ticket without unwanted changes to the shortcode syntax. Based on that, I strongly recommend this issue to close as wontfix.

For details, please see ​https://make.wordpress.org/core/2015/09/29/shortcode-roadmap-draft-two/

[miqrogroove]

At the October 14 meeting, participants agreed to close this ticket after receiving no further feedback.