Make WordPress Core

Opened 12 years ago

Last modified 3 weeks ago

#25021 new defect (bug)

Improve sanitize_title_with_dashes % removal

Reported by: duck_'s profile duck_ Owned by:
Milestone: Priority: normal
Severity: normal Version: 1.5
Component: Formatting Keywords: has-patch has-unit-tests has-test-info dev-feedback
Focuses: Cc:

Description

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]

Attachments (1)

25021.negative-lookahead.diff (799 bytes) - added by duck_ 12 years ago.

Download all attachments as: .zip

Change History (15)

#1 @duck_
12 years ago

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.

#2 @SergeyBiryukov
12 years ago

  • Keywords has-patch added

Related: #3329

#3 follow-up: @nacin
12 years ago

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

#5 in reply to: ↑ 3 @nacin
12 years ago

  • Milestone changed from Awaiting Review to Future Release

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.

#6 @chriscct7
10 years ago

  • Keywords needs-testing added

#8 @SirLouen
4 weeks ago

  • Keywords has-unit-tests has-test-info added; needs-testing removed

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' );

This ticket was mentioned in Slack in #core-test by sirlouen. View the logs.


4 weeks ago

#10 @lakshyajeet
4 weeks ago

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' );

This ticket was mentioned in Slack in #core by sirlouen. View the logs.


3 weeks ago

#12 follow-up: @SirLouen
3 weeks ago

  • Keywords dev-feedback added

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

cc @azaozz

This ticket was mentioned in Slack in #core by sirlouen. View the logs.


3 weeks ago

#14 in reply to: ↑ 12 @azaozz
3 weeks ago

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

Last edited 3 weeks ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.