WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 weeks ago

#18450 new enhancement

New safe action to add rewrite rules on

Reported by: simonwheatley Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.2.1
Component: Rewrite Rules Keywords: has-patch dev-feedback needs-refresh
Focuses: Cc:

Description

Currently I don't believe it's possible to meet the following two criteria:

  • Not flush rewrite rules on every page load
  • Ensure that you always have your rewrite rules available

The problem arises when Plugin A has not yet added it's rewrite rules, but Plugin B calls flush_rewrite_rules. Plugin A is a good citizen, and doesn't call flush_rewrite_rules unless it needs to and so now it's rewrite rules are no longer present. (See http://wordpress.stackexchange.com/questions/26029/when-to-call-add-rewrite-rule-for-safety for more.)

Westi suggested that hooks on delete_option and get_option of 'rewrite_rules' might work. This covers almost all situations, except the one where permalinks are off and then get turned on again when neither the delete_option or get_option actions/filters are fired.

Devs could hook the new action for their add_rewrite_rule calls, and use their own methodology to determine when to call flush_rewrite_rules.

Two attachments:

  • Example plugin, showing (hopefully) the problem with the existing situation
  • Diff showing where the hook might be added

Attachments (6)

pre_flush_rewrite_rules action.diff (568 bytes) - added by simonwheatley 3 years ago.
Adds a new action, run just before flushing rewrite rules
18450.diff (2.9 KB) - added by scribu 3 years ago.
18450.2.diff (5.3 KB) - added by scribu 3 years ago.
demo.php (2.3 KB) - added by simonwheatley 3 years ago.
Refactored to use the flush on activation method
doing_it_wrong.18450.diff (710 bytes) - added by scribu 3 years ago.
rewrite.diff (1.8 KB) - added by joostdevalk 2 years ago.
Patch that hooks on shutdown and blocks frontend flushes

Download all attachments as: .zip

Change History (41)

simonwheatley3 years ago

Adds a new action, run just before flushing rewrite rules

comment:1 mikeschinkel3 years ago

  • Cc mikeschinkel@… added

comment:2 follow-up: nacin3 years ago

sil_call_add_rewrite_rules() should be called on init, always.

On activate: Register rewrite rules and flush

On init: Register rewrite rules

Only with capabilities and roles do you want to always avoid running the functions at runtime.

comment:3 scribu3 years ago

Hooking into 'get_option_rewrite_rules' is an ugly hack.

+1 on 'pre_flush_rewrite_rules' hook.

comment:4 scribu3 years ago

Other advantages of 'pre_flush_rewrite_rules':

  • lower memory footprint: $wp_rewrite isn't filled with additional rules on each page load
  • simpler: you only need to hook into one place, instead of 'init' + activation hook

This hook should be used by register_post_type() as well, instead of immediately calling $wp_rewrite. Idem for register_taxonomy().

comment:5 scribu3 years ago

It could just be a single callback that goes through all the registered post types and adds the necessary rules for each.

comment:6 scribu3 years ago

18450.diff implements what I said above for register_post_type().

scribu3 years ago

comment:7 scribu3 years ago

18450.2.diff does the same for register_taxonomy().

scribu3 years ago

comment:8 in reply to: ↑ 2 ; follow-up: simonwheatley3 years ago

Replying to nacin:

sil_call_add_rewrite_rules() should be called on init, always.

On activate: Register rewrite rules and flush

On init: Register rewrite rules

The situation with hooking init to add_rewrite_rule calls is uncertain: If you run add_rewrite_rules on init at priority 10, then conditionally flush the rules, there's no guarantee that all rules from other plugins are there at that point. So perhaps you decide to run all your add_rewrite_rule calls with a priority of 0 and all your flushes with a priority of 10… you're still not entirely safe though, as some other plugin could run flush at an earlier point in the init calls at some ridiculously early priority.

The addition of a pre_flush_rewrite_rules hook makes life a lot simpler for the future.

comment:9 in reply to: ↑ 8 ; follow-up: dd323 years ago

Replying to simonwheatley:

Replying to nacin:

sil_call_add_rewrite_rules() should be called on init, always.

On activate: Register rewrite rules and flush

On init: Register rewrite rules

The situation with hooking init to add_rewrite_rule calls is uncertain: If you run add_rewrite_rules on init at priority 10, then conditionally flush the rules

