WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 5 weeks ago

#44171 new enhancement

Flushing the rewrite rules is "wrong" on the front-end

Reported by: azaozz Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: General Keywords: dev-note has-patch
Focuses: Cc:

Description

Follow-up from #44142.

"Flushing" these rules takes a lot of resources, writes to the db, and (usually) overwrites .htaccess. Because of that it should only be performed in wp-admin, especially in production. Add a _doing_it_wrong() notice so theme and plugin authors are aware if this.

Attachments (1)

44171.diff (479 bytes) - added by kraftbj 5 weeks ago.
Props azaozz for the code, kraftbj for the location. Carried over from #44142.

Download all attachments as: .zip

Change History (10)

#1 @swissspidy
5 weeks ago

This will create lots and lots of notices. Many plugins and themes flush rewrite rules on init, because they got that from some tutorial or something.

Such a change should get in as early as possible.

#2 @desrosj
5 weeks ago

  • Keywords dev-note added

What if we started with a post on Make Core that explains the reasons why it is wrong to flush on the front end, and details the correct way to perform this action? We could also state in that post that it will be included in 5.0, and we could post that any time.

#3 @kraftbj
5 weeks ago

Sample first run: Rewrite rules should only be flushed within wp-admin

In 5.0 via #44171, rewrite rules can only be flushed within an administrative context (wp-admin).

Processing rewrite rules, generally, is an expensive operation, which is why WordPress stores a compiled set of rewrite rules in the database. When plugins or themes flush the rewrite rules on the front-end, this requires the site to perform the work of compiling the rewrite rules, performing two writes to the database, and, by default, updating the .htaccess or web.config to continue loading the site.

At best, this increases the site load time for visitors, but at worst, can overload the database and bring the site down depending on the number of concurrent visitors.

Starting in 5.0, a _doing_it_wrong notice will occur and the flush_rewrite_rules() will return early without action if is_admin is false. Ideally, flushing the rewrite rules should only occur when there has been a preceding action impacting rewrite rules, such as plugin or theme activation/deactivation. If actively developing a site, plugin, or theme, add_action( 'admin_init', 'flush_rewrite_rules' ); is acceptable. This is often best done via a dedicated development plugin to avoid accidentally shipping it in production code.

@kraftbj
5 weeks ago

Props azaozz for the code, kraftbj for the location. Carried over from #44142.

#4 follow-up: @kraftbj
5 weeks ago

Another kicker: WP CLI does not pass is_admin() == true. Should we have a filter that is heavily documented that it should only be used by WP CLI or in similar contexts where allowing non-admin writes is understood and acceptable?

#5 in reply to: ↑ 4 ; follow-up: @iandunn
5 weeks ago

Replying to desrosj :

We could also state in that post that it will be included in 5.0, and we could post that any time.

I'm wondering if it'd be best to post that as soon as possible (after this is committed), to give devs plenty of lead time? Many will already have a lot of work to do around Gutenberg, so spreading it out as much as possible will help minimize the pain.

If it is published soon, though, it'd be good to include a reminder in a 5.0 dev note round-up.

Replying to kraftbj:

Should we have a filter that is heavily documented that it should only be used by WP CLI or in similar contexts [...]?

Would it be better to just change the condition to if ( ! is_admin() && 'cli' !== php_sapi_name() ) { ?

#6 in reply to: ↑ 5 @kraftbj
5 weeks ago

Replying to iandunn:

Would it be better to just change the condition to if ( ! is_admin() && 'cli' !== php_sapi_name() ) { ?

I don't see any other times in Core that we check for CLI (using that or ( defined( 'WP_CLI' ) && WP_CLI ).

I think it would be fine to add that, but are we good with adding the first CLI check to Core?

#7 @desrosj
5 weeks ago

I like the first pass, @kraftbj. I can copy it over to Make if we are all comfortable with this plan.

Should we also bring this up in the Core Dev Chat this week for feedback?

#8 @desrosj
5 weeks ago

  • Keywords has-patch added

#9 @iandunn
5 weeks ago

Adding the CLI check seems appropriate to me in this case, but I'd definitely like to hear what others think. I don't think we should test for WP-CLI specifically, though, since there are other CLI scripts that do similar things.

Note: See TracTickets for help on using tickets.