WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#18040 closed defect (bug) (fixed)

Turning on permalinks, CPT rewrites are ignored

Reported by: andy Owned by:
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.2
Component: Posts, Post Types Keywords: has-patch commit
Focuses: Cc:

Description

I'm working on rewrite issues in a plugin that registers custom post types. Everything works perfectly if rewrite rules already exist. However, when switching away from the default permalinks (?p=123), no CPT rewrite rules are added. I have to trigger another flush (e.g. by reloading the Permalink Settings page) before the CPT rules appear and my plugin's URLs work.

This bug is due to this check. When switching from no permalink_structure to some permalink_structure, the empty permalink_structure option prevents the rewrite rules and permastructs being added, even though they should be allowed in this case. The result is that the rewrite_rules are generated without any CPT rules.

I suppose the option check is purely for performance reasons. The simplest fix would be to omit the option check which would cause register_post_type to take a little bit longer than necessary on sites without a permalink structure. If this is unacceptable, the fix can ignore the option when is_admin() (or some other test).

Attachments (1)

18040.diff (532 bytes) - added by andy 7 years ago.
add is_admin() check

Download all attachments as: .zip

Change History (14)

@andy
7 years ago

add is_admin() check

#1 @scribu
7 years ago

  • Keywords has-patch added
  • Milestone changed from 3.2.1 to Awaiting Review

Sounds like a good idea to me, but I don't think it warrants being put into the 3.2.1 milestone, since it's been this way since CPT rewrites were first introduced, in 3.0: [12923].

#2 follow-up: @pavelevap
7 years ago

  • Cc pavelevap@… added

#3 in reply to: ↑ 2 @andy
7 years ago

In defense of the 3.2.1 designation, this is a backwards compatible bug fix in the existing API rather than new API functionality.

The damage this bug does is fairly subtle because the correct behavior eventually occurs. The registered rules are obeyed on the second flush after permalink_structure is populated. Flushes are triggered by many common actions: loading the Permalink Settings page, activate a plugin, save a plugin option, do an automatic upgrade, etc.

It is incorrect for the first flush to produce a different set of rules than the second flush. The patch I proposed is an internal change that fixes incorrect behavior.

Replying to pavelevap:

Related?

http://core.trac.wordpress.org/ticket/16736

Yes, good find.

#4 @nacin
7 years ago

  • Milestone changed from Awaiting Review to 3.2.1

Patch is quite smart.

Moving back to 3.2.1. Doubt it'll get in, but wouldn't object for 3.2.x if ryan likes it.

#5 follow-up: @nacin
7 years ago

Let me muse for a moment.

If the rules are flushed when we're in the admin, then permalink_structure would become populated with basic rules for post types (and, I guess, nothing else).

In a few areas of core, we do check '' == get_option('permalink_structure') to decide whether permalinks are in use, rather than checking $wp_rewrite. I imagine many plugins including some of ours might be the same.

Does this patch risk these rules ending up in the DB even with default permalinks on?

#6 in reply to: ↑ 5 @andy
7 years ago

Replying to nacin:

If the rules are flushed when we're in the admin, then permalink_structure would become populated with basic rules for post types (and, I guess, nothing else).

It is already true that rules are flushed when you're in (a subset of) the admin because GET /wp-admin/options-permalink.php flushes them. However, this has no effect on, but rather is predicated on, permalink_structure. Flushing populates the array option rewrite_rules if there is a permalink_structure, while the string option permalink_structure is what you save on the options-permalink page.

In a few areas of core, we do check '' == get_option('permalink_structure') to decide whether permalinks are in use, rather than checking $wp_rewrite. I imagine many plugins including some of ours might be the same.

The permalink_structure option check is still valid; $wp_rewrite wouldn't be able to provide a better check in the context of this patch because it doesn't know that the option is going to change in a few milliseconds. The problem arose between the time when you clicked Save and the time when WordPress registered the intention of that click (to enable permalinks). During init, the plugin called a registration function that sees permalinks are disabled and infers that there will be no need to add anything to the $wp_rewrite object properties. That inference is the error behind the error.

Does this patch risk these rules ending up in the DB even with default permalinks on?

If anything, this patch will cause erroneous rules to be removed sooner than they otherwise would.

#7 @nacin
7 years ago

  • Keywords commit added; dev-feedback removed

Looks like I mixed permalink_structure up with rewrite_rules.

Thanks for the further justification. +1 from me.

#8 @azaozz
7 years ago

  • Milestone changed from 3.2.1 to 3.2.2

This came in a bit too late for inclusion in 3.2.1, moving to 3.2.2

#9 @ryan
7 years ago

In [18443]:

Fix CPT rewrite generation when turning on permalinks. Props andy. see #18040 for trunk

#10 @andy
7 years ago

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

#11 @nacin
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I imagine Ryan left this open for 3.2.2, but wanted it up soak in trunk first.

#12 @nacin
7 years ago

  • Keywords fixed-major added

#13 @nacin
7 years ago

  • Keywords fixed-major removed
  • Milestone changed from 3.2.2 to 3.3
  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.