Improve sanitize_title_with_dashes % removal

[duck_]

The current method of % removal involves placeholders to prevent stomping on escape sequences:

// Preserve escaped octets.
$title = preg_replace('|%([a-fA-F0-9][a-fA-F0-9])|', '---$1---', $title);
// Remove percent signs that are not part of an octet.
$title = str_replace('%', '', $title);
// Restore octets.
$title = preg_replace('|---([a-fA-F0-9][a-fA-F0-9])---|', '%$1', $title);

This leads to sanitize_title_with_dashes('---aa---') producing %aa instead of just aa.

[2189]

[duck_]

25021.negative-lookahead.diff​ replaces the placeholder system with a single regex with a negative lookahead. This fixes the issue, but is very slightly slower in my very limited testing.

[SergeyBiryukov]

Related: #3329

[nacin]

I'm going to guess "very slightly slower" is insignificant enough to warrant this change? We've always suggested that sanitize_title() and friends are not light and thus should be avoided where possible anyway.

How might this affect sanitize_title_for_query() (as in, working with older slugs that don't sanitize the same way)?

[SergeyBiryukov]

Related: ticket:14773:14

[nacin]

Replying to nacin:

How might this affect sanitize_title_for_query() (as in, working with older slugs that don't sanitize the same way)?

As long as this doesn't break anything, I'm game.

Trac ticket: https://core.trac.wordpress.org/ticket/25021

[SirLouen]

Test Report

Description

✅ This report validates that the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/9011.diff

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 7.4.33
• Server: nginx/1.27.5
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 7.4.33)
• Browser: Chrome 137.0.0.0
• OS: Windows 10/11
• Theme: Twenty Twenty-Five 1.2
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0

Testing instructions

• Add the code proposed by OP anywhere that can be run
• For example as the one provided in artifacts, using [sanitize_shortcode] shortcode somewhere in a post
• 🐞 Text displayed: %aa

Expected results

• Text displayed aa

Actual Results

1. ✅ Issue resolved with patch.

Additional Notes

• This is old, but is still valid.
• I've patch refreshed, added some unit tests and posted this PR just to run the whole testing suite and see if it’s passing.
• I know that these kinds of regexes are not tasteful, but I see that this can move on.

Supplemental Artifacts

Code used for testing:

function sanitize_title_with_dashes_test() {
	$title = '---aa---';
	$sanitized_title = sanitize_title_with_dashes( $title );
	return $sanitized_title;
}
add_shortcode( 'sanitize_shortcode', 'sanitize_title_with_dashes_test' );

[lakshyajeet]

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/9011

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.28
• Server: nginx/1.27.5
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
• Browser: Chrome 136.0.0.0
• OS: macOS
• Theme: Twenty Nineteen 3.1
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0

Testing instructions

• Apply the patch.
• Use Shortcode to display the text (sample code below).
• Add shortcode in a post: [sanitize_title_shortcode]
• Text displayed %abc

Expected Results

• The text abc should be displayed.

Actual Results

• The text abc is displayed.
• ✅ Issue resolved with patch.

Supplemental Artifacts

Code used:

function sanitize_title_with_dashes_shortcode_test() {
        $title = '---abc---';
        $sanitized_title = sanitize_title_with_dashes( $title );
        return $sanitized_title;
}
add_shortcode( 'sanitize_title_shortcode', 'sanitize_title_with_dashes_shortcode_test' );

[SirLouen]

6.9.0 Milestone proposed during today's <bug-scrub>
​More information

cc @azaozz

[azaozz]

Replying to SirLouen:

6.9.0 Milestone proposed during today's <bug-scrub>

Thanks @SirLouen and @lakshyajeet for testing this patch. However seems testing has to be done for the rest of the cases. I think Nacin nailed it 11 years ago:

As long as this doesn't break anything, I'm game.

Seems it wasn't clear back then if anything else may break, so this never made it. Imho more testing will be great here (including PHPUnit tests).