Ampersand's in the Navigation block are converted to u0026

[kylekelly]

Description

Adding ampersands '&' to the label field of a link (page & custom) in the Navigation block converts it to u0026, which displays on the frontend and backend once refreshed.

Step-by-step reproduction instructions

1. Go to the latest 6.5-beta3 (or nightly) on your test site, or in playground: https://playground.wordpress.net/?php=8.0&wp=beta&storage=none
2. Edit site, and add a link to the navigation block.
3. Include an ampersand in the label of a link in the block, an example being 'News & Events'
4. View the frontend of the site and see it converted to 'u0026'

Screenshots, screen recording, code snippet

Environment info

• WordPress 6.5 beta3 & Nightly
• Tested and seen with both Gutenberg not installed, and installed and activated.
• Tested in both latest Chrome & Firefox

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[annezazu]

Adding to 6.5 but let's test with 6.4.3 to ensure this is related to the 6.5 release. If no one beats me to it, I'll do that tomorrow (@fabiankaegy in case you can!)

[fabiankaegy]

I just tested this and was able to verify that it is not happening in 6.4.3 but is happening on 6.5 Beta 3.

[fabiankaegy]

@getdave Tagging you here as one of the maintainers of the Navigation Block

[getdave]

I can replicate. Likely to due to how label is escaped in the core/navigation-link block.

I'm going to recommend that whatever fix we implement we add test coverage for in a follow up as similar issues have occurred a few times now.

[getdave]

Testing I found the following is stored in the database

<!-- wp:navigation-link {"label":"News u0026amp; Events","type":"page","id":2,"url":"http://localhost:8888/?page_id=2","kind":"post-type"} /-->

Notice the News u0026amp; Events.

I tested with removing the escaping in

gutenberg/packages/block-library/src/navigation-link/update-attributes.js

Lines 75 to 77 in 8c5d40b

	const label = useNewLabel
	? escapeHTML( newLabel )
	: originalLabel || escapeHTML( newUrlWithoutHttp );

...and it does change the conversion to entities.

[fabiankaegy]

Could there be any other unrelated change in WordPress core that would have this effect on escaping / uescaping characters in the DB?

[getdave]

Highly likely. The code I highlighted above is non-standard and only exists for historical reasons. So it's possible that there's a change in Core which has caused this hack to come undone.

Could someone test in 6.4 or even previous Betas?

[getdave]

Some more context here #41063 (comment)

[getdave]

I tested with WP 6.4 and the same issue is not present.

@dmsnell I know you have specific expertise in these types of problems and may well also be aware of changes in Core that could have caused this. Can you help shed any light on this?

Also pinging @swissspidy & @dream-encode as Core Tech Leads who may be aware.

[swissspidy]

Nothing off the top of my head. Sounds odd to store the ampersand like this in the database. Probably easiest to just do some step-by-step debugging on both 6.4 and 6.5 to find out where the difference occurs.

[ockham]

👋 @scruffian brought this issue to my attention since it seems to have been caused by #59021. I'll look into it now.

cc/ @tjcafferkey

[ockham]

This seems like it could be related: https://core.trac.wordpress.org/ticket/60671

[t-hamano]

If I'm testing correctly, this problem first appeared on the backport for Beta2. Commit hash c84cddb on GitHub.

WordPress/wordpress-develop@c84cddb

This problem does not seem to occur if you check out the previous commit (18b21c3).

[t-hamano]

wp_update_post() is being executed here, is it because it is escaped in case of an array?

https://developer.wordpress.org/reference/functions/wp_update_post/#parameters

Post data. Arrays are expected to be escaped, objects are not.

[ockham]

wp_update_post() is being executed here, is it because it is escaped in case of an array?

https://developer.wordpress.org/reference/functions/wp_update_post/#parameters

Post data. Arrays are expected to be escaped, objects are not.

Ohh, that looks like it could be the reason!

[ockham]

Oh, I'm not sure I read that correctly. Doesn't that mean that wp_update_post expects that the $postarr argument has already been escaped if it's an array, and escapes it itself if it's an object? If so, then I think that's what we want, in order to avoid escaping twice, no? 🤔

[getdave]

$postarr
Post data. Arrays are expected to be escaped, objects are not. See wp_insert_post() for accepted arguments.

https://developer.wordpress.org/reference/functions/wp_update_post/

[t-hamano]

Hmm, maybe wp_update_post itself is not the fundamental problem...

[ockham]

Yeah. My current theory is that this is happening somewhere along the way between parse_blocks and traverse_and_serialize_block.

The fact that News & Events becomes News u0026amp; Evens indicates that there are some transforms that work okay, probably News & Events -> News & Events -> News \\0026amp; Events ✅

But somehow, the double backslash is then stripped: News \\0026amp; Events -> News 0026amp; Events ❌

I haven't yet been able to find out where.

(Edit: Backslash, not slash.)

[ockham]

Hmm, looks like we have test coverage for parsing/serialization of backslashes in attribute values though: https://github.com/WordPress/wordpress-develop/blob/0f45a3c9fc10d1abf68ec9e201e6113815cdbb34/tests/phpunit/tests/blocks/serialize.php#L44-L45

(Edit: Backslashes, not slashes.)

[ockham]

I'll try creating a minimal unit test for this so we can debug more easily.

[ockham]

I'll try creating a minimal unit test for this so we can debug more easily.

#59561

I'll have to wrap up for today; I'll continue working on it tomorrow. In the meantime, feel free to continue investigating and push commits to that PR 😊

[t-hamano]

This problem appears to be not limited to the Navigation Link block. At least it also happens with Search and Home Link blocks. However, it doesn't seem to occur on the Button block.

95cce3c9abab7bd69d5766f732b93ebe.mp4

[tjcafferkey]

This problem appears to be not limited to the Navigation Link block. At least it also happens with Search and Home Link blocks. However, it doesn't seem to occur on the Button block.

Yeah, I suspect this is because button text is not stored in the blocks attribute values where as the other text/labels in the blocks you have mentioned are. It's the process of serializing/parsing these attributes which is where things go a bit wild.

[dmsnell]

@dmsnell I know you have specific expertise in these types of problems and may well also be aware of changes in Core that could have caused this. Can you help shed any light on this?

Thanks for asking @getdave. I had no ideas other than I wish we were done rewriting Core with the HTML API. I'm hoping to eliminate these kinds of issues, which creep up to frequently. It took me a lot of time and frustration to figure out a way to write <![CDATA[test]]> in the Dev Notes for the HTML API, and the solution involves some carefully-placed zero-width spaces.

The problem is almost certainly inside of wp_kses_normalize_entities() given the chain through u0026amp;, but there are many roads to and through there.