WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 7 months ago

#23668 closed defect (bug) (fixed)

Check for empty slug input in register_taxonomy

Reported by: cliffseal Owned by: 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 14 months ago.
Check for empty string in slug
23668.patch (800 bytes) - added by SergeyBiryukov 13 months ago.
23668.2.patch (648 bytes) - added by SergeyBiryukov 8 months ago.
23668.3.patch (725 bytes) - added by DrewAPicture 7 months ago.
isset
23668.diff (970 bytes) - added by wonderboymusic 7 months ago.
23668.4.patch (927 bytes) - added by SergeyBiryukov 7 months ago.

Download all attachments as: .zip

Change History (35)

cliffseal14 months ago

Check for empty string in slug

comment:1 SergeyBiryukov14 months ago

  • Milestone changed from Awaiting Review to 3.6

This would be consistent with [17298].

comment:2 cliffseal13 months ago

  • Keywords needs-testing added

SergeyBiryukov13 months ago

comment:3 follow-up: SergeyBiryukov13 months 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

comment:4 in reply to: ↑ 3 ; follow-up: cliffseal13 months 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 13 months ago by cliffseal (previous) (diff)

comment:5 in reply to: ↑ 4 ; follow-up: ericlewis13 months 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.

comment:6 in reply to: ↑ 5 cliffseal13 months 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!

Last edited 13 months ago by cliffseal (previous) (diff)

comment:7 nacin10 months ago

  • Milestone changed from 3.6 to Future Release

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

comment:8 aaronholbrook9 months ago

  • Keywords dev-feedback needs-testing removed

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

Version 0, edited 9 months ago by aaronholbrook (next)

comment:9 aaronholbrook9 months ago

  • Keywords commit added

comment:10 DrewAPicture9 months ago

  • Milestone changed from Future Release to 3.7

comment:11 wonderboymusic8 months ago

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

Fixed in [25130]

comment:12 SergeyBiryukov8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:14 SergeyBiryukov8 months ago

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

SergeyBiryukov8 months ago

comment:15 SergeyBiryukov8 months ago

Refreshed the patch.

comment:16 wonderboymusic8 months ago

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

comment:17 wonderboymusic8 months 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.

comment:18 nacin8 months 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 8 months ago by nacin (previous) (diff)

comment:19 jond3r8 months 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

comment:20 DrewAPicture7 months 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.

DrewAPicture7 months ago

isset

wonderboymusic7 months ago

comment:21 wonderboymusic7 months 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.

comment:22 follow-up: johnbillion7 months 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.)

comment:23 in reply to: ↑ 22 SergeyBiryukov7 months 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.

comment:24 dd327 months 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;

SergeyBiryukov7 months ago

comment:25 follow-up: SergeyBiryukov7 months 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.

comment:26 SergeyBiryukov7 months ago

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

comment:27 in reply to: ↑ 25 ; follow-up: nacin7 months 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.

comment:28 in reply to: ↑ 27 nacin7 months 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".

comment:29 wonderboymusic7 months 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.