Automattic / batcache

A memcached HTML page cache for WordPress.
http://wordpress.org/extend/plugins/batcache/
284 stars 104 forks source link

Use Plugins API rather than the global variable #73

Closed aaronjorbin closed 8 years ago

aaronjorbin commented 8 years ago

WordPress 4.6 moves wp-includes/plugins.php inclusion before the loading of advanced-cache.php which means the Plugins API is available to use

joshlevinson commented 8 years ago

From what I can tell, the current behavior is causing fatals on sites running Batcache + WP trunk.

aaronjorbin commented 8 years ago

@skeltoac Any thoughts on this? It likely means bumping the minimum version to 4.6

danielbachhuber commented 8 years ago

Any thoughts on this? It likely means bumping the minimum version to 4.6

Couldn't you do:

if ( function_exists( 'add_filter' ) ) {
add_filter()
} else {
// modify the global
}
sboisvert commented 8 years ago

Included @danielbachhuber suggestion. Did a PR here https://github.com/Automattic/batcache/pull/76 Anyone have thoughts? @joshlevinson this seems to work for me, can you confirm it's also good for you?

skeltoac commented 8 years ago

I thought the point of build_preinitialized_hooks was so 4.7 would work with this kind of code. Is there something wrong with it? What are the fatals?

joshlevinson commented 8 years ago

@skeltoac In general, it is intended to work with that kind of code. The issue is that plugins API is loaded first in wp-settings.php, and makes a call to build_preinitialized_hooks immediately.

advanced-cache.php is loaded after that happens, later down in wp-settings.php

I believe the point of build_preinitialized_hooks is to initialize hooks that have been added prior to WordPress (or at least the majority of it) being loaded. Hooks that are added in the now-deprecated fashion after the plugins API is loaded will fatal.

joshlevinson commented 8 years ago

Note that this is immediately reproducible in an environment that has Batcache (active/caching on the current request, such that it execution of L679-680 occur) and anything that hooks into the two filters Batcache adds ( status_header and wp_redirect_status ). In my environment, such are Debug Bar Slow Actions and/or Restricted Site Access. I imagine an mu-plugin that simply induces the application of those filters would suffice.

Here's a full error log entry:

[13-Sep-2016 20:47:41 UTC] PHP Fatal error:  Uncaught Error: Call to a member function add_filter() on array in /srv/www/vip/htdocs/wp-includes/plugin.php:111
Stack trace:
#0 /srv/www/vip/htdocs/wp-includes/plugin.php(399): add_filter('status_header', Array, 9000, 1)
#1 /srv/www/vip/htdocs/wp-content/plugins/debug-bar-slow-actions/debug-bar-slow-actions.php(34): add_action('status_header', Array, 9000)
#2 /srv/www/vip/htdocs/wp-includes/class-wp-hook.php(346): Debug_Bar_Slow_Actions->time_start('status_header', 'HTTP/1.1 200 OK', 200, 'OK', 'HTTP/1.1')
#3 /srv/www/vip/htdocs/wp-includes/plugin.php(837): WP_Hook->do_all_hook(Array)
#4 /srv/www/vip/htdocs/wp-includes/plugin.php(185): _wp_call_all_hook(Array)
#5 /srv/www/vip/htdocs/wp-includes/functions.php(1092): apply_filters('status_header', 'HTTP/1.1 200 OK', 200, 'OK', 'HTTP/1.1')
#6 /srv/www/vip/htdocs/wp-includes/class-wp.php(677): status_header(200)
#7 /srv/www/vip/htdocs/wp-includes/class-wp.php(728): WP->handle_404()
#8 /srv/www/vip/htdocs/wp-includes/functions.php(963): WP->main('')
#9 /srv/www/ in /srv/www/vip/htdocs/wp-includes/plugin.php on line 111
joshlevinson commented 8 years ago

Confirmed that an advanced-cache.php file with the contents:

<?php
$wp_filter['foo'][10]['foo'] = array(
    'function' => function( $foo ) { return $foo; },
    'accepted_args' => 1,
);

and an mu-plugin with the contents:

apply_filters( 'foo', 'foo' );

induces a fatal error.

@danielbachhuber and @sboisvert's #76 sounds like the way to go; works as expected for me in 4.7 and 4.5 (the latter of which loads the plugin API after advanced-cache.php).

joshbetz commented 8 years ago

Closing in favor of #76