Now that, You definately shouldn't do (flush on init).

The current best way, and the only foolproof cross-compatible method that should have ever been used is as nacin said:

On init, Register the rules. - This is so that the rules are loaded for other plugins if they flush, You should even be able to run this on admin_init if you don't want to add a few KB to the front end memory usage.
AND
On activation, Register the rules AND flush the rules. - This is to set the rules up, add them first (as init has already been called before the plugin was included), and then flush the rules to add them to the database, other plugins rules should've been loaded on init or admin_init at this point, so all plugins play nicely.

The current situation is pretty common really, You should do the same thing when adding Taxonomies or Post Types, Register them on init, Register them in the activation function & flush.. If you're doing it any other way, I question why someone hasn't made documentation clearer for that..

So; Adding another action, technically, will reduce memory usage (less array items in $wp_rewrite, but realy.. what, 1KB?), but will still require flushing the rules on activation, it'll save ONE line of code in most cases (no need for $this->addRules(); in the activation function before flush_rewrite_rules();)

If you've got a plugin installed which has to upgrade itself after upgrades, and it's modifyin rewrite rules, it should only do that after admin_init (or, late on admin_init), doing so before then (or on init) is a bad thing to do (as it won't play nice with plugins which add rules, AND you shouldn't do processes like that on the front end requests..)

comment:10 scribu3 years ago

Ok, so the advantage of the hook is that it's clearer, it saves an additional line of code for plugin developers and even saves a little bit of memory.

What are the drawbacks?

comment:11 scribu3 years ago

Actually, since verbose rules are gone, do we even need to cache rules in wp_options?

comment:12 in reply to: ↑ 9 simonwheatley3 years ago

Replying to dd32:

Now that, You definately shouldn't do (flush on init).

Agreed; apologies for the confusion, which has taken away from the point I was trying to make.

The methods described above are not fool-proof, I believe, as they rely on the add_rewrite_rules call coming before any flush_rewrite_rules call. As the demo plugin (refactored version attached, using the method Nacin and DD32 described) shows, all it needs is some unintentionally keen plugin author to come along and flush the rules at an inopportunely early point and the rewrite rules we added are no longer in place.

This additional action will make it possible to know that your rewrite rules are definitely in place each and every time flush_rewrite_rules gets called.

simonwheatley3 years ago

Refactored to use the flush on activation method

comment:13 follow-up: aaroncampbell3 years ago

I'm all for keeping the barrier to entry as low as possible, but I think in this situation the solution is to educate those that are doing it wrong rather than add new actions. The truth is that if everyone did it right things would work perfectly well, so let's teach them to do it right.

comment:14 in reply to: ↑ 13 simonwheatley3 years ago

Replying to aaroncampbell:

I'm all for keeping the barrier to entry as low as possible, but I think in this situation the solution is to educate those that are doing it wrong rather than add new actions. The truth is that if everyone did it right things would work perfectly well, so let's teach them to do it right.

Educate everyone, really? Everyone? Seriously though, I seem to be losing this one, so I'm going to attempt to gracefully retire. :)

comment:15 GaryJ3 years ago

Not everyone, just those who are doing it wrong...

Could flush_rewrite_rules() have a call to _doing_it_wrong() if it's called at the wrong time?

comment:16 scribu3 years ago

Good idea: check did_action( 'init' )

comment:17 scribu3 years ago

You could actually go so far as refusing to flush the rules, besides the _doing_it_wrong() call.

comment:18 dd323 years ago

There was a similar discussion in #wordpress-dev, and I suggested:

if ( ! did_action('admin_init') )
   _doing_it_wrong('Never flush rules before admin_init, and NEVER on a front-end page load!, Do it on plugin activation only unless you\'re "special".');

Of course, not everyone is going to notice the message/error, but at least those that care about their programming will.. You'll still be at the mercy of cowboys.

comment:19 scribu3 years ago

Even better. I think the "unless you're special" bit doesn't help much. How about just "Do it on plugin activation."?

comment:20 dd323 years ago

I think the "unless you're special" bit doesn't help much. How about just "Do it on plugin activation."?

Purely for comical relief :) A patch shouldn't incorporate humour that isn't well known :)

scribu3 years ago

comment:21 scribu3 years ago

Purely for comical relief :) A patch shouldn't incorporate humour that isn't well known :)

