Plugin does not gracefully fail when editing active plugin causes fatal error

[ovann86]

When editing an active plugin in the wp-admin you see the following message above the 'Update file' button.

Warning: Making changes to active plugins is not recommended. If your changes cause a fatal error, the plugin will be automatically deactivated.

However, if an edit causes a fatal error the plugin is not automatically deactivated - resulting in the "white screen of death".

Similarly, attempting to activate a plugin that contains a fatal error will automatically deactivate the plugin.

Steps to reproduce

1. In the WordPress Administration, go to Plugins-> Installed Plugins
2. For an active plugin, click on the 'Edit' link
3. Add erroneous code to plugin, for example

// DO NOT ADD THIS TO YOUR PLUGIN - IT WILL CAUSE A FATAL ERROR -- MISSING CLOSING endif;
if ( ! function_exists( 'wp_password_change_notification' ) ) :
    function wp_password_change_notification( $user ) {
        return;
    }

4. Click 'Update File'
5. Changes are saved to the plugin, page loads and confirmation "File edited successfully." is displayed
6. Attempt to load another page (for example, the same page or the home page) and the "white screen of death" occurs.

[MattyRob]

@ovann86

I've just used your code and editing the main plugin file. The plugin was deactivated on the edit with an error message (screen shot to follow).

