Load plugin.php earlier in wp-settings.php

[jorbin]

In order to rectify the differences between wp-settings.php and the wp-cli counterpart, filters need to be available earlier.

@rmccue summerized it in #34936 with:

Generally speaking, I think we should have more hooks more early. Given that plugin.php is almost entirely self-contained (we could split the file if needed), I'd be in favour of loading it earlier than it currently, moving it up to the first block of includes (load.php and default-constants.php). We could then add an action to wp_debug_mode for that, filter wp_maintenance, etc. It would result in a small amount more code loaded before advanced-cache kicks in (I imagine negligible in any case), but be super helpful for edge cases and advanced tooling.

This could potentially affect advance-cache.php, if it is preloading and changing the filter globals, so it will need dev notice and testing.

[dd32]

This could potentially affect advance-cache.php, if it is preloading and changing the filter globals

For one such example of that, see Batcache:
​https://plugins.trac.wordpress.org/browser/batcache/trunk/advanced-cache.php#L538

Batcache appears to be "safe" as it doesn't first do $wp_filter = array(), however there's bound to be others that do.

[danielbachhuber]

Replying to dd32:

Batcache appears to be "safe" as it doesn't first do $wp_filter = array(), however there's bound to be others that do.

If a drop-in resets $wp_filter, then it would already be breaking use of $wp_filter in wp-config.php

While I agree with the concern, I don't think this is a new risk.

[dd32]

If a drop-in resets $wp_filter, then it would already be breaking use of $wp_filter in wp-config.php

I don't think that's a use-case which we even care about - if anyone is doing that, stop it, you control the environment just use a mu-plugin. Dropins are different in that they're stand alone.

[jorbin]

Replying to dd32:

This could potentially affect advance-cache.php, if it is preloading and changing the filter globals

For one such example of that, see Batcache:
​https://plugins.trac.wordpress.org/browser/batcache/trunk/advanced-cache.php#L538

Batcache appears to be "safe" as it doesn't first do $wp_filter = array(), however there's bound to be others that do.

The above patch is an early version of something that will help prevent this potential issue. It backups up and merges the plugin globals around advance-cache.php. If it looks good to others, we can flesh it out with tests, documentation, etc

[jorbin]

Added some initial tests. More tests are likely needed before this is completely ready for core. Would also still like to know others opinions on this direction. array_merge_recursive can be a potentially expensive operation, so if we can perhaps figure out an alternative, I would be :+1:

[danielbachhuber]

36819.3.diff adds docs

array_merge_recursive can be a potentially expensive operation, so if we can perhaps figure out an alternative, I would be :+1:

In this case though, isn't the likelihood of array_merge_recursive() actually being called negligible? I have a hard time imagining a scenario where it's called, and actually has a performance impact.

[danielbachhuber]

@jorbin What's remaining before this can land?

[jorbin]

In 37588:

Bootstrap/Load: Load plugin.php earlier in wp-settings.php

In order to allow non-web initializations of WordPress (such as through wp-cli) to modify things like the check for maintenance mode, plugins.php and the associated functions must be available much earlier. The use of these functions earlier than the loading of plugins is not recommended in most use cases.

Fixes #36819. See #34936.
Props jorbin, danielbachhuber for documentation.

[pento]

array_merge_recursive() is funky.

36819.4.diff​ tries to do some saner array merging.

[schlessera]

array_merge_recursive() will create new sublevels when there are collisions. Instead of writing a custom loop like in 36819.4.diff (bad for performance), consider using array_replace_recursive(), which is probably the behavior that is wanted here.

[schlessera]

Argh, of course array_replace_recursive() has only been introduced with PHP 5.3. I think that a polyfill would be best suited then. It will have slightly worse performance of custom PHP looping in PHP 5.2, but provide full implemented speed from PHP 5.3 onwards.

Something like this is proposed in PHP docs:

