WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 18 months ago

#30501 closed defect (bug) (fixed)

Prevent flushing rewrites during page load

Reported by: joostdevalk Owned by: SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: Permalinks Keywords: 4.2-early has-patch commit
Focuses: Cc:
PR Number:

Description

Flushing rewrites during the pageload breaks a lot of plugins. They might not have registered their rewrite yet which would cause issues. I propose preventing the flushing from working and forcing it to be run on shutdown instead.

Attachments (6)

rewrite.2.patch (648 bytes) - added by joostdevalk 5 years ago.
Original patch
rewrite.patch (668 bytes) - added by joostdevalk 5 years ago.
Patch for did_action( 'init' )
rewrite-wp-loaded.patch (678 bytes) - added by joostdevalk 5 years ago.
Rewrite to wp-loaded patch (v3)
30501.patch (906 bytes) - added by mordauk 5 years ago.
30501.2.patch (972 bytes) - added by DrewAPicture 5 years ago.
spacing
30501.3.patch (976 bytes) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (40)

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


5 years ago

#2 @joostdevalk
5 years ago

Related to but not the same as #29118.

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


5 years ago

#4 @rmccue
5 years ago

I'm +1 on adding this, for sure. I don't think there's any issues with changing this that I can see.

(Note that this solves a different problem to the one I'd discussed previously in ticket:29118#comment:5: I was aiming to avoid ever calling flush rewrites, whereas the problem here is the infamous flush-before-registration.)

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


5 years ago

#6 @joostdevalk
5 years ago

  • Milestone changed from Awaiting Review to 4.1

#7 @joostdevalk
5 years ago

  • Milestone changed from 4.1 to Future Release

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


5 years ago

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


5 years ago

#10 @joostdevalk
5 years ago

Just updated the patch to check for did_action( 'init' ) so you can't flush too early anymore, but the flush would still work on the current pageload.

@joostdevalk
5 years ago

Original patch

@joostdevalk
5 years ago

Patch for did_action( 'init' )

#11 @mboynes
5 years ago

@joostdevalk, how about pushing this out of init, perhaps to wp_loaded? I can imagine plenty of plugins may register rewrites after priority 10 of init, and this patch wouldn't solve this problem for them. wp_loaded is the last action that fires before WP::parse_request() (where the rewrite rules are used), so it seems like a decent time to regenerate should that be requested.

#12 @joostdevalk
5 years ago

Good point @mboynes, uploading new patch.

@joostdevalk
5 years ago

Rewrite to wp-loaded patch (v3)

#13 @rmccue
5 years ago

  • Keywords 4.2-early added

rmccue approves! If we're going to do this (and we should), let's try and do it early in 4.2 so that we find any plugin breakages as soon as possible.

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


5 years ago

#15 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 4.2

#16 @SergeyBiryukov
5 years ago

  • Keywords commit added

#17 @SergeyBiryukov
5 years ago

In 31104:

If WP_Rewrite::flush_rules() is called on 'init' or earlier, wait until 'wp_loaded' before actually flushing the rules, to make sure all the rules registered on 'init' are included.

props joostdevalk.
see #30501.

#18 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#19 @pavelevap
5 years ago

I am not sure if I understand it well, please see example code:

function some_rewrite() {
  global $wp_rewrite;
  $wp_rewrite->author_base = 'something';
  $wp_rewrite->flush_rules();
}
add_action('init', 'some_rewrite');

Calling $wp_rewrite->flush_rules() is wrong here because it is called during every page load. What is the right way for users when they want to change author to something else? And they need only quick hack for functions.php (and not flushing rewrite rules during plugin activation).

I saw many times this piece of code. It will work in 4.2 and only running $wp_rewrite->flush_rules() function will be automatically moved to wp_loaded?

#20 follow-up: @joostdevalk
5 years ago

Flushing on every page load is still bad and this patch does not prevent that. What this DOES prevent is a plugin or theme running flush_rewrite_rules before plugins have had a chance to register their rewrites, which causes URLs to break. So it doesn't prevent it from running, it just moves it.

#21 in reply to: ↑ 20 ; follow-up: @Funkatronic
5 years ago

Replying to joostdevalk:

Flushing on every page load is still bad and this patch does not prevent that. What this DOES prevent is a plugin or theme running flush_rewrite_rules before plugins have had a chance to register their rewrites, which causes URLs to break. So it doesn't prevent it from running, it just moves it.

Is add_action smart enough to call flush_rewrite_rules only once vs the n amount of calls various plugins will do?

#22 in reply to: ↑ 21 @SergeyBiryukov
5 years ago

Replying to Funkatronic:

Is add_action smart enough to call flush_rewrite_rules only once vs the n amount of calls various plugins will do?

Yes.

#23 follow-up: @SergeyBiryukov
5 years ago

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

[31104] seems to work as expected.

#24 in reply to: ↑ 23 @Denis-de-Bernardy
5 years ago

Replying to SergeyBiryukov:

[31104] seems to work as expected.

Is there any reason for dumping the $hard argument? From the looks of the changeset, if you try to do a soft flush before wp_loaded, you'll end up doing a hard one later on instead.

#25 @SergeyBiryukov
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Good point.

#26 @Denis-de-Bernardy
5 years ago

A possible workaround off the top of my head:

static $do_hard_later;

if ( ! did_action( 'wp_loaded' ) ) { 
  add_action( 'wp_loaded', array( $this, 'flush_rules' ) );
  $do_hard_later ||= $hard;
  return; 
}
elseif ( isset( $do_hard_later ) && doing_action( 'wp_loaded' ) ) {
  $hard ||= $do_hard_later;
  $do_hard_later = null;
}

#27 @DrewAPicture
5 years ago

  • Keywords needs-patch added; has-patch commit removed

@Denis-de-Bernardy: Would you care to follow up with a patch? It seems solving the $hard issue is the only thing preventing this ticket from being resolved in 4.2.

@mordauk
5 years ago

#28 @mordauk
5 years ago

30501.patch should take care of the $hard issue.

@DrewAPicture
5 years ago

spacing

#29 @DrewAPicture
5 years ago

  • Keywords has-patch commit added; dev-feedback needs-patch removed

#30 @SergeyBiryukov
5 years ago

30501.2.patch doesn't account for a situation where flush_rules() is called with $hard = true and then with $hard = false. It would only do a soft flush in that case.

30501.3.patch fixes that.

#31 @SergeyBiryukov
5 years ago

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

In 31964:

When shifting WP_Rewrite::flush_rules() to a later action if it was called too early, make sure to do a hard flush if requested.

props Denis-de-Bernardy, mordauk for initial patch.
fixes #30501.

#32 @selnomeria
5 years ago

please, tell the world (all plugins developers, in a big RED NOTICE, that they should disable flushing on every page load...
they'd better to use smth like this:

if (get_option($postype . "_checkpoint_12345")) {
   ....flush... 
   ....delete_option($postype . "_checkpoint_12345");........
}
Last edited 5 years ago by selnomeria (previous) (diff)

#33 @dd32
4 years ago

#32023 was marked as a duplicate.

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


18 months ago

Note: See TracTickets for help on using tickets.