This can still happen if you edit an included file on occasions (I've been bitten a few times) but the current steps are not a 100% way to reproduce this issue.

[ovann86]

@MattyRob

Thanks for taking the time to run the test.

After some more testing it looks like this issue only occurs under PHP version 7.0 (I tested 5.3, 5.4, 5.5, 5.6, 7.1 and the plugin deactivated automatically).

If you can, could you try running the same test under PHP 7.0?

I'll try to do a screen capture of what I see shortly.

[ovann86]

Adding fatal php to active plugin under PHP 5.6 - plugin successfully deactivates

[ovann86]

Adding fatal php to active plugin under PHP 7.0 - plugin DOES NOT deactivate - subsequent white screen of death on reload

[MattyRob]

Weird, I've just installed PHP 7.0.15 and got it working on my localhost testing site - it still worked as expected?!?

What subversion of PHP 7 are you using? Could it be a PHP glitch that's been patched?

[ovann86]

I'm seeing the issue until 7.0.15 and 7.0.13, both Apache but different hosts (the later being a clean VPN install).

I did a little research and saw that PHP 7 introduced different error handling for included files with parse errors, but that wouldn't explain why it works for you with 7.0.15.

Under my test environment I was able to see that the activate_plugin function continued to run after the include_once() (with parse error) rather than stop and redirect to the previously set header location.

[MattyRob]

How bizarre. Must be something to do with PHP or server configuration then I would have though. Perhaps one for a core dev opinion.

[Ste_95]

I get this all the time on a variety of customers sites, regardless of the PHP version. Anyone else?

[westonruter]

I'm able to reproduce this issue. The error detection logic does not seem to be reliable. More often than not when I edit a plugin's PHP file with any fatal error (syntax, missing function, throwing explicit exception) then somehow it seems PHP is just ignoring it when activate_plugin() ​does:

<?php
include_once( WP_PLUGIN_DIR . '/' . $plugin );

I've tried switching it to an include or a require but there is no change. If I file_get_contents() on it before including it, then I see the file has the error in it. But when PHP actually loads it, then it doesn't complain. It's like PHP has a cache of the file in memory. I'm using VVV for testing with PHP 7.0.17.

[westonruter]

I believe I've found the problem and have a solution. It's PHP's opcache that is indeed causing the problem. The solution here seems to simply be to call opcache_invalidate() on the file right after it is written.

I also noticed a two problems with catching fatal errors when editing a plugin's file other than the main one, in particular editing akismet/class.akismet-admin.php instead of akismet/admin.php:

1. Since akismet/class.akismet-admin.php only gets included in the admin, the fatal error would not get raised when attempted sandbox activation. The solution here is simply to define WP_ADMIN and trigger the init and admin_init actions, as well as any other key actions that plugins would hook into during loading.
2. With 1 fixed, when you are taken back to the plugin editor after trying to save an error causing the plugin to get deactivated, only the file param is provided and the plugin editor assumed the plugin was the file if the plugin param was absent. So when you tried to re-save the file to re-activate the plugin, then WordPress would complain about the plugin not having a file editor, because for example it incorrectly identified akismet/class.akismet-admin.php as the plugin file. The fix here is just to try to look up the plugin for a given file.

Lastly, in the case of trying to save with a file with errors in it, the resulting notice about the plugin being deactivated strangely had a green success updated style as opposed to an error style.

These issues are addressed in 39766.0.diff​.

[westonruter]

​https://github.com/xwp/wordpress-develop/pull/262

[westonruter]

And 39766.1.diff​ adds:

• Adding more actions to trigger after activating.
• Removing all existing actions that other plugins may have already added.
• Ensure fatal error printed to screen will appear when editing plugin file other than the main one.

@clorith Would you test please?

[Clorith]

Fatal error is detected, and displayed within the iframe. I'm still not a huge fan of deactivating a previously active plugin, but it is in line with the expected current behavior of the plugins editor at least.

[westonruter]

​https://github.com/xwp/wordpress-develop/pull/262/files/1e2292e..0dcf189

[westonruter]

In 41560:

Plugin Editor: Improve reliability of detecting PHP fatal errors when editing an active plugin.

• Invalidate PHP opcache after file is updated to ensure include will include the written changes.
• Define WP_ADMIN when activating plugin in sandbox so plugin code targeting admin will be loaded.
• Do actions that get triggered when loading the admin to ensure plugin code runs that could cause errors on plugin editor screen (and lock out access).
• Fix ability to re-activate a plugin after editing a PHP file other than the main plugin file, and ensure PHP fatal error will be displayed in such cases.
• Consolidate duplicated code into plugin_sandbox_scrape() and re-use in activate_plugin().
• Show an error notice instead of a success notice when a file is updated but a plugin was deactivated due to a fatal error.
• Update style of warning when editing an active plugin to be styled as an actual warning notice.

See #12423, #21622.
Fixes #39766.

[westonruter]

In 41561:

Plugin Editor: Use include_once instead of include in plugin_sandbox_scrape() to fix unit tests broken with [41560].

See #39766.

[jdgrimes]

I started getting a huge number of failures in the unit tests for one of my plugins, and I was able to trace it back to [41560]. The use of plugin_sandbox_scrape() in activate_plugin() is the problem, in particular, the running of the 'plugins_loaded' action. I am still investigating exactly what is going on there (it has to do with how the plugin is activated before the tests run), but after reviewing this change I think that in general it needs a lot more testing with various plugins. Running these actions immediately after activation is unexpected by plugins and may cause issues. Also, some of the functions hooked to these actions by plugins may expect other actions to run between them, and will have problems. While running these common actions seems like a reasonable idea, I think that it needs a lot more thought, and is probably not feasible. It would be better to trigger a remote request that would run all of the usual code, rather than just simulating a few actions. A request for the plugins screen would be ideal, since even if things break somewhere else, as long as the user can access the plugins screen they can still deactivate the plugin if needed. But regardless, I think that the part of the commit running those actions may need to be reverted for now, until a better solution is found.

[westonruter]

@jdgrimes thanks for raising this. Have you see the approach suggested on #21622?

[westonruter]

@jdgrimes Just removing the plugins_loaded action fixes the issue for your own plugins?

[jdgrimes]

Replying to westonruter:

@jdgrimes Just removing the plugins_loaded action fixes the issue for your own plugins?

Yes.

The reason that it was causing errors is because my plugin has some code hooked to 'plugins_loaded' that alters an option in the database. That option also affects the behavior of the plugin's activation function. Basically, when that option is already set, the activation function assumes that the plugin has already been installed, and so it doesn't run some parts of the install routine (parts that it only needs to run on initial install and not on reactivation). As a result, those parts of the plugin are no longer activated by default.

In short, the design of the activation function was based on the (up to now valid) assumption that no parts of the plugin code would be run prior to activation. Now you are running any code hooked to these actions prior to activation, and if the plugin has never been activated, prior to it running its install routine (assuming that it installs itself via the activation hook).

Perhaps if you could determine whether the plugin had already been activated before or not, it would be OK to do something like this in activate_plugin() only in the case when the plugin was being reactivated. Otherwise, I think that this is just asking for trouble.

[westonruter]

In 41671:

Plugin Editor: Remove plugins_loaded from being re-triggered in plugin_sandbox_scrape().

Props jdgrimes.
Amends [41560].
Fixes #39766.

[jdgrimes]

I can confirm that [41671] fixes the problem with my plugin. However, I suspect that other plugins will have issues with the other actions being called. This should probably be tested with a variety of plugins to determine whether there is going to be many issues.

[westonruter]

I think the plugin_sandbox_scrape() approach should be removed entirely in favor of #21622 which can then be used by themes and plugins alike. The problem is time.

[westonruter]

As I just commented on #21622:

Please test and review these changes: ​https://github.com/xwp/wordpress-develop/pull/272

• Refactor themes and plugin editors to submit file changes via Ajax instead of doing full page refreshes. Moves file modifications from theme-editor.php and plugin-editor.php to common Ajax handler.
• Do sandboxed updates of PHP files for both active themes and plugins, with rollbacks in the case of a loopback request results in a fatal error. This supersedes changes in #39766 (Plugin does not gracefully fail when editing active plugin causes fatal error).

The improvements to the user experience and the robustness of the file editors are huge.

[westonruter]

In 41721:

File Editors: Introduce sandboxed live editing of PHP files with rollbacks for both themes and plugins.

• Edits to active plugins which cause PHP fatal errors will no longer auto-deactivate the plugin. Supersedes #39766.
• Introduce sandboxed PHP file edits for active themes, preventing accidental whitescreening of a user's site when introducing a fatal error.
• After writing a change to a PHP file for an active theme or plugin, perform loopback requests on the file editor admin screens and the homepage to check for fatal errors. If a fatal error is encountered, roll back the edited file and display the error to the user to fix and try again.
• Introduce a secure way to scrape PHP fatal errors from a site via wp_start_scraping_edited_file_errors() and wp_finalize_scraping_edited_file_errors().
• Moves file modifications from theme-editor.php and plugin-editor.php to common wp_edit_theme_plugin_file() function.
• Refactor themes and plugin editors to submit file changes via Ajax instead of doing full page refreshes when JS is available.
• Use get method for theme/plugin dropdowns.
• Improve styling of plugin editors, including width of plugin/theme dropdowns.
• Improve notices API for theme/plugin editor JS component.
• Strip common base directory from plugin file list. See #24048.
• Factor out functions to list editable file types in wp_get_theme_file_editable_extensions() and wp_get_plugin_file_editable_extensions().
• Scroll to line in editor that has linting error when attempting to save. See #41886.
• Add checkbox to dismiss lint errors to proceed with saving. See #41887.
• Only style the Update File button as disabled instead of actually disabling it for accessibility reasons.
• Ensure that value from CodeMirror is used instead of textarea when CodeMirror is present.
• Add "Are you sure?" check when leaving editor when there are unsaved changes.

Supersedes [41560].
See #39766, #24048, #41886.
Props westonruter, Clorith, melchoyce, johnbillion, jjj, jdgrimes, azaozz.
Fixes #21622, #41887.

[jdgrimes]

@westonruter [41721] says that [41560] is superseded, but [41560] has not been reverted. Is that an oversight?

[westonruter]

@jdgrimes it is superseded in that it is a better approach to sandboxed plugin editing. However, the improvements to plugin_sandbox_scrape() would still make plugin activation more robust as it also uses this function. That is why I didn't revert.

[jdgrimes]

Replying to westonruter:

@jdgrimes it is superseded in that it is a better approach to sandboxed plugin editing. However, the improvements to plugin_sandbox_scrape() would still make plugin activation more robust as it also uses this function. That is why I didn't revert.

OK. :-)

