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

[dd32]

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.

[dd32]

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.

[pento]

Let's do this.

[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".

[dd32]

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)

[nacin]

This looks good — elegant.

Is it worth putting this inside the WP_CACHE branch?

[dd32]

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]

38929.2.diff​ moves the code inside the WP_CACHE branch.

[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.

[dd32]

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.

[azaozz]

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.

[dd32]

In 39369:

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

Props dd32, ocean90.
Fixes #38929.

[dd32]

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.