Opened 12 years ago
Last modified 3 weeks ago
#25021 new defect (bug)
Improve sanitize_title_with_dashes % removal
Reported by: |
|
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
.
Attachments (1)
Change History (15)
#3
follow-up:
↓ 5
@
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)?
#4
@
12 years ago
Related: ticket:14773:14
#5
in reply to:
↑ 3
@
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.
This ticket was mentioned in PR #9011 on WordPress/wordpress-develop by @SirLouen.
4 weeks ago
#7
Trac ticket: https://core.trac.wordpress.org/ticket/25021
#8
@
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
- ✅ 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
@
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:
↓ 14
@
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
@
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).
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.