Sandboxed live editing of PHP files doesn't work together with WSOD protection

[ocean90]

To reproduce:

• Open the plugin editor
• Select an active plugin
• Do a change which will cause a fatal error
• Click "Update File"
• You should now get the loopback_request_failed message (Unable to communicate back with site…)
• Go to wp-admin/plugins.php and notice that the plugin has been paused which means the changes were actually saved because the WSOD protection did kick in even the changes were rolled back by wp_edit_theme_plugin_file().

[ohiosierra]

Am I missing a step? I don't have this issue. The plugin is paused for me, presumably in time to prevent the loopback request from failing.

On a fresh install of 5.2-beta1-20190404.183103:

1. Created an empty plugin.

<?php
/*
Plugin Name: Fatal Error plugin
Version: .1
Description: This plugin starts blank, but you can fix...errr, break...that.
Text Domain: fatal-error
*/

2. Activated the plugin.
3. Loaded Plugins->Plugin Editor
4. Modified the plugin to induce a fatal error

<?php
/*
Plugin Name: Fatal Error plugin
Version: .1
Description: This plugin starts blank, but you can fix...errr, break...that.
Text Domain: fatal-error
*/

trigger_error("Fatal error", E_USER_ERROR);

5. Hit "Update File"

Result: The message, "File edited successfully." is shown. On the next page load, WordPress triggers WSOD protection and generates the recovery mode email. If I enter Recovery Mode, the plugin has been paused. The error has been saved in the file and not rolled back.

[xkon]

If it's of any help:

While using an Apache/Mysql setup I was able to replicate @ocean90 report.

While using VVV everything is as @ohiosierra mentions.

In both cases ( same php, same options, fresh trunk pulls, clean installs etc ).

[ohiosierra]

I've identified the difference for when it occurs and doesn't -> WP_DEBUG.

When WP_DEBUG is enabled, the "doing_it_wrong" function outputs a user notice. Specifically, it outputs the doing it wrong notice 3 times during the wp_die() function.

One of these notices becomes the error_get_last().

When wp_finalize_scraping_edited_file_errors() tries to determine if the change caused a fatal error, it specifically looks at the last error only and sees if it is one of E_CORE_ERROR, E_COMPILE_ERROR, E_ERROR, E_PARSE, E_USER_ERROR, or E_RECOVERABLE_ERROR.

Since doing_it_wrong is only a user notice, it fails that check. wp_finalize_scraping_edited_file_errors() returns a "true" (success), the loopback completes, and no changes are rolled back.

Of course, there *is* a fatal error, so the next load triggers the WSOD protection.

This might be worth a separate ticket to improve the wp_finalize_scraping_edited_file_errors function to look at all errors and see if any are fatal. However, for the purposes of this ticket, the only note is for reproducing the error and confirming resolution, make sure WP_DEBUG is off.

[flixos90]

This is caused because the fatal error handler treats such errors, although in this case it shouldn't.

We actually noticed this is also affecting other areas:

• When you resume a plugin/theme that is still broken, you will end up in a redirect loop.
• When you activate a plugin/theme that is still stored as paused/broken, you will end up in a redirect loop too.

46045.diff​ fixes this:

• It introduces a WP_SANDBOX_SCRAPING constant and sets it whenever it is loading plugin/theme code in some "sandbox" mode.
• The fatal error handler bails if that constant is set and true so that it no longer conflicts.

[ohiosierra]

Tested @flixos90 's patch.

It does fix the redirect loop for resuming a paused plugin with a fatal error. I do still get the redirect loop when re-activating a plugin that was paused with a fatal error and then deactivated.

As for this specific ticket, I was still not seeing the issue @ocean90 specified even before the patch. It was rolling back my fatal error change with the appropriate message and not triggering WSOD protection. I believe @xkon had the same experience based on our conversation on Slack, but he would have to confirm.

[flixos90]

Good catch @ohiosierra! 46045.2.diff​ adds the missing occurrence where that constant needs to be set too.

[flixos90]

In 45114:

Bootstrap/Load: Ensure WP_Fatal_Error_Handler does not conflict with existing mechanisms treating fatal errors.

Prior to this change, resuming or activating a plugin or theme that is still broken would result in a redirect loop if in recovery mode. If outside recovery mode, it would cause the error template to be displayed.

Furthermore this applies to breaking a plugin or theme when editing from the backend.

Props aandrewdixon, azaozz, dhanukanuwan, henrywright, ocean90, ohiosierra, PandelisZ, xkon.
Fixes #46045, #46751.

[flixos90]

In 45116:

Bootstrap/Load: Do not redefine constants because tests don't like it.

See #46045.

[tmdesigned]

@flixos90 that latest patch took care of the loop on activating a broken plugin, thanks. (this is @ohiosierra, sorry for using 2 usernames)

There is a slight difference in terms of display for un-pausing a broken plugin vs activating a broken plugin, but not a very significant one.

Un-pausing a broken plugin keeps it paused, showing the code problem, whereas activating a broken plugin keeps it deactivated, so there is no error message shown via the pause functionality. This seems natural to me since it's not paused, although maybe in the future plugins that fail to activate could do something similar.

[xkon]

In case this is helpful as well even though this was closed as I kept seeing the same message:

When testing with VVV or any other local setup but going via https ( with the generated certs ) it will continue to show the Unable to communicate back with site to check for fatal errors... message, it works via http though (there's no need to actually change anything in the file as well, just hitting the Update File without any changes).

On the other hand testing the same build on a live host with a proper ssl cert I'm getting the Your PHP code changes were rolled back due to an error....