Gotcha. :)

See doing_it_wrong.18450.diff

comment:22 ejdanderson2 years ago

  • Cc ejdanderson@… added

+1

comment:23 follow-up: markjaquith2 years ago

I think we can do better. The problem is that Plugin A might flush before Plugin B has added its rule. We can add a new hook, but there are lots of well-behaved plugins that add their rules on 'admin_init', but are getting their rules erased by plugins that flush too early.

When a plugin flushes the rules, it generally doesn't need that rule to be present on the current page load. So, why don't we just change flush_rewrite_rules() so that instead of immediately flushing, it queues a flush for shutdown. That way, all other plugins get their shot to add their rules. And if multiple plugins are calling flushes, we'll just do one, at the end of the page load.

We should, however, also considering blocking front end flushes and give them a "doing it wrong" notice.

Also worth thinking about is how we could use hashing to do automatic flushes when a new plugin adds its rule. There has to be a performant way to do that. Thoughts?

joostdevalk2 years ago

Patch that hooks on shutdown and blocks frontend flushes

comment:24 joostdevalk2 years ago

Just discussed this with Mark on Skype and wrote the patch that does as he discussed above.

comment:25 joostdevalk2 years ago

  • Cc joost@… added

comment:26 in reply to: ↑ 23 ; follow-up: duck_2 years ago

Replying to markjaquith:

instead of immediately flushing, it queues a flush for shutdown

That sounds like a good idea. Though there's the minor annoyance of rules potentially lagging by a page load if you're displaying them.

Related: #20171 for a suggestion of "rewrite_init"

comment:27 in reply to: ↑ 26 ; follow-up: joostdevalk2 years ago

Replying to duck_:

Replying to markjaquith:

instead of immediately flushing, it queues a flush for shutdown

That sounds like a good idea. Though there's the minor annoyance of rules potentially lagging by a page load if you're displaying them.

But if you're doing that, aren't you "doing it wrong" already?

comment:28 in reply to: ↑ 27 duck_2 years ago

Replying to joostdevalk:

But if you're doing that, aren't you "doing it wrong" already?

Yes, probably. Good point.

comment:29 scribu2 years ago

Since we don't have verbose page rules anymore, how about generating the rules on the fly? We already do it on each page load for custom post types and taxonomies.

comment:30 joostdevalk2 years ago

Why would we? If the current system, with this patch, works and requires less resources on pageload, I don't really see an advantage?

comment:31 scribu2 years ago

What about all the mental cycles saved for all the devs that don't have to deal with flushing the rewrite rules at the appropriate times?

What about all the time saved in support forums by not having to solve problems related to stale rewrite rules?

comment:32 nacin2 years ago

Perhaps let's figure out how much it costs to generate rewrite rules? Tools like XDebug + KCacheGrind could quickly reveal how much time is spent in rewrite.php.

I could go for generating both the capabilities array and rewrite rules on the fly, if we could make them both performant.

comment:33 DrewAPicture2 years ago

  • Cc xoodrew@… added

comment:34 c3mdigital8 months ago

  • Keywords needs-refresh added

comment:35 tott5 weeks ago

Whenever I ran into this problem so far I'd either enforce rules using pre_option_rewrite_rules or if I like to keep things as dynamic as possible I use some code along the lines of the following code hooked to 'init' to ensure that my rewrite rules will be flushed when needed but not on every page load.

<?php
function maybe_flush_rules() {
        global $wp_rewrite;
        $rewrite_rules = get_option( 'rewrite_rules' );

        foreach( $rewrite_rules as $rule => $rewrite ) {
                $rewrite_rules_array[$rule]['rewrite'] = $rewrite;
        }

        $maybe_missing = $wp_rewrite->rewrite_rules();
        $missing_rules = false;
        $rewrite_rules_array = array_reverse( $rewrite_rules_array, true );

        foreach( $maybe_missing as $rule => $rewrite ) {
                if ( ! array_key_exists( $rule, $rewrite_rules_array ) ) {
                        $missing_rules = true;
                        break;
                }
        }

        if ( true === $missing_rules ) {
                flush_rewrite_rules();
        }
}

This checks only for the existance of the key in the rules but serves me well in most cases and could be extended to also catch updates in rules.

Last edited 5 weeks ago by DrewAPicture (previous) (diff)
Note: See TracTickets for help on using tickets.