Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#38929 closed defect (bug) (fixed)

WP_Hook should support filters added manually through `advanced-cache.php`

Reported by: dd32's profile dd32 Owned by: dd32's profile dd32
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Plugins Keywords: commit has-patch dev-reviewed
Focuses: Cc:

Description

As noted in Slack, with WP_Hook in 4.7, we've seemingly glossed over the impact of these changes upon advanced-cache.php dropins.
Prior to WP_Hook being committed, we'd attempted to work around filter additions from advanced-cache.php with some funky code which we eventually reverted in [38251] - we didn't *really* need it at the time, but with WP_Hook additions we're now in the scenario where something like that IS needed.

WP_Hook includes the ability to upgrade a non-wp_hook $wp_filter array to an array of WP_Hooks to account for cases where filters are added before plugins.php / WP_Hook loads.

With the upgrade to 4.7, any user who is running an object cache which performs direct $wp_filter modifications is likely to cause a fatal error, unless they've updated the cache first and done something like Batcache: https://github.com/Automattic/batcache/pull/76/files

However, thanks to the upgrade code, we can simply call it again after any non-code files are included - thankfully, the only user file which this could apply to is advanced-cache.php.

Attached is a 4 line patch (38929.diff) which upgrades the $wp_filter modifications so that a older advanced-cache.php is compatible with 4.7+.

At this point of the pageload, most WordPress installs will have *zero* filters registered, unless something such as WP-CLI is in use, or a filter was added before WordPress loaded, so the performance of this is a no-op for most people.


It's worth noting that there exist some incompatibilities with WP_Hook, in that any plugin which manually alters $wp_filter will at best cause a PHP notice, and at worst, cause a PHP fatal error (although it's also still possible to manually manipulate $wp_filter if you must, by setting entire arrays rather than modifying it - see below)

One of these cases affects advanced-cache.php - If something has added a hook, and advanced-cache.php attempts to also add a hook through manual $wp_filter modification it'll cause a PHP notice and the filter addition will fail - I feel that this is an edgecase enough not to need to support.

test-wp-hook.php is a stand-alone-ish script to obviously show these edge cases, but also shows that the diff in 38929.diff should work as expected, if you discount the PHP notice possibility and the hopefully unlikely fatal.

Attachments (3)

38929.diff (1.6 KB) - added by dd32 7 years ago.
test-wp-hook.php (2.2 KB) - added by dd32 7 years ago.
38929.2.diff (851 bytes) - added by ocean90 6 years ago.

Download all attachments as: .zip

Change History (15)

@dd32
7 years ago

@dd32
7 years ago

#1 follow-up: @dd32
7 years ago

As noted in slack, I'll also mention that there's the possibility we could've simply used a new variable $wp_filter_hooks (for example) and migrated added hooks over as needed (instead of calling WP_Hook::build_preinitialized_hooks()) - this would've have the ability of avoiding any lost hooks and fatal errors, but while having all our public API's continue to work.

I can't really see any downside to that, but given the lack of bug reports during 4.7, and the ability to support the most common example I can think of (Batcache) through 38929.diff I don't see the need to explore that option further at this point - anyone modifying $wp_filter directly shouldn't probably be doing so anyway after WordPress loads.

#2 @pento
7 years ago

  • Keywords commit added
  • Owner set to dd32
  • Status changed from new to assigned
  • Version set to trunk

Let's do this.

#3 in reply to: ↑ 1 ; follow-ups: @azaozz
7 years ago

Replying to dd32:

...anyone modifying $wp_filter directly shouldn't probably be doing so anyway after WordPress loads.

Don't think anybody should be modifying $wp_filter directly in any case. Do you think it is a good idea to add "Doing it wrong..." somewhere? Perhaps in WP_Hook::build_preinitialized_hooks() as it exists to fix that "bad behavior".

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

Replying to azaozz:

Replying to dd32:

...anyone modifying $wp_filter directly shouldn't probably be doing so anyway after WordPress loads.

Don't think anybody should be modifying $wp_filter directly in any case. Do you think it is a good idea to add "Doing it wrong..." somewhere? Perhaps in WP_Hook::build_preinitialized_hooks() as it exists to fix that "bad behavior".

I don't see a huge benefit in doing that, as it'll only get triggered for existing use-cases that had to use that method pre-4.6.
The only case of that that we really care about is when people do that much later in the load process, where add_action() would've previously been available. It's way too hard to catch that though right now, but will at least throw a PHP Notice now if it's an existing hook (a hook with no filters though would cause a fatal, I'm not convinced that adding extra checks to apply_filters() or do_action() to catch that scenario is worth it)

#5 follow-up: @nacin
7 years ago

This looks good — elegant.

Is it worth putting this inside the WP_CACHE branch?

#6 in reply to: ↑ 5 @dd32
7 years ago

Replying to nacin:

This looks good — elegant.

Is it worth putting this inside the WP_CACHE branch?

I see no harm in doing so, doesn't look like any of the functions between that and plugin.php inclusion should be requiring user functions; it wasn't intentional to include it outside the conditional, it just happened that way :)

@ocean90
6 years ago

#7 @ocean90
6 years ago

  • Keywords has-patch dev-reviewed added

38929.2.diff moves the code inside the WP_CACHE branch.

#8 in reply to: ↑ 3 ; follow-up: @rmccue
6 years ago

Replying to azaozz:

Replying to dd32:

...anyone modifying $wp_filter directly shouldn't probably be doing so anyway after WordPress loads.

Don't think anybody should be modifying $wp_filter directly in any case. Do you think it is a good idea to add "Doing it wrong..." somewhere? Perhaps in WP_Hook::build_preinitialized_hooks() as it exists to fix that "bad behavior".

Marking it with deprecation for cache drop-ins would be a good idea, IMO. I don't think it should trigger that for hooks declared before WP is loaded though, since there's no alternative to setting $wp_filter if WP hasn't yet loaded.

#9 in reply to: ↑ 8 @dd32
6 years ago

  • Status changed from assigned to accepted

Replying to rmccue:

Replying to azaozz:

Replying to dd32:

...anyone modifying $wp_filter directly shouldn't probably be doing so anyway after WordPress loads.

Don't think anybody should be modifying $wp_filter directly in any case. Do you think it is a good idea to add "Doing it wrong..." somewhere? Perhaps in WP_Hook::build_preinitialized_hooks() as it exists to fix that "bad behavior".

Marking it with deprecation for cache drop-ins would be a good idea, IMO. I don't think it should trigger that for hooks declared before WP is loaded though, since there's no alternative to setting $wp_filter if WP hasn't yet loaded.

I believe the correct way is for the code to include plugin.php before WordPress to get access to the methods.
I don't think it's worth putting a deprecation warning in here, especially as it's unlikely to really be seen by developers when it's triggered this early in the bootstrap.

#10 @azaozz
6 years ago

Yeah, agreed. Few months ago we chatted a bit about introducing a third type of plugins, (early-load/bootstrap?) that would replace the drop-ins, or rather remove the need for drop-ins. The first step for that would be to include plugin.php very early.

#11 @dd32
6 years ago

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

In 39369:

WP_Hook: Re-initialize any actions added directly to $wp_filter by advanced-cache.php.

Props dd32, ocean90.
Fixes #38929.

#12 @dd32
6 years ago

In 39370:

WP_Hook: Re-initialize any actions added directly to $wp_filter by advanced-cache.php.

Props dd32, ocean90.
Merges [39369] to the 4.7 branch.
Fixes #38929.

Note: See TracTickets for help on using tickets.