Check for empty slug input in register_taxonomy

[cliffseal]

If you give an empty string for the slug part of the rewrite array in a register_taxonomy call, like:

'rewrite' => array(
     'slug' => ''
)

...you get some wacky permalink issues, even when you flush properly. In my case, I was seeing top-level pages (only) 404 while everything else worked. Removing this admittedly poor part of the code fixed it, but this should be checked for. The register_post_type function ensures this isn't empty, and this function should as well.

[cliffseal]

Check for empty string in slug

[SergeyBiryukov]

This would be consistent with [17298].

[SergeyBiryukov]

Looks like emptytax.diff​ is missing sanitize_title_with_dashes(). See 23668.patch​.

register_post_type(), on the other hand, uses sanitize_key():
​http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/post.php#L1234

[cliffseal]

Replying to SergeyBiryukov:

Looks like emptytax.diff​ is missing sanitize_title_with_dashes(). See 23668.patch​.

I'm not sure I understand. It runs sanitize_title_with_dashes() in the next line in the wp_parse_args() call.

[ericlewis]

Replying to cliffseal:

Replying to SergeyBiryukov:

Looks like emptytax.diff​ is missing sanitize_title_with_dashes(). See 23668.patch​.

I'm not sure I understand. It runs sanitize_title_with_dashes() in the next line in the wp_parse_args() call.

wp_parse_args favors the values in the first argument - so in emptytax.diff​ the sanitize_title_with_dashes would have no affect, since the array value of the first argument would be favored, which would be the (potentially) unsanitized $taxonomy string.

[cliffseal]

Replying to ericlewis:

wp_parse_args favors the values in the first argument - so in emptytax.diff​ the sanitize_title_with_dashes would have no affect, since the array value of the first argument would be favored, which would be the (potentially) unsanitized $taxonomy string.

Interesting. Thanks, @ericlewis!

[nacin]

Seems like a good fix, but low priority. Punting to 3.7.

[aaronholbrook]

Tested against trunk rev 24833, and it works great - good work Cliff + Eric!

[wonderboymusic]

Fixed in [25130]

[SergeyBiryukov]

I don't see any functionality changes in [25130].

[wonderboymusic]

Line 350

[SergeyBiryukov]

Line 350 is the same as line 351 in the earlier revision, it's just whitespace fixes.

[SergeyBiryukov]

Refreshed the patch.

[wonderboymusic]

All of the green in the blame tricked my eyes - commit your change if you want.

[wonderboymusic]

In 25335:

Don't call sanitize_title_with_dashes( $taxonomy ) in register_taxonomy() unless $args['rewrite']['slug'] is empty.

Props SergeyBiryukov.
Fixes #23668.

[nacin]

I don't think [25335] needs a revert, but the use of wp_parse_args() there means a query string is valid, so it needs an adjustment. I am fine with actually converting this to array_merge()* and removing the parse_str, but either way, we should try to avoid what we're doing.

*Keeping in mind that the arguments would also need to be flipped.

[jond3r]

[25335] causes a Warning and Notice all over front-end and back-end (using PHP 5.3.8):

Warning: Cannot use a scalar value as an array in \WordpressTrunk\wp-includes\taxonomy.php on line 356
Notice: Undefined index: slug in \WordpressTrunk\wp-includes\taxonomy.php on line 369

[DrewAPicture]

Seems like 23668.3.patch​ should fix the warning introduced in [25335], unless I'm missing something.

We should actually check if $args['rewrite']['slug'] is set and empty since the wp_parse_args() call will only assign $args['rewrite'] a value of true if unspecified.

[DrewAPicture]

isset

[wonderboymusic]

In 25351:

Avoid a notice by casting $args['rewrite'] to array() before adding a slug property and running array_merge().

Fixes #23668.

[johnbillion]

Why are we referring to 'post type key' and 'taxonomy key' in the docblocks instead of 'post type name' and 'taxonomy name'? (Tthe latter was changed in r25130.)

[SergeyBiryukov]

Replying to johnbillion:

Why are we referring to 'post type key' and 'taxonomy key' in the docblocks instead of 'post type name' and 'taxonomy name'? (Tthe latter was changed in r25130.)

"Post type key" was originally introduced in [20734]. #25150 would be a better ticket for this question.

[dd32]

[25351]

I do have to mention, that if someone had their register_taxonomy call with

array(
 'rewrite' => 'with_front=0&slug=something', // or slug=something, etc.
 ...
)

That code will now be broken, which was what nacin was alluding to in comment:18, and jond3r in comment:19 appears to be using.

An alternative to this would've been either

if ( is_array( $args['rewrite'] ) && empty( $args['rewrite']['slug'] ) )
   unset( $args['rewrite']['slug'] );

or

$default_slug = sanitize_title_with_dashes( $taxonomy );
wp_parse_args(.. $default_slug..)
if ( empty( $args['rewrite']['slug'] ) )
   $args['rewrite']['slug'] = $default_slug;

[SergeyBiryukov]

Technically, $args['rewrite'] should only be an array or a boolean value, but a string indeed would have worked too, and I guess we shouldn't break that.

23668.4.patch​ restores wp_parse_args() and moves it above the slug assignment to make sure $args['rewrite'] is an array before treating it as such.

[nacin]

Replying to SergeyBiryukov:

Technically, $args['rewrite'] should only be an array or a boolean value, but a string indeed would have worked too, and I guess we shouldn't break that.

For the record, I don't mind breaking that.

[nacin]

Replying to nacin:

Replying to SergeyBiryukov:

Technically, $args['rewrite'] should only be an array or a boolean value, but a string indeed would have worked too, and I guess we shouldn't break that.

For the record, I don't mind breaking that.

And it wouldn't work anyway. This would only work if someone passed $args as an array but $args['rewrite'] as a query string. I believe the technical term for that is "stupid".

[wonderboymusic]

In 25483:

Use wp_parse_args() again for $args['rewrite'] in register_taxonomy().

Props SergeyBiryukov.
Fixes #23668.