Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#41422 new enhancement

Need replace_filter() function for improved Just In Time filter adjusment

Reported by: bobbingwide's profile bobbingwide Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9
Component: Formatting Keywords:
Focuses: Cc:

Description

In the new solution for text widgets there is some Just In Time code that adjusts the attached filter functions. The logic finds the priority before removing the filter function then re-instates the filter with the same priority.

This logic assumes that the sequence in which the filter functions have been attached with the same priority are performed doesn't matter.

If this is not the case then the order change could affect processing where the filter is shared but invoked for different purposes. e.g. the 'widget_text' filter for Text widget and Custom HTML widget

The logic could be improved by implementing a new suite of filter functions including replace_filter(), restore_filter(), disable_filter() and a dummy filter function disabled_filter().

Instead of calling remove_filter() and add_filter() we'd use
disable_filter() and restore_filter()

disable_filter() is simply replace_filter() using the disabled_filter() function.

Example:

Filters attached at priority 10 have been set to do_shortcode and balanceTags.
With the current logic after JIT replacement the attached filters would have become
balanceTags then do_shortcode.

Using disable_filter() and restore_filter() the attached filters at priority 10 would be unchanged.


Attachments (1)

41422.diff (5.9 KB) - added by bobbingwide 7 years ago.
logic to support replacement and restore of filter functions

Download all attachments as: .zip

Change History (7)

#1 @azaozz
7 years ago

Not sure if that's a good idea: more complexity for something that is already easy to do with the current functionality. The filters priority is exactly for these cases. Wouldn't it be much clearer and easier to add balanceTags with priority of 11 (or 15)?

(BTW plugins can do that too: remove both filters then add do_shortcode with priority 10 and balanceTags with priority 11.)

Last edited 7 years ago by azaozz (previous) (diff)

@bobbingwide
7 years ago

logic to support replacement and restore of filter functions

#2 follow-up: @bobbingwide
7 years ago

Hi @azaozz
I might not have proposed this solution had priorities been used correctly in the first place.
But the new just in time code for text widgets and custom HTML widgets has to attempt to work with what it's been given, which could mean that the filter function being removed (do_shortcode) and replaced is not the only one at a particular priority.

In the tests that I've added I show how remove and add filter can affect the results.
Whereas, with the replacement method, when all the disabled functions have been restored, we get the same results as before.

Last edited 7 years ago by bobbingwide (previous) (diff)

#3 in reply to: ↑ 2 ; follow-up: @azaozz
7 years ago

Replying to bobbingwide:

I might not have proposed this solution had priorities been used correctly in the first place.

I understand, but fixing a (minor) bug by extending the API is.... perhaps an overkill? :)

As far as I see disabling and then enabling a hook is pretty similar to removing and then re-adding a hook. The only (meaningful) difference is that it keeps the "order of loading" sub-priority which shouldn't matter in the first place if priorities are used properly. If they are not, lets fix them.

So this adds a second "priority" thing which is starting to get confusing: remove_action, disable_action, remove_disabled_action, enable_removed_action, ... etc.

I agree replace_action may be useful in some cases. Perhaps we can add that and leave it to plugins if they want to do the disable/enable stuff themselves (i.e. replace an action with a noop callback for some time)?

In any case a plugin can always do:

remove_filter( 'the_content', 'wpautop' );
remove_filter( 'the_content', 'balance_tags' );
....
add_filter( 'the_content', 'wpautop' );
add_filter( 'the_content', 'balance_tags', 15 );

#4 in reply to: ↑ 3 @bobbingwide
7 years ago

Replying to azaozz:

As far as I see disabling and then enabling a hook is pretty similar to removing and then re-adding a hook. The only (meaningful) difference is that it keeps the "order of loading" sub-priority

Exactly. It maintains the status quo. And just in time hook replacement should aim to do this. WordPress core allows plugins and themes to change the sequence of hooks. It should respect those changes.

which shouldn't matter in the first place if priorities are used properly. If they are not, lets fix them.

Ah, there's the rub. If you change core then people will complain.
So you have to work with the existing flawed implementation and patch it up a bit.
WordPress core has many cases where multiple filter functions are used at the same priority.

So this adds a second "priority" thing which is starting to get confusing: remove_action, disable_action, remove_disabled_action, enable_removed_action, ... etc.

I didn't want to propose too many bells and whistles.

  • No need to develop aliases of the filter functions for hooks
  • No need to be able to stack replacements using push/pop logic
  • Priority must be specified

I haven't actually raised a bug report against the do_shortcode, balanceTags problem since

  • It's not actually a problem for me
  • I can develop a workaround for my own plugin
  • My workaround is backward compatible; it should not require changes to any other plugins

But I can envisage other plugins and themes where the current JIT logic can cause a problem and will need a fix. Hence this TRAC.


#5 @jnylen0
7 years ago

I am -1 on this change. This should be achieved by a combination of remove_filter, add_filter, and where necessary, fixing the core logic instead to be less confusing. (You referenced "Just In Time code that adjusts the attached filter functions" - can we get a link to this code please?)

#6 @bobbingwide
7 years ago

The solution for #40951 includes just in time code to remove_filter( 'widget_text', 'do_shortcode', $widget_text_do_shortcode_priority ) and then re-add it with the same priority that it had.

If there are other filters at that priority then the sequence in which the filters are applied is unintentionally changed. See src/wp-includes/widgets/class-wp-widget-text.php

This new core logic is trying to make adjustments to filters that have been added / changed by plugins or the theme. It should not break something that has been carefully crafted by the plugins or theme.

Use of replace_filter() is easier than other solutions that would be required to maintain the sequence within priority.

Note: See TracTickets for help on using tickets.