Skip to content

Catch WSODs and provide a means for recovery for end users#3

Closed
felixarntz wants to merge 73 commits into
masterfrom
44458
Closed

Catch WSODs and provide a means for recovery for end users#3
felixarntz wants to merge 73 commits into
masterfrom
44458

Conversation

@felixarntz

Copy link
Copy Markdown
Member

This is the PR accompanying https://core.trac.wordpress.org/ticket/44458.

This is only for development, discussion should continue to happen on the Trac ticket as usual.

@felixarntz

Copy link
Copy Markdown
Member Author

The initial commit here is based on the state of 44458.6.diff.

@felixarntz felixarntz changed the title Port-over 44458.6.diff from Trac. Catch WSODs and provide a means for recovery for end users Jul 30, 2018
When we are on a protected endpoint, we redirect to the same page after we paused a plugin.

This has two effects:

* The protected endpoints are always immediately available, they don't show the error message on first encounter.
* The flow is exactly the same, whether only one plugin or multiple plugins are broken. So, if you update PHP and ten plugins break at once, you still get into the admin backend with one click, and the ten plugins will be paused already.
Comment thread src/wp-admin/plugins.php Outdated
@felixarntz

Copy link
Copy Markdown
Member Author

I'm not entirely sure about dbe5bc0 yet (see https://core.trac.wordpress.org/ticket/44458#comment:33), we might need to discuss this further.

Comment thread src/wp-admin/includes/class-wp-plugins-list-table.php Outdated
Comment thread src/wp-admin/includes/class-wp-plugins-list-table.php Outdated
Comment thread src/wp-admin/includes/class-wp-plugins-list-table.php Outdated
Felix Arntz and others added 22 commits January 7, 2019 16:37
…an attack vector"

This reverts commit 5d38162 as it
requires another discussion.
* @return string Error message HTML output.
*/
protected function get_error_message_markup() {
if ( ! function_exists( '__' ) ) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call wp_load_translations_early(); here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but that sounds like it might be too risky for a shutdown handler...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @schlessera. We might not even have the database initialized and thus might not even know which locale to use. Stubbing __() is not risky in that regard.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that is the point of the function, it doesn’t call the database or cache, but does it best to load translations. It loads in wp_db class super early. This function is designed exactly for this...

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stubbing __() is safe, but it also means that the translation do not work at all. Trying to support translations if possible.

@TimothyBJacobs

Copy link
Copy Markdown
Collaborator

I opened #4 as requested to implement an opt-out mechanism via a plugin header.

@felixarntz

Copy link
Copy Markdown
Member Author

This has been fixed (see https://core.trac.wordpress.org/ticket/44458#comment:56). Further work should happen through separate Trac tickets and patches or PRs.

@felixarntz felixarntz closed this Jan 9, 2019
@felixarntz felixarntz deleted the 44458 branch January 30, 2019 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants