Remove html encoded entities from post slug when cleaning #62549

tharsheblows · 2024-06-13T13:40:08Z

What?

This removes the html encoded entity strings for & and < from the slug in cleanForSlug(). These are only two entities which seem to cause an issue.

Why?

When you type Ampersand & And as the title of an Post and save the Post as a draft, you will see that the URL becomes /ampersand-amp-and/. If you publish the Post, the URL becomes /ampersand-and/.

Step-by-step reproduction instructions

Go to Settings > Permalinks and set Post name as a permalink structure.
Create a Post and type Ampersand & And as the Post title.
Save the Post as a Draft.
In the editor sidebar click on the URL to view the generated URL. It is this: /ampersand-amp-and/
Publish the Post.
Chceck URL of the Post. It is this: /ampersand-and/

How?

Replaces & and < with an empty string in cleanForSlug(). Also adds tests.

Testing Instructions

create a post with the title And & &&& & &&& <1 039898':!#*() &&& <>.“‘”’" hey (note the string & and string <)
check the post slug shown in the Link field in the sidebar, it should be and-amp-lt1-039898-hey
publish the post and look at the post name bit in the slug on the front end, it should be the same and-amp-lt1-039898-hey

github-actions · 2024-06-13T13:40:24Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tharsheblows <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

tharsheblows · 2024-06-14T13:54:47Z

Adding a comment to update the props as I've linked my account now (thanks to whoever set up that bot with instructions!)

tyxla

Thanks for the contribution, @tharsheblows!

I have left a few initial thoughts I had while reviewing the code. Let me know what you think!

tyxla · 2024-06-17T16:19:18Z

packages/url/src/clean-for-slug.js

@@ -24,7 +24,7 @@ export function cleanForSlug( string ) {
 		return '';
 	}
 	return (
-		removeAccents( string )
+		removeAccents( string.replace( /&amp;/g, '' ).replace( /&lt;/g, '' ) )


What about any other commonly used HTML entities, like >,  , ', " and others (not an exhaustive list by any means)?

@tyxla Yes, I agree with you, it should handle all entities. I wavered between keeping it limited to what you would get from the WP REST API response for the title (only those entities need to be handled) and all entities, wasn't quite sure which approach to take!

I see. In light of the recent conversation to stay compatible with the sanitize_title_with_dashes filter, let's introduce a utility function compatible with sanitize_title filtered by sanitize_title_with_dashes which converts some entities (spaces and dashes) to hyphens, and obliterates the rest of the HTML entities.

tyxla · 2024-06-17T16:20:06Z

packages/url/src/test/index.js

+	it( 'Should remove html entities for ampersands', () => {
+		expect(
+			cleanForSlug( 'the long cat &amp; a dog &amp;&amp; fish' )
+		).toBe( 'the-long-cat-a-dog-fish' );
+		expect( cleanForSlug( 'the long cat &amp;amp; dog' ) ).toBe(
+			'the-long-cat-amp-dog'
+		);
+	} );
+
+	it( 'Should remove html entities for less thans', () => {


Likely not applicable after addressing my other feedback, but this could be a good case for it.each as that could simplify the test code and remove redundancy.

Nice! I'll add in after the discussion below, thank you!

tyxla · 2024-06-17T16:21:03Z

packages/url/src/clean-for-slug.js

@@ -24,7 +24,7 @@ export function cleanForSlug( string ) {
 		return '';
 	}
 	return (
-		removeAccents( string )
+		removeAccents( string.replace( /&amp;/g, '' ).replace( /&lt;/g, '' ) )


The cleanForSlug function is intended to be a mirror of sanitize_title(), which doesn't account for HTML entities. This is also confirmed by the cleanForSlug documentation, which specifically claims it doesn't touch any HTML entities. With that in mind, perhaps there is a better place to address this change?

Yes, I wondered why it wasn't a mirror – there is a core default filter added to sanitize_title() which runs the $title through sanitize_title_with_dashes() and the expectation when doing PHP development is that sanitize_title( 'the dog & the cat' ) will return the-dog-the-cat.

It's that core filter which handles the heavy lifting of sanitization and removal of the html entities among other things, this is the reason the string returned by cleanForSlug() doesn't match the actual slug returned by sanitize_title().

People might be using this and changing the returned value could horribly mess their urls up – what about keeping back compat and introducing a new function sanitizeForSlug() which mirrors sanitize_title() with the default filter added (ie add in all the sanitization done by sanitize_title_with_dashes()) and then using sanitizeForSlug() when displaying the slug.

Does that approach sound ok?

there is a core default filter added to sanitize_title() which runs the $title through sanitize_title_with_dashes() and the expectation when doing PHP development is that sanitize_title( 'the dog & the cat' ) will return the-dog-the-cat.

This makes sense, thanks for pointing it out 👍

People might be using this and changing the returned value could horribly mess their urls up – what about keeping back compat and introducing a new function sanitizeForSlug() which mirrors sanitize_title() with the default filter added (ie add in all the sanitization done by sanitize_title_with_dashes()) and then using sanitizeForSlug() when displaying the slug.

Does that approach sound ok?

A separate utility function sounds good to me 👍 We'll definitely want to keep cleanForSlug as it is, since it intentionally doesn't handle entities.

remove html encoded entities from post slug when cleaning

d98de2d

akasunil added [Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended labels Jun 14, 2024

Mamaduka requested a review from tyxla June 17, 2024 07:43

tyxla reviewed Jun 17, 2024

View reviewed changes

tharsheblows requested a review from tyxla June 18, 2024 08:30

Mamaduka mentioned this pull request Jan 22, 2025

Remove HTML entities from post slug during cleaning #68813

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove html encoded entities from post slug when cleaning #62549

Remove html encoded entities from post slug when cleaning #62549

tharsheblows commented Jun 13, 2024 •

edited by Mamaduka

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading

tharsheblows commented Jun 14, 2024

tyxla left a comment

tyxla Jun 17, 2024

tharsheblows Jun 18, 2024

tyxla Jun 18, 2024

tyxla Jun 17, 2024

tharsheblows Jun 18, 2024

tyxla Jun 17, 2024

tharsheblows Jun 18, 2024

tyxla Jun 18, 2024

Remove html encoded entities from post slug when cleaning #62549

Are you sure you want to change the base?

Remove html encoded entities from post slug when cleaning #62549

Conversation

tharsheblows commented Jun 13, 2024 • edited by Mamaduka Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Jun 13, 2024 • edited Loading

tharsheblows commented Jun 14, 2024

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tharsheblows commented Jun 13, 2024 •

edited by Mamaduka

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading