Opened 2 years ago
Closed 2 years ago
#55883 closed enhancement (fixed)
Reverse wrapping of `apply_shortcodes()` and `do_shortcode()`
Reported by: | SergeyBiryukov | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Shortcodes | Keywords: | has-patch 2nd-opinion |
Focuses: | Cc: |
Description
In WordPress 5.4, apply_shortcodes() was added as an alias for do_shortcode() for better semantics. It aligns more with apply_filters()
and the semantics of applying filters to the input and returning a result, rather than performing an action and outputting to the current buffer.
Since we've revisited sanitize_url()
and esc_url_raw()
in [53452], I think it's time to consider doing the same here. Should we make apply_shortcodes()
the recommended function, instead of do_shortcode()
?
Change History (34)
#2
in reply to:
↑ 1
;
follow-up:
↓ 4
@
2 years ago
Replying to rafiahmedd:
should we make the changes in wp-includes/shortcodes.php?
If there is enough consensus for this to go forward, then yes, that would be the start :) The next step would be replacing all do_shortcode()
calls in core with apply_shortcodes()
.
#3
follow-up:
↓ 5
@
2 years ago
Seems like the logical thing to do, What about related functions like do_shortcodes_in_html_tags and filters like pre_do_shortcode_tag
#4
in reply to:
↑ 2
@
2 years ago
Replying to SergeyBiryukov:
If there is enough consensus for this to go forward, then yes, that would be the start :) The next step would be replacing all
do_shortcode()
calls in core withapply_shortcodes()
.
Then I think we have to wait for some more commiters to make a decision about this. And I can start replacing this from now.
#5
in reply to:
↑ 3
@
2 years ago
Replying to namith.jawahar:
What about related functions like do_shortcodes_in_html_tags and filters like pre_do_shortcode_tag
Good question, thanks! For consistency, I think we can use the new naming for them as well.
Functions:
do_shortcode_tag()
→apply_shortcode_tag()
. Since the original function is marked as private, we can probably deprecate it in favor of the new one. Looking at the usage in plugins, most instances appear to be false positives or copies of the function.do_shortcodes_in_html_tags()
→apply_shortcodes_in_html_tags()
. The original function is not marked as private and is used in about 15 plugins. It can be up for discussion whether to deprecate it too, or keep as an alias of the new one.
Filters:
pre_do_shortcode_tag
→pre_apply_shortcode_tag
. Used in about 25 plugins.do_shortcode_tag
→apply_shortcode_tag
. Used in about 132 plugins. Looks like we'll have to keep the old filters as aliases for a while, though usingapply_filters_deprecated()
for them can also be an option.
As a side note, we might want to the same for do_blocks()
→ apply_blocks()
in a separate ticket.
#7
@
2 years ago
- Keywords needs-docs added
@SergeyBiryukov I endorse this change.
Looking at WordPress.org, it looks like various pages of docs will need updating but nothing major. https://www.google.com/search?q=site%3Awordpress.org%2F+do_shortcode+-topic
This ticket was mentioned in PR #3284 on WordPress/wordpress-develop by costdev.
2 years ago
#9
- Keywords has-patch added
This PR changes the wrapping of apply_shortcodes()
and do_shortcode()
such that apply_shortcodes()
is now the recommended function.
In addition:
- Calls to
do_shortcode()
have been changed toapply_shortcodes()
. - Some default filter callbacks have been changed from
'do_shortcode'
to'apply_shortcodes'
. - Applicable documentation has been updated to refer to
apply_shortcodes()
instead. (please check these!)
This PR does not touch:
- Any other
do_shortcode_*()
function, as I'm not clear if a decision has been made about introducing new wrappers or deprecating thedo_shortcode_*()
variants. - Any hooks, as I'm not clear if a decision has been made about keeping the hooks, or using
apply_filters_deprecated()
. - The default filter callback for widget_text_content, as this causes test failures and may impact legacy support.
- This line in
wp-includes/blocks/template-part.php
as this file is sourced from Gutenberg.
Trac ticket: https://core.trac.wordpress.org/ticket/55883
#10
@
2 years ago
- Keywords commit needs-dev-note add-to-field-guide added; 2nd-opinion removed
- Owner set to audrasjb
- Status changed from new to accepted
Looks go to me!
Let's start with this first implementation for 6.1, then we can open a new ticket to address filters, and open an issue upstream on Gutenberg repository to address the occurrence located in Gutenberg.
Self assigning to commit this PR.
2 years ago
#12
Committed in https://core.trac.wordpress.org/changeset/54248
#13
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
[54248] will break any usage of remove_filter( 'the_content', 'do_shortcode', 11 );
. How can this be avoided?
This ticket was mentioned in PR #3304 on WordPress/wordpress-develop by dream-encode.
2 years ago
#14
Trac ticket: https://core.trac.wordpress.org/ticket/55883
dream-encode commented on PR #3304:
2 years ago
#17
Merged into core in https://core.trac.wordpress.org/changeset/54278.
#19
@
2 years ago
@davidbaumwald: Should an inline comment be added to these lines, so that caution is taken once do_shortcode
is formally deprecated, or so that this change is not repeated accidentally?
#20
@
2 years ago
@TobiasBg given its age and widespread usage, I don't think there's a plan to deprecate do_shortcode
. I think extra care should just be taken when changing default filter/function names in the future.
#21
@
2 years ago
- Keywords 2nd-opinion added; commit needs-dev-note removed
- Resolution fixed deleted
- Status changed from closed to reopened
I'm sorry but I don't think this is a good change.
Is apply_shortcodes()
really that much better (in plain English) than do_shortcode()
. Is it really worth it breaking all references to do_shortcode
just for the sake of changing do
with apply
? I think not.
do_shortcode
has existed for many many years. It is referenced in a lot of places, online documentation, many posts, thousands of themes and plugins, many books, etc. Why would WordPress want to break all these references? It is the same as breaking links, very bad UX.
I think it is a pretty bad idea to create all that confusion and broken references for such a minor benefit. A much better solution imho would be to update the docs, add as much content there as needed. That will serve all existing WP contributors and help all new contributors, and most importantly, will not break any links and cause confusion.
Also I'm not sure if the apply_shortcodes
alias was needed in the first place. It's been around for couple of years, since WP 5.4, but seems the adoption is pretty low? Think it can be classified as an "unsuccessful experiment" at this point. A quick search shows 134 matches in plugins for apply_shortcodes
versus 38653 matches for do_shortcode
. See https://wpdirectory.net/search/01GDK9ZK4WX0KQT21ZS28K9E06 and https://wpdirectory.net/search/01GDKA13MP0C6FKDYXWHPPXQ9P. That alias probably just brings some confusion for new contributors.
I'd like to propose that this is reverted and closed. This type of name changes is not worth the trouble and confusion they bring imho.
#23
follow-up:
↓ 24
@
2 years ago
@azaozz I think the revert you're requesting was handled in r54278, correct?
#24
in reply to:
↑ 23
@
2 years ago
Replying to JeffPaul:
No, [54278] was a partial revert to fix a regression. What I meant is to revert the whole "rename" of do_shortcode()
to apply_shortcodes()
as committed in [54248]. Imho it is not good to break all existing references (38k in plugins and about 263k on Google) to the old name, as it is not that bad. The confusion of these broken references far outweighs any possible benefits of renaming. The partial revert seems to have made this even more confusing.
Another reason for full revert would be that "apply shortcodes" doesn't describe properly what that function does. It does search and replace, and in English you usually cannot say "apply search and replace". The proper way to say that would be to "do search and replace". Maybe not such a big deal, but why not use good English :)
#25
@
2 years ago
I agree with @azaozz that there is little to no benefit in renaming this function and we should revert the renaming.
#28
@
2 years ago
- Keywords needs-docs add-to-field-guide removed
Cleaning up the tags a bit post-revert.
#29
follow-up:
↓ 31
@
2 years ago
Even if I weighted in by committing the initial patch (since it was working, reviewed, and nobody raised any issue with the change), I can see how this change is probably a bit overkill. Thanks for sharing a detailed review on this 👍
#30
follow-up:
↓ 32
@
2 years ago
I agree that reverting this is the proper decision.
While the initial idea of introducing the alias and potentially considering deprecating do_shortcode()
if the alias gained enough traction, was definitely worthwhile, the problem is that [54278] will always be needed:
If Core were to change add_filter( 'the_content', 'do_shortcode', 11 );
to the alias, there would be an immediate back-compat break. First deprecating do_shortcode()
(with a proper _deprecated_function()
call in it) would mean that Core itself would be triggering these warnings. Soft-deprecating do_shortcode()
without a _deprecated_function()
call (basically the status quo after the revert, and from wording of initial commit messages and the 5.4 dev note) has not resulted in widespread use of the alias (see azaozz's posted links).
Due to there not being a feasible path to fully switching to a new function name, the rich history and widespread use of do_shortcode()
, the low use of apply_shortcodes()
, I would therefore actually suggest to deprecate apply_shortcodes()
again. There's no need to keep a rarely used alias that Core itself will not be able to use and that plugin developers have not started to use in 6 or 7 WP releases. I agree with azaozz that this was an experiment that simply has shown to be unsuccessful, due to lack of user adoption and (only now) brought-up BC concerns.
#31
in reply to:
↑ 29
@
2 years ago
Replying to audrasjb:
Sorry it took so long to review this. At first I thought having more aliases is a good idea, a chance to improve some function names without breaking back-compat. But thinking more about it the price to pay for that is usually pretty high considering all the broken references to the old names. Still think is would be good to add aliases for some of the old functions where (over the years) the code and usage have moved beyond the initial intent. If nothing else having an alias would make the "true use" easier to discover. Not sure about replacing the names that are used in core. Seems that may get confusing.
#32
in reply to:
↑ 30
@
2 years ago
Replying to TobiasBg:
Due to there not being a feasible path to fully switching to a new function name, the rich history and widespread use of
do_shortcode()
, the low use ofapply_shortcodes()
, I would therefore actually suggest to deprecateapply_shortcodes()
again. There's no need to keep a rarely used alias that Core itself will not be able to use and that plugin developers have not started to use in 6 or 7 WP releases.
Deprecating apply_shortcodes()
sounds like the next logical step. But even if it is deprecated it cannot ever be removed. So perhaps the docs can be updated a bit and it can remain as an alias. Extenders would still have the choice to use it (that's the case even if it is deprecated). However the chances to confuse new extenders and contributors will be super low.
@SergeyBiryukov should we make the changes in wp-includes/shortcodes.php?