Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#47594 accepted enhancement

Add nbsp symbol codes to convert throught sanitize_title

Reported by: hokku's profile hokku Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release 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 5 years ago.
Patch
47594.1.diff (693 bytes) - added by audrasjb 3 years ago.
Patch refreshed and simplified as per today's bug scrub
47594-before-after-patch.png (191.1 KB) - added by hellofromTonya 3 years ago.
Works: shows before and after for space character %e2%80%af

Download all attachments as: .zip

Change History (33)

#1 @hokku
5 years ago

  • Type changed from defect (bug) to enhancement

#2 @hokku
5 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
5 years ago

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

#4 @adhitya03
5 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 5 years ago by adhitya03 (previous) (diff)

@adhitya03
5 years ago

Patch

#5 @adhitya03
5 years ago

  • Keywords has-patch added

#6 @azaozz
5 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
5 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
4 years ago

  • Milestone changed from Future Release to 5.5

#11 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.5 to 5.6

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


4 years ago

#14 @hellofromTonya
4 years ago

Per Sergey during today's scrub:

Will try to address before beta, or punt otherwise.

#15 @hellofromTonya
4 years ago

  • Keywords needs-refresh added

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

#16 @SergeyBiryukov
4 years 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.


3 years ago

#18 @noisysocks
3 years ago

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

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


3 years ago

@audrasjb
3 years ago

Patch refreshed and simplified as per today's bug scrub

#20 @audrasjb
3 years ago

  • Keywords 2nd-opinion needs-refresh removed

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

#21 @hellofromTonya
3 years 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.


3 years ago
#22

  • 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

hellofromtonya commented on PR #915:


3 years ago
#23

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

@hellofromTonya
3 years ago

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

#24 @hellofromTonya
3 years 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
3 years 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
3 years 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.


3 years ago

#28 @SergeyBiryukov
3 years ago

  • Milestone changed from 5.8 to 5.9

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


3 years ago

#30 @audrasjb
3 years ago

  • Milestone changed from 5.9 to Future Release

As per today's bug scrub, let's move this ticket to Future release pending a new refreshed patch to address the previous comments.

Note: See TracTickets for help on using tickets.