I asked in part because I realized that there was another test failure in my plugin resulting from this change. This time a function hooked to init. It isn't a major thing, but I still have my doubts about this entire approach.

I think that if activate_plugin() wants to use this approach, it should at least run the actions after the plugin is installed. Meaning that it wouldn't be able to use plugin_sandbox_scrape().

But I'd actually rather see a clean revert of the change to activate_plugin() completely. After all, increasing the robustness of plugin activation in general wasn't the point of this ticket. I think if that is needed for some reason, it should be pursued in a separate ticket. Ideally, an approach more like that taken in #21622 would end up being used. As it is, I think that the current approach is fragile enough to make plugin activation less robust instead of more. I may be wrong about that though, of course.

[westonruter]

OK, we'll revert.

[westonruter]

In 41819:

Plugin Editor: Revert superseded changes to activate_plugin() and plugin_sandbox_scrape().

Partially revert [41671], [41561], [41560].
See #21622.
Fixes #39766.

[jdgrimes]

Replying to westonruter:

In 41819:

Plugin Editor: Revert superseded changes to activate_plugin() and plugin_sandbox_scrape().

Partially revert [41671], [41561], [41560].
See #21622.
Fixes #39766.

Thank you @westonruter! :-D I can confirm that that fixed the remaining issue that I was having.

[Ste_95]

I'm not sure reopening this is the correct policy, but I believe the implemented fix causes more issues. More often than not, it happens that I cannot edit a file on a customer's site, getting the following error:

This has happened more than once, on apparently unrelated websites.

[Clorith]

Please do create a new topic on our ​support forums if you are having issues, this is likely caused by a plugin or theme conflict though which we'll be able to help you troubleshoot there.