WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#35236 closed defect (bug) (fixed)

remove_rewrite_tag()

Reported by: swissspidy Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Rewrite Rules Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

As suggested in #14761 and #35227, a remove_rewrite_tag() helper function would be useful to remove registered rewrite tags.

Figured a separate ticket for this function is better to work on it.

Attachments (3)

35236.diff (3.5 KB) - added by swissspidy 5 years ago.
35236.2.diff (5.4 KB) - added by swissspidy 5 years ago.
35236.3.diff (6.5 KB) - added by swissspidy 5 years ago.

Download all attachments as: .zip

Change History (11)

@swissspidy
5 years ago

#1 @SergeyBiryukov
5 years ago

  • Component changed from General to Rewrite Rules

#2 follow-up: @boonebgorges
5 years ago

In your tests, you're using array_pop() to test the presence of a value in $wp_rewrite properties. This is dangerous: array_pop() modifies the original array, which might break later tests. Can you use assertContains() and assertNotContains() instead? If it's important to check that it's the last item in the array, maybe you could use end()?

@swissspidy
5 years ago

#3 in reply to: ↑ 2 ; follow-up: @swissspidy
5 years ago

Replying to boonebgorges:

In your tests, you're using array_pop() to test the presence of a value in $wp_rewrite properties. This is dangerous: array_pop() modifies the original array, which might break later tests. Can you use assertContains() and assertNotContains() instead? If it's important to check that it's the last item in the array, maybe you could use end()?

Ah, thanks for pointing this out! For WP_Rewrite::$rewritereplace it is indeed important.

For example, when you register custom post types, '(.+)' gets added to WP_Rewrite::$rewritereplace multiple times. We only want to remove this once from the array, not all occurrences.

The positions of tags, regexes and query strings inside the arrays need to be in sync because we remove them that way.

Rewrite tags are unique because you can't add one twice (it gets updated instead).

I now switched to using end() and assertContains() where needed and added some more tests.

#4 in reply to: ↑ 3 @boonebgorges
5 years ago

Replying to swissspidy:

Ah, thanks for pointing this out! For WP_Rewrite::$rewritereplace it is indeed important.

For example, when you register custom post types, '(.+)' gets added to WP_Rewrite::$rewritereplace multiple times. We only want to remove this once from the array, not all occurrences.

The positions of tags, regexes and query strings inside the arrays need to be in sync because we remove them that way.

Rewrite tags are unique because you can't add one twice (it gets updated instead).

I now switched to using end() and assertContains() where needed and added some more tests.

Oh, interesting. Another strategy would be to stash a copy of the array before running remove_rewrite_tag(), and then compare it to the array after running remove_rewrite_tag(). But what you have here looks like it should work :)

@swissspidy
5 years ago

#5 @swissspidy
5 years ago

  • Keywords commit added

35236.3.diff adds setUp() / tearDown() methods to properly reset the arrays. Also, using array comparison as suggested by @boonebgorges where it makes sense.

#6 @boonebgorges
5 years ago

Looks good to me.

#7 @swissspidy
5 years ago

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

In 36217:

Rewrite: Add a remove_rewrite_tag() helper function.

It can be used to properly remove registered rewrite tags. Adds unit tests.

Fixes #35236.

#8 @swissspidy
5 years ago

In 36218:

Tests: Fix unit tests after [36217].

See #35236.

Note: See TracTickets for help on using tickets.