Make WordPress Core

Opened 2 years ago

Closed 20 months ago

#55883 closed enhancement (fixed)

Reverse wrapping of `apply_shortcodes()` and `do_shortcode()`

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Shortcodes Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Background: #37422, #55852.

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)

#1 follow-up: @rafiahmedd
2 years ago

@SergeyBiryukov should we make the changes in wp-includes/shortcodes.php?

#2 in reply to: ↑ 1 ; follow-up: @SergeyBiryukov
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: @namith.jawahar
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 @rafiahmedd
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 with apply_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 @SergeyBiryukov
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_tagpre_apply_shortcode_tag. Used in about 25 plugins.
  • do_shortcode_tagapply_shortcode_tag. Used in about 132 plugins. Looks like we'll have to keep the old filters as aliases for a while, though using apply_filters_deprecated() for them can also be an option.

As a side note, we might want to do the same for do_blocks()apply_blocks() in a separate ticket.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#6 @rafiahmedd
2 years ago

@SergeyBiryukov should we start replacing these?

#7 @peterwilsoncc
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

#8 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.1

This ticket was mentioned in PR #3284 on WordPress/wordpress-develop by costdev.


21 months 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 to apply_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 the do_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 @audrasjb
21 months 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.

#11 @audrasjb
21 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 54248:

Shortcodes: Reverse wrapping of apply_shortcodes() and do_shortcode().

This changeset reverses 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 to apply_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.

Follow-up to [47004].

Props SergeyBiryukov, rafiahmedd, namithjawahar, peterwilsoncc, costdev.
Fixes #55883.

#13 @ocean90
20 months 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?

#15 @davidbaumwald
20 months ago

Prepping a commit to revert the filter changes.

#16 @davidbaumwald
20 months ago

In 54278:

Shortcodes: Revert default filter callback changes from apply_shortcodes to do_shortcode.

[54248] reversed the wrapping of do_shortcode and apply_shortcodes and updated all direct internal calls of do_shortcode to apply_shortcodes after [47004]. Default filter callbacks that used do_shortcode were also updated to use apply_shortcodes. However, this introduced a backward-compatibility break because any attempt to unhook a filter using the previous do_shortcode callback would be futile.

This change reverts only the filter callback changes in [54248] to resolve the backward-compatibility break.

Follow-up to [47004] and [54248].

Props ocean90, SergeyBiryukov.
See #55883.

#18 @davidbaumwald
20 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#19 @TobiasBg
20 months 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 @davidbaumwald
20 months 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 @azaozz
20 months 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.

Last edited 20 months ago by azaozz (previous) (diff)

#22 @azaozz
20 months ago

  • Keywords revert added

#23 follow-up: @JeffPaul
20 months ago

@azaozz I think the revert you're requesting was handled in r54278, correct?

#24 in reply to: ↑ 23 @azaozz
20 months 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 @jorbin
20 months ago

I agree with @azaozz that there is little to no benefit in renaming this function and we should revert the renaming.

#26 @davidbaumwald
20 months ago

Prepping a full revert for this now.

#27 @davidbaumwald
20 months ago

In 54319:

Shortcodes: Revert recent apply_shortcodes and do_shortcode changes.

[54248] reversed the wrapping of do_shortcode and apply_shortcodes and updated all direct internal calls of do_shortcode to apply_shortcodes after [47004]. After further consideration, the long history of do_shortcodes should be favored over any subjective semantic improvements. This change reverts the remaining changes from #55883 not already reverted in [54278].

Follow-up to [47004], [54248], and [54278].

Props azaozz, jorbin.
See #55883.

#28 @davidbaumwald
20 months ago

  • Keywords needs-docs add-to-field-guide removed

Cleaning up the tags a bit post-revert.

#29 follow-up: @audrasjb
20 months 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 👍

Last edited 20 months ago by audrasjb (previous) (diff)

#30 follow-up: @TobiasBg
20 months 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 @azaozz
20 months 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 @azaozz
20 months 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 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.

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.

Last edited 20 months ago by azaozz (previous) (diff)

#33 @azaozz
20 months ago

  • Keywords close added; revert removed

Imho this is completed now. Leaving it open only as a reminder to perhaps update the apply_shortcodes() docblock a bit before 6.1. Thanks everybody for the great discussion here! :)

#34 @audrasjb
20 months ago

  • Keywords close removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Closing this as fixed, since 6.1 RC1 is approaching, so we have cleared milestone.
Better to open a new ticket to handle follow-up docblock changes if needed.

Note: See TracTickets for help on using tickets.