WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 4 days ago

Last modified 4 days ago

#44142 closed defect (bug) (fixed)

Load Order: save_mod_rewrite_rules assumes wp-admin/includes/file.php is loaded

Reported by: YuriV Owned by: kraftbj
Milestone: 4.9.7 Priority: normal
Severity: normal Version: 4.9.6
Component: Bootstrap/Load Keywords: has-patch commit
Focuses: Cc:

Description

Hi, I'm not completely sure if it's a bug but it definitely caused us some problems. After updating WP to 4.9.6. this morning, we received http 500 erros on our frontpage. The errorlog came up with this. The stack (below) gave us no info about a possible plugin causing this. Our host helped us out by editing misc.php

On line 206 the added:

                if ( ! function_exists( 'get_home_path' ) ) {
                        include_once ABSPATH . '/wp-admin/includes/file.php';
                }

FYI: we never edited core WP files before

Error log after the update to 4.9.6:

==> /var/log/php/error.log <==
[18-May-2018 07:26:39 UTC] PHP Fatal error:  Uncaught Error: Call to undefined function get_home_path() in /data/www/historiek.net/wp-admin/includes/misc.php:204
Stack trace:
#0 /data/www/historiek.net/wp-includes/class-wp-rewrite.php(1775): save_mod_rewrite_rules()
#1 /data/www/historiek.net/wp-includes/class-wp-hook.php(286): WP_Rewrite->flush_rules(true)
#2 /data/www/historiek.net/wp-includes/class-wp-hook.php(310): WP_Hook->apply_filters(NULL, Array)
#3 /data/www/historiek.net/wp-includes/plugin.php(453): WP_Hook->do_action(Array)
#4 /data/www/historiek.net/wp-settings.php(471): do_action('wp_loaded')
#5 /data/www/historiek.net/wp-config.php(98): require_once('/data/www/histo...')
#6 /data/www/historiek.net/wp-load.php(37): require_once('/data/www/histo...')
#7 /data/www/historiek.net/wp-blog-header.php(13): require_once('/data/www/histo...')
#8 /data/www/historiek.net/index.php(17): require('/data/www/histo...')
#9 {main}
  thrown in /data/www/historiek.net/wp-admin/includes/misc.php on line 204

Attachments (5)

44142.diff (610 bytes) - added by kraftbj 5 weeks ago.
require_once file.php to ensure get_home_path is declared.
44142.2.diff (3.1 KB) - added by azaozz 5 weeks ago.
44142.3.diff (3.3 KB) - added by kraftbj 4 weeks ago.
doing_it_wrong on any non-admin flush
44142.4.diff (2.8 KB) - added by kraftbj 4 weeks ago.
Focus the patch on the load order issue at hand
44142.5.diff (2.8 KB) - added by kraftbj 4 weeks ago.
Same as .4 with "Plugin Handbook" title reference corrected.

Download all attachments as: .zip

Change History (39)

#1 @YuriV
5 weeks ago

PS: The error was not only on the frontpage, but on all articles pages. Backend was working fine.

#2 @iandunn
5 weeks ago

Can you paste in a list of all the plugins that are active, and the theme too?

#3 @YuriV
5 weeks ago

Hi Ian,

Hereby.

Theme Rhonda 1.5 (Inibot)

Plugins

Accelerated Mobile Pages (0.9.86.2)
Ad Inserter (2.3.8)
Akismet Anti-Spam (4.0.3)
Autoptimize (2.3.4)
Better Search Replace (1.3.2)
Bounce Handler Mailpoet (1.3.8)
Co-Authors Plus (3.3.0)
Code-snippets (2.10.1.1)
Cookie Notice (1.2.42)
Disable Comments (1.7.1)
Doneren met Mollie (2.4.11)
Easy FancyBox (1.8.3)
EWWW Image Optimizer (4.2.1)
GoodBarber Api (1.0.12)
Jetpack (6.1)
MailPoet 3 (3.7.2)
MailPoet 3 Premium (3.0.18)
Q2W3 Fixed Widget (5.1.4)
Redirection (3.2)
Rich Text Tags, Categories, and Taxonomies (1.8)
Schema (3.0.1)
Use Google Libraries (1.6.2.3)
User Rol Editor (4.42)
Varnish HTTP Purge (4.5.0)
Widget CSS Classes (1.5.2.1)
WP Subtitle (3.0)
Yoast SEO (7.5.1)

I did a Google Search and found this: https://wordpress.org/support/topic/undefined-function-get_home_path/ So it might have something to do with Mailpoet.