<?php
if ( ! function_exists( 'array_replace_recursive' ) ) {
        function array_replace_recursive( $array, $array1 ) {
                function recurse( $array, $array1 ) {
                        foreach ( $array1 as $key => $value ) {
                                // create new key in $array, if it is empty or not an array
                                if ( ! isset( $array[ $key ] ) || ( isset( $array[ $key ] ) && ! is_array( $array[ $key ] ) ) ) {
                                        $array[ $key ] = [ ];
                                }

                                // overwrite the value in the base array
                                if ( is_array( $value ) ) {
                                        $value = recurse( $array[ $key ], $value );
                                }
                                $array[ $key ] = $value;
                        }
                        return $array;
                }

                // handle the arguments, merge one by one
                $args  = func_get_args();
                $array = $args[0];
                if ( ! is_array( $array ) ) {
                        return $array;
                }
                for ( $i = 1; $i < count( $args ); $i ++ ) {
                        if ( is_array( $args[ $i ] ) ) {
                                $array = recurse( $array, $args[ $i ] );
                        }
                }
                return $array;
        }
}

[johnbillion]

Where are the tests that demonstrate what's currently broken and what gets fixed by 36819.4.diff​ ?

[swissspidy]

@schlessera We already have a array_replace_recursive() compat function in wp-includes/compat.php since 4.5.3.

[schlessera]

Replying to swissspidy:

@schlessera We already have a array_replace_recursive() compat function in wp-includes/compat.php since 4.5.3.

Ah, thanks for pointing that out! Then please just ignore my second comment above.

[jorbin]

36819.4.diff​ introduced a broken test. 36819.5.diff​ fixes it. I also updated _backup_plugin_globals to specific if we wanted to be setting or getting.

[pento]

Noice.

36819.6.diff​ adds the test_applied_actions_are_counted_after_restore test, which ensures $wp_actions is restored correctly.

test_restore_plugin_globals_includes_additions tests that $wp_filter is restored correctly, all relevant tests continue to pass when both 36819.6.diff​ and 17817.11.diff​ are applied.

(There are some unrelated bugs in 17817.11.diff​ that need fixing.)

[pento]

Upon re-reading my previous comment, I realised I probably need to clarify the state of things.

• Trunk: All tests pass.
• Trunk + 17817.11.diff​: Hook tests fail, because _restore_plugin_globals() treats the content of each value in $wp_filter as an array. WP_Hook implements ArrayAccess, so happily complies, not realising it's about to be overwritten by an actual array.
• Trunk + 36819.6.diff​ + 17817.11.diff​: All hook tests pass, there are some other tests that fail, but they're not relevant to this code, and can be fixed as part of #17817.

My concern with these functions is that they're pretty hacky, but I don't think we're going to be able to do much better, due to how it has to wrap around cache drop-ins. The best outcome is a patch we can apply now, then we will never speak of this code again.

With that in mind, I think 36819.6.diff​ is ready for commit. @jorbin, could you please review, and add any other thoughts?

[pento]

In 38223:

Boostrap/Load: Improve forward compatiblity of plugin global backup methods.

[37588] added methods to backup the plugin globals, for ensuring that advanced cache drop-ins don't overwrite hooks that've been added earlier in the load process.

The method for restoring the plugin globals wasn't compatible with the implementation of WP_Hook in #17817. WP_Hook implements ArrayAccess, so _restore_plugin_globals() was treating it as an array, and inadvertantly overwriting the WP_Hook object with a plain array.

To avoid having to re-write this code as part of #17817, we now use add_filter() to restore any hooks that were added by cache drop-ins, which WP_Hook correctly supports.

Props pento, jorbin.
See #36819.

[pento]

In 38224:

Boostrap/Load: Improve forward compatiblity of plugin global backup methods.

[37588] added methods to backup the plugin globals, for ensuring that advanced cache drop-ins don't overwrite hooks that've been added earlier in the load process.

The method for restoring the plugin globals wasn't compatible with the implementation of WP_Hook in #17817. WP_Hook implements ArrayAccess, so _restore_plugin_globals() was treating it as an array, and inadvertantly overwriting the WP_Hook object with a plain array.

To avoid having to re-write this code as part of #17817, we now use add_filter() to restore any hooks that were added by cache drop-ins, which WP_Hook correctly supports.

Merge of [38223] with the 4.6 branch.

Props pento, jorbin.
Fixes #36819.

[andy]

Nicely! I'd like to mention that since batcache I have used the same pattern several times, adding filters even as early as ​auto_prepend_file. I would never clobber the core arrays, nor would spend any time accommodating that abuse case beyond detecting and exiting loudly.

[johnjamesjacoby]

I would never clobber the core arrays, nor would spend any time accommodating that abuse case beyond detecting and exiting loudly.

Bingo. That said, the work to protect everyone else from the edge-case is noble and hugely awesome.

[westi]

Replying to andy:

Nicely! I'd like to mention that since batcache I have used the same pattern several times, adding filters even as early as ​auto_prepend_file. I would never clobber the core arrays, nor would spend any time accommodating that abuse case beyond detecting and exiting loudly.

+1

I think that the work that was put in to support the idea the arrays might get clobbered is a little over done.

It also looks like it makes it impossible for an advanced-cache dropin to remove an already registered filter/action hook?

[pento]

Replying to westi:

It also looks like it makes it impossible for an advanced-cache dropin to remove an already registered filter/action hook?

You're killing me, @westi.

That's correct, advanced-cache dropins can no longer remove a hook that was registered earlier in the load process.

It's something we could potentially allow for, but it would cause a performance hit - rather than skipping an unchanged $wp_fliter, _restore_plugin_globals() would need to compare every entry in $wp_filter with its backup on every page load.

[westi]

Replying to pento:

Replying to westi:

It also looks like it makes it impossible for an advanced-cache dropin to remove an already registered filter/action hook?

You're killing me, @westi.

That's correct, advanced-cache dropins can no longer remove a hook that was registered earlier in the load process.

It's something we could potentially allow for, but it would cause a performance hit - rather than skipping an unchanged $wp_fliter, _restore_plugin_globals() would need to compare every entry in $wp_filter with its backup on every page load.

I am tempted to say we should just remove the code.

In part this is because I can't get my head around what bug this fixes.

If prior to this change a drop in was setting any of things things to an empty array and prior to it being called the array has something in it got lost.

After this change we now reverse that behaviour on existing sites...... and ensure that instead that entry that was lost (and the site was running fine with it lost) now persists.

Are we creating work trying to fix a bug that doesn't really "exist" here?

[pento]

Replying to westi:

If prior to this change a drop in was setting any of things things to an empty array and prior to it being called the array has something in it got lost.

After this change we now reverse that behaviour on existing sites...... and ensure that instead that entry that was lost (and the site was running fine with it lost) now persists.

Previously, I wouldn't be wildly concerned either way. But by moving the plugin.php load up to before the drop-in is loaded, we're now supporting hooks before then. I'm reasonably comfortable with external tools (wp-cli is the obvious example) being able to define their own hooks in $wp_filter before they even load WordPress.

Drop-ins don't get to accidentally stomp on things that are actually supported.

Are we creating work trying to fix a bug that doesn't really "exist" here?

Maybe. As dion mentioned, there are probably drop-ins who stomp on the global, expecting that they need to define it. Most of the code here is to take care of that situation.

[westi]

Replying to pento:

Replying to westi:

If prior to this change a drop in was setting any of things things to an empty array and prior to it being called the array has something in it got lost.

After this change we now reverse that behaviour on existing sites...... and ensure that instead that entry that was lost (and the site was running fine with it lost) now persists.

Previously, I wouldn't be wildly concerned either way. But by moving the plugin.php load up to before the drop-in is loaded, we're now supporting hooks before then. I'm reasonably comfortable with external tools (wp-cli is the obvious example) being able to define their own hooks in $wp_filter before they even load WordPress.

Drop-ins don't get to accidentally stomp on things that are actually supported.

Are we creating work trying to fix a bug that doesn't really "exist" here?

Maybe. As dion mentioned, there are probably drop-ins who stomp on the global, expecting that they need to define it. Most of the code here is to take care of that situation.

I get that.

But this code is here to fix bugs in other peoples code that previously didn't break something and only break something if they also add other new code to their sites.

aka we appear to have added compatibility code to fix future potential bugs at the expense of making something else impossible that someone might want to do in future.

[pento]

I'm re-opening this ticket, because I don't have a good answer for @westi's comments, but I think we should have an answer before 4.6 is released.

@jorbin, @danielbachhuber in particular, can you please provide your thoughts?

[danielbachhuber]

To speak to @westi's last comment, I hadn't previously considered this aspect:

aka we appear to have added compatibility code to fix future potential bugs at the expense of making something else impossible that someone might want to do in future.

At the end of the day, I think someone just needs to make a judgement call and own it.

Previously, the judgement call was to try to be as safe as possible, and save the end user from accidentally stomping on a $wp_filter global that's now already been defined in 4.6.

However, I'm not opposed to removing _backup_plugin_globals(), and this behavior of trying to save the user from themselves. In total numbers, WordPress sites using an advanced cache drop-in is probably relatively small. WP-CLI will continue to work as expected with a faulty advanced cache drop-in because it always disables the drop-in.

