New safe action to add rewrite rules on

[simonwheatley]

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

[simonwheatley]

Adds a new action, run just before flushing rewrite rules

[nacin]

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.

[scribu]

Hooking into 'get_option_rewrite_rules' is an ugly hack.

+1 on 'pre_flush_rewrite_rules' hook.

[scribu]

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().

[scribu]

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

[scribu]

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

[scribu]

18450.2.diff​ does the same for register_taxonomy().

[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, 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.

[dd32]

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..)

[scribu]

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?

[scribu]

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

[simonwheatley]

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.

[simonwheatley]

Refactored to use the flush on activation method

[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.

[simonwheatley]

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. :)

[GaryJ]

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?

[scribu]

Good idea: check did_action( 'init' )

[scribu]

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

[dd32]

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.

[scribu]

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

[dd32]

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 :)

[scribu]

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

Gotcha. :)

See doing_it_wrong.18450.diff​

[ejdanderson]

+1

[markjaquith]

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?

[joostdevalk]

Patch that hooks on shutdown and blocks frontend flushes

[joostdevalk]

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

[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.

Related: #20171 for a suggestion of "rewrite_init"

[joostdevalk]

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?

[duck_]

Replying to joostdevalk:

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

Yes, probably. Good point.

[scribu]

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.

[joostdevalk]

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

[scribu]

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?

[nacin]

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.

[tott]

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.

[SergeyBiryukov]

Related: #30501