Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#23668 closed defect (bug) (fixed)

Check for empty slug input in register_taxonomy

Reported by: cliffseal's profile cliffseal Owned by: wonderboymusic's profile wonderboymusic
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.5.1
Component: Taxonomy Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (6)

emptytax.diff (570 bytes) - added by cliffseal 11 years ago.
Check for empty string in slug
23668.patch (800 bytes) - added by SergeyBiryukov 11 years ago.
23668.2.patch (648 bytes) - added by SergeyBiryukov 11 years ago.
23668.3.patch (725 bytes) - added by DrewAPicture 11 years ago.
isset
23668.diff (970 bytes) - added by wonderboymusic 11 years ago.
23668.4.patch (927 bytes) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (35)

@cliffseal
11 years ago

Check for empty string in slug

#1 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.6

This would be consistent with [17298].

#2 @cliffseal
11 years ago

  • Keywords needs-testing added

#3 follow-up: @SergeyBiryukov
11 years ago

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

#4 in reply to: ↑ 3 ; follow-up: @cliffseal
11 years ago

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.

Last edited 11 years ago by cliffseal (previous) (diff)

#5 in reply to: ↑ 4 ; follow-up: @ericlewis
11 years ago

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.

#6 in reply to: ↑ 5 @cliffseal
11 years ago

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!

Version 0, edited 11 years ago by cliffseal (next)

#7 @nacin
11 years ago

  • Milestone changed from 3.6 to Future Release

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

#8 @aaronholbrook
11 years ago

  • Keywords dev-feedback needs-testing removed

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

Last edited 11 years ago by aaronholbrook (previous) (diff)

#9 @aaronholbrook
11 years ago

  • Keywords commit added

#10 @DrewAPicture
11 years ago

  • Milestone changed from Future Release to 3.7

#11 @wonderboymusic
11 years ago

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

Fixed in [25130]

#12 @SergeyBiryukov
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#14 @SergeyBiryukov
11 years ago

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

#15 @SergeyBiryukov
11 years ago

Refreshed the patch.

#16 @wonderboymusic
11 years ago

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

#17 @wonderboymusic
11 years ago

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

In 25335:

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

Props SergeyBiryukov.
Fixes #23668.

#18 @nacin
11 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Last edited 11 years ago by nacin (previous) (diff)

#19 @jond3r
11 years ago

[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

#20 @DrewAPicture
11 years ago

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
11 years ago

isset

#21 @wonderboymusic
11 years ago

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

In 25351:

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

Fixes #23668.

#22 follow-up: @johnbillion
11 years ago

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

#23 in reply to: ↑ 22 @SergeyBiryukov
11 years ago

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.

#24 @dd32
11 years ago

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

#25 follow-up: @SergeyBiryukov
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#26 @SergeyBiryukov
11 years ago

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

#27 in reply to: ↑ 25 ; follow-up: @nacin
11 years ago

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.

#28 in reply to: ↑ 27 @nacin
11 years ago

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

#29 @wonderboymusic
11 years ago

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

In 25483:

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

Props SergeyBiryukov.
Fixes #23668.

Note: See TracTickets for help on using tickets.