Also found this: https://wordpress.org/support/topic/error-500-on-all-pages-after-wordpress-4-9-6-update/

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 weeks ago

#5 @kraftbj
5 weeks ago

  • Component changed from General to Bootstrap/Load
  • Milestone changed from Awaiting Review to 4.9.7
  • Owner set to kraftbj
  • Status changed from new to assigned

@SergeyBiryukov identified the exact path for this particular error, which is caused by wp-admin/includes/misc.php being loaded via wp_add_privacy_policy_content() when an rewrite flush is requested outside of the admin context. That combo leads to save_mod_rewrite_rules function_exists check to pass. That function assumes wp-admin/includes/file.php has been loaded, which isn't true in this case.

Patch incoming.

@kraftbj
5 weeks ago

require_once file.php to ensure get_home_path is declared.

#6 @kraftbj
5 weeks ago

  • Keywords has-patch added

#7 @iandunn
5 weeks ago

If we can programmatically detect the problem, we could also consider adding a _doing_it_wrong() call, to notify the plugin author which hook they should use.

#8 @kraftbj
5 weeks ago

I don't think a doing_it_wrong is merited. Generally, the issue is more our privacy code includes a file that didn't expect itself to be loaded without the rest of the admin suite.

A good Todo is audit the rest of misc.php to see if there are other instances of similar assumptions. I can check that out this afternoon.

#9 @kraftbj
5 weeks ago

I made a relatively quick scan of misc.php and I didn't see other functions that I would expect to not be loaded—mainly things from wp-includes/functions.php.

#10 @jcobrien
5 weeks ago

Might be related, but upgrading to 4.9.6 made editing variations / attributes in woo commerce go into an infinite loading loop. I verified this on two cloned installations. The only change between working and non-working was this update.

I'd like to add that I replicated this issue using only two active plugins, woo commerce and tinymce.

Version 2, edited 5 weeks ago by jcobrien (previous) (next) (diff)

#11 follow-up: @kraftbj
5 weeks ago

  • Summary changed from 500 error after update to Load Order: save_mod_rewrite_rules assumes wp-admin/includes/file.php is loaded

@jcobrien Can you report that to WooCommerce support via https://woocommerce.com/my-account/tickets/ or https://wordpress.org/support/plugin/woocommerce ? I haven't personally heard of that, but I would not expect it to be related to this specific ticket.

If you could submit that (and let me know you've done so), as soon as possible, that would be extremely helpful. I know WooCommerce is aiming for an update related to GDPR pretty soon.

#12 in reply to: ↑ 11 ; follow-up: @jcobrien
5 weeks ago

I have submitted I ticket. I should add that I got a 404 error after update to 4.9.6. I also cannot view the privacy setttings *privacy.php did not exist. and I get a 404 on viewing cron events.

#13 in reply to: ↑ 12 @azaozz
5 weeks ago

Replying to jcobrien:

I got a 404 error after update to 4.9.6. I also cannot view the privacy settings *privacy.php did not exist.

This sounds like a partial/failed upgrade. Please reinstall WP. If the problem persists, perhaps post to the support forum.

Last edited 5 weeks ago by azaozz (previous) (diff)

@azaozz
5 weeks ago

#14 @azaozz
5 weeks ago

In 44142.2.diff:

  • Add _doing_it_wrong() and return early when a wrong hook was used to call wp_add_privacy_policy_content().
  • Add _doing_it_wrong() when rewrite rules are flushed outside wp-admin context.

Also wondering if we should just return early for attempts to flush the rewrite rules outside wp-admin instead of making it possible. Seems that didn't work/wasn't possible before 4.9.6.

Note that in normal circumstances wp_add_privacy_policy_content() cannot be used on the init hook. It throws a fatal error right away as wp-admin/includes/admin.php (Core Administration API) is not loaded yet :)

This ticket was mentioned in Slack in #core by azaozz. View the logs.


5 weeks ago

#16 @kraftbj
5 weeks ago

Also wondering if we should just return early for attempts to flush the rewrite rules outside wp-admin instead of making it possible. Seems that didn't work/wasn't possible before 4.9.6.

General flushing of the rewrite rules worked (and still does without a fatal), but hard flushes used to not work and now lead to the fatal described in this ticket under a perfect storm. https://core.trac.wordpress.org/browser/tags/4.9.6/src/wp-includes/class-wp-rewrite.php#L1774

