8000 Improve shutdown callback by stodorovic · Pull Request #367 · Automattic/wp-super-cache · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improve shutdown callback #367

New issue

Have a question about this project? Sign up for a free GitHub a 8000 ccount to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Aug 28, 2017
Merged

Improve shutdown callback #367

merged 6 commits into from
Aug 28, 2017

Conversation

stodorovic
Copy link
@stodorovic stodorovic commented Aug 25, 2017 •
edited
Loading

Summary

There are two cases when shutdown callback is executed too early:

  • Some plugin died before main WP Query is created,.
  • PHP Fatal error occurred.

This PR is introducing function wpsc_is_fatal_error which uses error_get_last and stop caching.

Also, if output buffer is empty or main query doesn't exist then prevents calling wp_super_cache_query_vars.

Test instructions

  • Install plugin Pretty Link. Create redirect. Everything should work without errors.
  • Make page template which will trigger PHP Fatal error. Page will not be cached with notice in debug log.

@stodorovic stodorovic mentioned this pull request Aug 25, 2017
Merged
@donnchawp
Copy link
Contributor

I like this way of handling that situation. It's probably safer to not cache the page, but there are sites that simply exit early in the process so that wp_query isn't defined (Paul's one is an example), but still want their pages cached.

They should define wp_query of course, and maybe this is what the plugin will force users to do in the future, but if there are unmaintained plugins doing this, ordinary users won't have the skills to fix the problem.

Will think about this over the weekend.

@donnchawp
Copy link
Contributor

In the case of a fatal error I agree that the plugin should not cache the page, but the wp_query object is used by the plugin at the very end of the PHP process, after all WordPress code is supposed to have already run so I don't think there's much danger in creating it in the output buffer callback.

It's mostly only used to check if the plugin needs to ignore caching of particular types of pages too, so it's not depending on the WordPress loop or anything complicated.

Saying that, I suspect that creating that object will create a blank object where none of the is_* functions return true, but I have yet to test that.

@stodorovic
Copy link
Author
stodorovic commented Aug 26, 2017 •
edited
Loading

By all my tests, in 99% (in last few days) it's impossible to find any useful info from wp_query, so your suspicions are legitimate. Also, many plugins use die in action plugins_loaded and sometime init (for better performance) and creating object isn't enough. (without other actions, ... )

These type of plugins often use $_SERVER['REQUEST_URI'] and avoid almost all WP hooks. Type of requests are redirects and some kind of ajax requests. (Paul's case).

I've some PHP scripts which only include wp-load.php to load WP bootstrap (same as admin-ajax.php). Also in this case, creating object is useless.. So, it's my research.

From other side, you added code which block caching for unknown types, but later, we fixed caching of sitemaps, I'm thinking that we can add some filter there which enable/disable caching for unknown pages. Or even you can cache it. (it's possible to check is $buffer empty to avoid redirects).

I agree that handling PHP fatal errors are good option and we can add it regardless to WP query. By my testing, it works fine. I've enabled xdebug stack trace on all my websites and I'm trying to catch all PHP warnings/notices. (I've found some minor issues)

@stodorovic stodorovic mentioned this pull request Aug 27, 2 8000 017
Closed
@stodorovic
Copy link
Author

If found PHP notices in debug.log:

[23-Aug-2017 16:41:01 UTC] PHP Notice:  Undefined offset: 0 in wp-content/plugins/wp-super-cache/wp-cache-phase2.php on line 1521
[23-Aug-2017 16:41:01 UTC] PHP Stack trace:
[23-Aug-2017 16:41:01 UTC] PHP   1. shutdown_action_hook() wp-includes/load.php:0
[23-Aug-2017 16:41:01 UTC] PHP   2. do_action() wp-includes/load.php:677
[23-Aug-2017 16:41:01 UTC] PHP   3. WP_Hook->do_action() wp-includes/plugin.php:453
[23-Aug-2017 16:41:01 UTC] PHP   4. WP_Hook->apply_filters() wp-includes/class-wp-hook.php:323
[23-Aug-2017 16:41:01 UTC] PHP   5. wp_ob_end_flush_all() wp-includes/class-wp-hook.php:298
[23-Aug-2017 16:41:01 UTC] PHP   6. ob_end_flush() wp-includes/functions.php:3721
[23-Aug-2017 16:41:01 UTC] PHP   7. wp_cache_ob_callback() wp-includes/functions.php:3721
[23-Aug-2017 16:41:01 UTC] PHP   8. wp_cache_shutdown_callback() wp-content/plugins/wp-super-cache/wp-cache-phase2.php:465
[23-Aug-2017 16:41:01 UTC] PHP   9. wp_cache_post_id() wp-content/plugins/wp-super-cache/wp-cache-phase2.php:1108
[23-Aug-2017 18:02:01 UTC] PHP Notice:  Undefined offset: 1 in wp-content/plugins/wp-super-cache/wp-cache-phase2.php on line 1511
[23-Aug-2017 18:02:01 UTC] PHP Stack trace:
[23-Aug-2017 18:02:01 UTC] PHP   1. shutdown_action_hook() wp-includes/load.php:0
[23-Aug-2017 18:02:01 UTC] PHP   2. do_action() wp-includes/load.php:677
[23-Aug-2017 18:02:01 UTC] PHP   3. WP_Hook->do_action() wp-includes/plugin.php:453
[23-Aug-2017 18:02:01 UTC] PHP   4. WP_Hook->apply_filters() wp-includes/class-wp-hook.php:323
[23-Aug-2017 18:02:01 UTC] PHP   5. wp_ob_end_flush_all() wp-includes/class-wp-hook.php:298
[23-Aug-2017 18:02:01 UTC] PHP   6. ob_end_flush() wp-includes/functions.php:3721
[23-Aug-2017 18:02:01 UTC] PHP   7. wp_cache_ob_callback() wp-includes/functions.php:3721
[23-Aug-2017 18:02:01 UTC] PHP   8. wp_cache_get_ob() wp-content/plugins/wp-super-cache/wp-cache-phase2.php:464
[23-Aug-2017 18:02:01 UTC] PHP   9. wp_cache_microtime_diff() wp-content/plugins/wp-super-cache/wp-cache-phase2.php:596

I was able way how to reproduce them and find root of cause. I made new commit which also fixes some unnecessary whitespace. If it needs, I can elaborate more about changes. I'm testing it on all websites and I didn't notice new PHP notices except an issue related to is_front_page in WP core (related to https://core.trac.wordpress.org/ticket/30210).

@donnchawp
Copy link
Contributor

Thanks for testing that! I agree we will need a filter on the unknown page caching. The debug message should inform the site owner how to enable it too.

stodorovic and others added 2 commits August 28, 2017 11:27
@stodorovic
Add is_404 into wp_super_cache_query_vars
d1051f0
@donnchawp
Added wpsc_only_cache_known_pages filter
ceb4379 
The wpsc_only_cache_known_pages filter returns 1  so that unknown page types are not cached. If the filter returns 0 even unknown page types will be cached, if all other conditions for caching are satisfied.
@donnchawp
Copy link
Contributor

I added the filter "wpsc_only_cache_known_pages" to your PR. That should let site owners cache these types of pages.

@stodorovic
Copy link
Author

Thanks for filter! You are welcome 😃

I added is_404 into wp_super_cache_query_vars because $wp_super_cache_query was empty in this case. It seems that wp_super_cache_query_vars covers all cases now. I'll add function (on all my websites) which uses this filter and logs all unknown page types into separate logs. (and analyze all particular cases again)

@donnchawp
Copy link
Contributor

I'm hoping to make a new release today in the next 5 hours. It would be great to have this PR in there but otherwise it can wait until the next one. I think it looks ok, but will wait for your testing.

The non-caching of fatal errors is enough for a new point release in a week if we need more time.

@stodorovic
Copy link
Author
stodorovic commented Aug 28, 2017 •
edited
Loading

Based on your checking of apply_filters, I added as additional protection of fatal errors method_exists( $GLOBALS['wp_query'], 'get'). I think that's safe related to fatal errors. Please check again the code, I think that's correct.

So, we should merge this PR. If we need to fix something else, we will create new PR.

@donnchawp donnchawp merged commit 92a82b0 into Automattic:master Aug 28, 2017
@stodorovic stodorovic deleted the improve-shutdown-callback branch December 18, 2017 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stodorovic @donnchawp
0