WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 5 months ago

#47594 accepted enhancement

Add nbsp symbol codes to convert throught sanitize_title

Reported by: hokku Owned by: SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: minor Version: 5.3
Component: Formatting Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Default filters needs to be supplemented nbsp codes such as

[ '%e2%80%af', '%e2%80%87', '%e2%81%a0' ]

and

[ ' ', ' ', ' ', ' ', '⁠', '⁠' ]

Attachments (3)

47594.diff (1.7 KB) - added by adhitya03 2 years ago.
Patch
47594.1.diff (693 bytes) - added by audrasjb 9 months ago.
Patch refreshed and simplified as per today's bug scrub
47594-before-after-patch.png (191.1 KB) - added by hellofromTonya 9 months ago.
Works: shows before and after for space character %e2%80%af

Download all attachments as: .zip

Change History (31)

#1 @hokku
2 years ago

  • Type changed from defect (bug) to enhancement

#2 @hokku
2 years ago

  • Summary changed from Add nbsp symbol codes to convert to sanitize_title to Add nbsp symbol codes to convert throught sanitize_title

#3 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#4 @adhitya03
2 years ago

Hi,I wanna make a patch for this but I need clarify something so I don't get it wrong.
In this case, do mean "Default filters needs to be supplemented nbsp" is to sanitize all kind of Unicode characters of SPACE to be its HTML Entity "&nbsp" ?

Last edited 2 years ago by adhitya03 (previous) (diff)

@adhitya03
2 years ago

Patch

#5 @adhitya03
2 years ago

  • Keywords has-patch added

#6 @azaozz
2 years ago

  • Keywords reporter-feedback 2nd-opinion added

I need to clarify something so I don't get it wrong.

I'm missing some context here too :)

@SergeyBiryukov are you looking at this for 5.3? I'm not sure I understand what's being fixed and why.

#7 @davidbaumwald
2 years ago

  • Milestone changed from 5.3 to Future Release

With version 5.3 Beta 1 releasing today, I'm moving this to Future Release.

#10 @SergeyBiryukov
20 months ago

  • Milestone changed from Future Release to 5.5

#11 @SergeyBiryukov
16 months ago

  • Milestone changed from 5.5 to 5.6

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


12 months ago

#14 @hellofromTonya
12 months ago

Per Sergey during today's scrub:

Will try to address before beta, or punt otherwise.

#15 @hellofromTonya
12 months ago

  • Keywords needs-refresh added

Marking for a refresh as the patch no longer applies cleanly to trunk.

#16 @SergeyBiryukov
12 months ago

  • Milestone changed from 5.6 to 5.7

Didn't get a chance to write the tests for this in time for 5.6 Beta 1, moving to the next release for now.

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


9 months ago

#18 @noisysocks
9 months ago

  • Keywords needs-unit-tests added; reporter-feedback removed

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


9 months ago

@audrasjb
9 months ago

Patch refreshed and simplified as per today's bug scrub

#20 @audrasjb
9 months ago

  • Keywords 2nd-opinion needs-refresh removed

@hellofromTonya @SergeyBiryukov I refreshed and simplified the previous patch as per today's bug scrub.

#21 @hellofromTonya
9 months ago

Thank you @audrasjb! I'll get the unit tests written for it. And it should be ready for commit consideration.

This ticket was mentioned in PR #915 on WordPress/wordpress-develop by hellofromtonya.


9 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

  • Applies patch 47594.1.diff. Props @Adhitya03 and @audrasjb.
  • Adds unit tests
  • Also renames test file to comply with handbook test standard and adds @covers

#23 @prbot
9 months ago

hellofromtonya commented on PR #915:

Before the patch, all tests fail except for  :

.FFFFFFFFF                                                        10 / 10 (100%)

Time: 6.18 seconds, Memory: 116.00 MB

There were 9 failures:

1) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #1 ('%e2%80%af')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handle%e2%80%afspace%e2%80%afcharacter'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

2) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #2 ('%e2%80%87')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handle%e2%80%87space%e2%80%87character'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

3) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #3 ('%e2%81%a0')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handle%e2%81%a0space%e2%81%a0character'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

4) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #4 (' ')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handlespacecharacter'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

5) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #5 (' ')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handlespacecharacter'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

6) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #6 (' ')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handlespacecharacter'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

7) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #7 (' ')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handlespacecharacter'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

8) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #8 ('⁠')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handlespacecharacter'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26

9) Tests_Formatting_SanitizeTitle::test_unicode_space_characters with data set #9 ('⁠')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'handle-space-character'
+'handlespacecharacter'

/var/www/tests/phpunit/tests/formatting/sanitizeTitle.php:26
Last edited 9 months ago by hellofromTonya (previous) (diff)

@hellofromTonya
9 months ago

Works: shows before and after for space character %e2%80%af

#24 @hellofromTonya
9 months ago

  • Keywords commit added

Applied patch. Works as expected in the unit tests and in manual live testing. Marking this ticket as a commit candidate. @SergeyBiryukov it's ready for your review.

#25 @ocean90
9 months ago

  • Keywords commit removed

Why are the changes part of sanitize_title() and not sanitize_title_with_dashes() which already handles some of the cases?

The list also seems to be incomplete. I think if we're going to update the list we should cover all space characters. Here's a filter I have recently used (which is also incomplete):

<?php
/**
 * Extends sanitize_title_with_dashes() to replace more space related characters.
 *
 * @param string $title     Sanitized title.
 * @param string $raw_title The title prior to sanitization.
 * @param string $context   The context for which the title is being sanitized.
 * @return string The sanitized string.
 */
function trac_replace_spaces_in_titles( string $title, string $raw_title, string $context ): string {
        if ( 'save' !== $context ) {
                return $title;
        }

        $title = str_replace(
                [
                        '%e2%80%89', // THIN SPACE.
                        '&thinsp;', // THIN SPACE.
                        '&#8201;', // THIN SPACE.
                        '%e2%80%8a', // HAIR SPACE.
                        '%e2%80%91', // NON-BREAKING HYPHEN.
                        '%e2%80%92', // FIGURE DASH.
                        '%e2%80%af', // NARROW NO-BREAK SPACE.
                        '&#8239;', // NARROW NO-BREAK SPACE.
                ],
                '-',
                $title
        );

        $title = preg_replace( '|-+|', '-', $title );
        $title = trim( $title, '-' );

        return $title;
}
add_filter( 'sanitize_title', 'trac_replace_spaces_in_titles', 10, 3 );

#26 @hellofromTonya
9 months ago

  • Milestone changed from 5.7 to 5.8

5.7 Beta 1 is happening in shortly. Ran out of time to get this ticket into the release. Punting to 5.8.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

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


5 months ago

#28 @SergeyBiryukov
5 months ago

  • Milestone changed from 5.8 to 5.9
Note: See TracTickets for help on using tickets.