WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 7 months ago

#44171 assigned enhancement

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

Reported by: azaozz Owned by: kraftbj
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: dev-note has-patch
Focuses: Cc:
PR Number:

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 17 months ago.
Props azaozz for the code, kraftbj for the location. Carried over from #44142.

Download all attachments as: .zip

Change History (14)

#1 @swissspidy
17 months 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
17 months 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
17 months 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
17 months ago

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

#4 follow-up: @kraftbj
17 months 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
17 months 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
17 months 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
17 months 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
17 months ago

  • Keywords has-patch added

#9 @iandunn
17 months 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.

#10 @pento
12 months ago

  • Milestone changed from 5.0 to 5.1

#11 @pento
9 months ago

  • Milestone changed from 5.1 to Future Release

The number of warnings this will generate makes me nervous. 🙂

It seems like we could try some outreach to popular plugin/theme authors to get them to fix this behaviour, before we start enforcing it.

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


9 months ago

#13 @kraftbj
7 months ago

  • Owner set to kraftbj
  • Status changed from new to assigned

What's the best way to identify some of these plugins or themes that do that? I'd like to move forward with this so we hopefully add it sometime in 2019.

Note: See TracTickets for help on using tickets.