From a performance standpoint, flushing rewrite rules on the front end is a bad idea, but I think we should split that off to a separate ticket since who knows what plugins have been doing. Since we can reasonably fix this issue without doing that, let's not conflate the two issues (loading and new code being hooked in an unintended place vs front-end rewrite flushing).

@kraftbj
4 weeks ago

doing_it_wrong on any non-admin flush

#17 follow-up: @kraftbj
4 weeks ago

@azaozz What about indicating that rules should only be flushed within wp-admin within the flush_rewrite_rules function that is typically called, as seen in 44142.3.diff?

#18 in reply to: ↑ 17 @SergeyBiryukov
4 weeks ago

Replying to kraftbj:

What about indicating that rules should only be flushed within wp-admin within the flush_rewrite_rules function that is typically called, as seen in 44142.3.diff?

44142.3.diff has a non-zero chance of displaying unexpected notices on front end if someone accidentally has WP_DEBUG enabled on production.

Developer Reference has an example of flushing rewrite rules on front end every 48 hours for theme development.

I'd be more comfortable with landing this notice in a major release with a dev note written at beta stage than in an automatically installed minor update.

I have no objections to the notice in wp_add_privacy_policy_content() though, since that's new functionality.

#19 @azaozz
4 weeks ago

What about indicating that rules should only be flushed within wp-admin within the flush_rewrite_rules function that is typically called, as seen in 44142.3.diff?

Yeah, makes sense.

I'd be more comfortable with landing this notice in a major release...

Makes sense too. Also perhaps we can add to the "doing it wrong" message that flushing the rules is acceptable only in development.

...I think we should split that (doing it wrong notice when flushing the rules) off to a separate ticket

Agreed. Lets make a ticket with milestone for the next major release.

Last edited 4 weeks ago by azaozz (previous) (diff)

#20 @azaozz
4 weeks ago

Created #44171 as follow-up.

#21 @kraftbj
4 weeks ago

Agreed with all of the above. In .4, everything from .3 except removing the notice thrown for rewrite rules. Will add that to #44171.

@kraftbj
4 weeks ago

Focus the patch on the load order issue at hand

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 weeks ago

#23 @azaozz
4 weeks ago

44142.4.diff looks good.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 weeks ago

#25 @chetan200891
4 weeks ago

Just a minor suggestion for DocBlock in 44142.4.diff.

Can we use Plugin Handbook instead of Plugins Handbook? For consistent wording. Because https://developer.wordpress.org/plugins/privacy/suggesting-text-for-the-site-privacy-policy/ page title is Plugin Handbook.

@kraftbj
4 weeks ago

Same as .4 with "Plugin Handbook" title reference corrected.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 weeks ago

#27 @kraftbj
4 weeks ago

  • Keywords commit added

This ticket was mentioned in Slack in #meta by shelob9. View the logs.


3 weeks ago

#29 @Shelob9
2 weeks ago

I wanted to add a link to a conflict related to this we are having for context.

I will add more details if I can find them, but it appears this issue also happens when Caldera Forms is active with Ultimate Member active and persists after the update plugin process. It also happens during update if Divi is active in some way (I can't reproduce this well.)

https://github.com/CalderaWP/Caldera-Forms/issues/2574#issuecomment-394825977

#30 @desrosj
11 days ago

The save_mod_rewrite_rules() function's inline documentation is missing a @return tag. We can add that when we commit this.

#31 @SergeyBiryukov
4 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 43361:

Privacy: Make sure wp_add_privacy_policy_content() does not cause a fatal error by unintentionally flushing rewrite rules outside of the admin context.

Add a _doing_it_wrong() message describing the correct usage of the function.

Props kraftbj, azaozz, SergeyBiryukov, YuriV.
Fixes #44142.

#32 @SergeyBiryukov
4 days ago

In 43362:

Docs: Add missing @return value for save_mod_rewrite_rules() and iis7_save_url_rewrite_rules().

See #44142.

#33 @SergeyBiryukov
4 days ago

In 43363:

Docs: Correct inline comment added in [43361] for consistency with other comments.

See #44142.

#34 @SergeyBiryukov
4 days ago

In 43364:

Privacy: Make sure wp_add_privacy_policy_content() does not cause a fatal error by unintentionally flushing rewrite rules outside of the admin context.

Add a _doing_it_wrong() message describing the correct usage of the function.

Props kraftbj, azaozz, SergeyBiryukov, YuriV.
Merges [43361], [43362], [43363] to the 4.9 branch.
Fixes #44142.

Note: See TracTickets for help on using tickets.