[jorbin]

I don't think reverting is a good idea.

As advance-cache.php is something that isn’t a part of the plugin system, there isn’t a good way to update them. I think if you asked most users “Are you using an advance cache dropin?” they wouldn’t be able to give you a solid answer. So we have the potential to break sites unintentionally. Is that a change we really want to make after RC2? What is the benefit of reverting? What do we gain?

It’s not impossible to remove an already registered filter/action, but it is impossible to know about other hooks during the inclusion. advance-cache.php could add an action to a later hook and run the removal there. Not ideal, but it could be done.

[westi]

Replying to jorbin:

I don't think reverting is a good idea.

As advance-cache.php is something that isn’t a part of the plugin system, there isn’t a good way to update them. I think if you asked most users “Are you using an advance cache dropin?” they wouldn’t be able to give you a solid answer. So we have the potential to break sites unintentionally. Is that a change we really want to make after RC2? What is the benefit of reverting? What do we gain?

I'm still not sure how the code we have in trunk stops a site breaking because of this change.

From what I can see the code we have in trunk ensures that if someone uses add_action or add_filter before the advanced-cache is includes in new code (they couldn't use them previously) that they will be protected from their existing advanced-cache drop-in from trampling on this call by setting a new array in the globals.

If someone was already writing to these arrays using a prepend-file or other hackery and then mutating them in their advanced-cache they are fine.

If someone was already writing to these arrays using a prepend-file or other hackery and then resetting them to empty arrays in their advanced-cache, and unwittingly relying on this behaviour, this change breaks them.

I think allowing people to use plugin api functions earlier is great, I just don't understand why users of advanced-cache drop-ins need this extra hand holding which just seems to waste cpu cycles for the exact set of people who are trying to make their sites faster.

[azaozz]

I too don't think we need _backup_plugin_globals() and _restore_plugin_globals(). They change the existing behavior for no good reason, see long slack discussion: ​https://wordpress.slack.com/archives/core/p1471041725003663.

If we don't backup and restore the globals, it all behaves in the same way as in 4.5. It doesn't make any difference where $wp_filter or add_filter() are defined, it only matters if the global is set before the function is called, and the place that function is called for the first time hasn't changed.

[azaozz]

36819.8.diff is the same as 36819.7.diff except it removes the two function completely. They are private functions that have not existed in a release.

[pento]

+1 for committing 36819.8.diff​.

[dd32]

I'm also +1 for committing 36819.8.diff​.

As I ​posted in slack I expect that this is going to break someone doing something very wrong, but I, on a second thought, don't think it warrants the efforts done here.

[aaroncampbell]

Finally caught up on all of this (here and on Slack) and I'm +1 on 36819.8.diff​ as well.

[jorbin]

In 38251:

Bootstrap/Load: Revert Plugin Global restoration around advance-cache.php.

First added in [37588] and later modified in [38224], the idea was to ensure that filters/actions added before advance-cache.php would not disappear if advance-cache.php overloaded the filters/actions with code such as $wp_filter = array(). This is an edge case and one that there is no documented case of existing.

This restores the behavior from WordPress 4.5 and before. It is strongly encouraged that developers using advance-cache.php to use the Plugins API that is available before the loading of advance-cache.php rather than directly interacting with any of the globals.

Props azaozz, jorbin, dd32 for review, pento for review, westi for investigation, ipstenu for research.
See #36819.

[jorbin]

In 38252:

Bootstrap/Load: Revert Plugin Global restoration around advance-cache.php.

Merges [38251] to the 4.6 branch.

First added in [37588] and later modified in [38224], the idea was to ensure that filters/actions added before advance-cache.php would not disappear if advance-cache.php overloaded the filters/actions with code such as $wp_filter = array(). This is an edge case and one that there is no documented case of existing.

This restores the behavior from WordPress 4.5 and before. It is strongly encouraged that developers using advance-cache.php to use the Plugins API that is available before the loading of advance-cache.php rather than directly interacting with any of the globals.

Props azaozz, jorbin, dd32 for review, pento for review, westi for investigation, ipstenu for research.

See #36819.

[knutsp]

@jorbin You are repeatingly referring to a, as far as I know, non-existent drop-in file named advance-cache.php. Isn't the correct file name for the drop-in in question advanced-cache.php?

[jorbin]

#jorbined