tollmanz / ostrichcize

A WordPress plugin to hide PHP errors for specified plugins or themes
26 stars 2 forks source link

Ostrichize needs to instantiate itself at muplugins_loaded to take advantage of installation in mu-plugins folder #8

Closed tollmanz closed 11 years ago

tollmanz commented 11 years ago

@jeremyclarke Noted in issue https://github.com/tollmanz/ostrichcize/issues/7#issuecomment-10815122 that MU plugins do not have notices and errors hidden. Theoretically, this should be handled here: https://github.com/tollmanz/ostrichcize/blob/master/ostrichcize.php#L87.

@jeremyclarke Can you dump $paths here (https://github.com/tollmanz/ostrichcize/blob/master/ostrichcize.php#L123) and let me know if the proper paths are being output? In other words, are you seeing the proper paths to your MU plugins?

jerclarke commented 11 years ago

Hey Tollmanz,

I think this is actually not the issue I was having, sorry! I wasn't trying to hide errors from a plugin inside mu-plugins, the only reason I mentioned mu-plugins is because it makes me pretty confident that I'm doing all Ostrichize stuff as early as I possibly can.

The issue confusing me was a plugin where a deprecated function was called outside a filter (i.e. inside the plugin's .php file). In such cases Ostrichize seems to fail to hide the error, but if I move the code into a hooked function then the error is hidden. This is frustrating because a lot of stupid plugins run their bad code outside filters, so Ostrichize misses out on a lot of errors.

NOW, at the time I hadn't actually looked at your code. I just dove in and found what I think is the real "problem" and a workable "solution", and based on it the title of this ticket should probably be Ostrichize needs to instantiate itself at muplugins_loaded to take advantage of installation in mu-plugins folder.

See, the thing is that putting Ostrichize and my ostriched-plugins-hook-function into mu-plugins SHOULD silence even errors outside a filter in normal plugins because mu-plugins loads earlier, whereas if you install Ostrichize as a normal plugin it makes sense that it can't affect other plugins loading at the same time.

The issue is that right now, regardless of where you install Ostrichize, it only instantiates itself (and sets the error handler) during the plugins_loaded hook. This means it waits much longer than it needs to before setting the error handler. If I change your code to attach to the muplugins_loaded hook then that fixes the issue because the error handler is set before WP even touches the normal plugins.

For consideration, here's how my mu-plugin looks to take advantage of the change:

function gv_ostrichcized_plugins( $slugs ) {
    $slugs[] = 'subscribe-to-comments'; 
    return $slugs;
}
function gv_pre_my_ostrichcized_plugins() {
    add_filter( 'ostrichcized_plugins', 'gv_ostrichcized_plugins' );
}
add_action( 'muplugins_loaded', 'gv_pre_my_ostrichcized_plugins', 1 );

Note that I have to register my hook on muplugins_loaded as well for this to work.

Now the issue with this is that if you instantiate the Ostrichize object on muplugins_loaded then all users of your plugin would have to declare their list of banned errors in an mu-plugin as well. In theory you could make this a pre-requisite of the plugin, the best argument being that the plugin is much more effective this way. Obviously that is not at all desirable though, as mu-plugins is a ghetto that most people don't know about or understand (e.g. plugin files can't be in folders).

Initially I thought we could just run your instantiation action on both hooks because it doesn't generate any errors, but that would actually do nothing more than just running it on muplugins_loaded because once the object is instantiated and the error handler is set running struthrio_get_the_ostrich again does nothing.

The ultimate problem that needs solving is: How can we load Ostrichize at muplugins_loaded for those that have it installed in mu-plugins, while loading it at plugins_loaded for those that have it as a normal plugin.

Looking at the whole plugin, the most obvious answer I can think of is to modify struthrio_get_the_ostrich to only instantiate Ostrichize if at least one of the ostrichized_* filters has a function attached to it:

function struthrio_get_the_ostrich() {
    if (    has_filter('ostrichcized_directories') 
        OR has_filter('ostrichcized_plugins')
        OR has_filter('ostrichcized_paths')
        OR has_filter('ostrichcize_theme') )
    return Struthio_Camelus::instance();
}

// Initiate the plugin functionality
add_action( 'muplugins_loaded', 'struthrio_get_the_ostrich' );
add_action( 'plugins_loaded', 'struthrio_get_the_ostrich' );

This way struthrio_get_the_ostrich will try to load itself during muplugins_loaded but will quietly do nothing if the dev hasn't registered anything. Then it tries again at plugins_loaded. A bonus of this setup is that if nothing was ever declared as ostrichized then we don't even create the unnecessary object :)

To me that's a pretty good fix. The one issue with it that I can think of is that you have only one chance to register ostrichized plugins/dirs, either MU plugins or normal plugins, whereas some people might want to register them in multiple places (e.g. In current version I could have multiple plugins registering ostrichized stuff as long as they are all processed before plugins_loaded, but if I have a single registered thing in my mu-plugin then anything that comes later would be effectively ignored). IMHO this isn't a huge limitation, and I doubt many people would actually want more than one place where they declare Ostrichized stuff (especially if they're being all sneaky with mu-plugins). It's definitely less of an issue than not being able to take advantage of mu-plugins.

Wow that was a long reply! Hope you find it helpful :)

tollmanz commented 11 years ago

Changing ticket title from "Plugins in MU plugins folder are not ostrichcized" to "Ostrichize needs to instantiate itself at muplugins_loaded to take advantage of installation in mu-plugins folder"

tollmanz commented 11 years ago

Thank you for taking the time to write such a thorough ticket with an excellent description of the problem and explanation of how to recreate it. I love making and open sourcing code and the reward is when other people pop in to take a look at it and help make it better! Thanks for the motivation.

I think your suggestions are all fantastic. I totally get the need to register struthio_get_the_ostrich on muplugins_loaded. Additionally, I really like the idea of not instantiating the object unless needed. That is very clean and performant. I'll get these in ASAP!

tollmanz commented 11 years ago

I believe I patched this up. Can you take it for a spin and let me know if it works for you?

jerclarke commented 11 years ago

Yeah it's working for me! Thanks dude.

Also I can't believe I never thought of this before: Do a proper git clone of this plugin into /plugins/ then put an ln -s ../plugins/ostrichcize/ostrichcize.php ostrichcize.php into mu-plugins. That way I get notified when there are updates.

tollmanz commented 11 years ago

Thanks for verifying! I'll close this issue.

The symlink is an interesting idea. Let me know how that works out long term.