Block directory: aggressive <script replacement

[Djennez]

Describe the bug
The introduction of the data-handle in #22721 seems to introduce an aggressive search and replace for the script tag. This can break escaped json output by introducing unescaped quotes " to otherwise valid escaped json data. This can cause the "New post" page to show a white screen and errors in the console:

12:47:28.767 SyntaxError: missing } after property list post-new.php:2348:167904note: { opened at line 2348, column 164883post-new.php:2348:164883

The part the line and column are referring to is (in our case):

(truncated)...<script data-handle="wp-api-fetch" type=\"application\/ld+json\" class=\"yoast-schema-graph\">...(truncated)

Though this is not the only instance.

To reproduce
Steps to reproduce the behavior:

1. Install and activate Gutenberg 8.3.0 and Yoast SEO 14.3
2. Activate the Block experiment in Gutenberg -> Experiments
3. Go create a new post

Expected behavior
To be able to use the post editor without errors.

Screenshots

Editor version (please complete the following information):

• WordPress version: 5.4.2
• Gutenberg plugin 8.3.0

Desktop (please complete the following information):

• OS: OSX Catalina 10.15.3
• Browser Firefox DE 77.0b9

Smartphone (please complete the following information):
N/A

[jdevalk]

cc @spacedmonkey :)

[spacedmonkey]

@jdevalk Can you have any idea what you doing on the plugin end that is making this error happen?

[Djennez]

@spacedmonkey from what I could gather:

• We add yoast_head to the REST API. If a post / page gets requested, this field will contain the data that is normally present in our head (meta tags, schema data, et cetera). Our schema is wrapped in script tags <script type="application/ld+json" class="yoast-schema-graph">
• The edit-post page source would contain this script (I truncated createPreloadingMiddleware)

<script>
wp.apiFetch.use( wp.apiFetch.createRootURLMiddleware( "http://acceptance.test/wp-json/" ) );
wp.apiFetch.nonceMiddleware = wp.apiFetch.createNonceMiddleware( "a6392f23cf" );
wp.apiFetch.use( wp.apiFetch.nonceMiddleware );
wp.apiFetch.use( wp.apiFetch.mediaUploadMiddleware );
wp.apiFetch.nonceEndpoint = "http://acceptance.test/wp-admin/admin-ajax.php?action=rest-nonce";
wp.apiFetch.use( wp.apiFetch.createPreloadingMiddleware( {"\/":{"body":{"name":"Yoast acceptance WP site","..............."}}}) );
</script>

• The truncated part seems to contain the complete REST response of this page (for a preload?), including an json escaped version of our yoast_head:

...\n<script data-handle="wp-api-fetch" type=\"application\/ld+json\" class=\"yoast-schema-graph\">{...

But it now contains unescaped quotes because <script was replaced by the code in Gutenberg to now contain <script data-handle="wp-api-fetch".

As a possible fix: would it be sufficient to run this replace only on the first instance of <script that is found? Instead of globally? I believe that would still fulfill the goal of the PR?

[spacedmonkey]

@Djennez The bug is in this line - https://github.com/WordPress/gutenberg/pull/22721/files#diff-ced9b72e7d9418c27bc2fbb5fba83ebaR34

It is a simple fix to replace str_replace with preg_replace. In this regex just needs to check for src attribute.

[spacedmonkey]

Example

// Add 'srcset' and 'sizes' attributes to the image markup.
$image = preg_replace( '/<img ([^>]+?)[\/ ]*>/', '<img $1' . $attr . ' />', $image );

[jdevalk]

@spacedmonkey I think Gutenberg shouldn't be changing the Yoast SEO output at all, to be honest :)

[spacedmonkey]

@spacedmonkey I think Gutenberg shouldn't be changing the Yoast SEO output at all, to be honest :)

No clearly, that is a mistake. But it because Yoast is doing something a little weird here. Do you want to work on a fix @Djennez ?

[jdevalk]

I don't think what we're doing is weird at all. We have output, like any plugin could have, core is changing it :) seems to me the fault is not on our end. @Djennez is a member of our QA team, I honestly think someone with more insight into the code should fix it :)

[Djennez]

I don't think it's our intention to put our data as json on these pages. That seems to be done by core adding preloaded API responses.

@spacedmonkey I'm not comfortable creating a PR for this, because I don't know what the original code's intended purpose is and I don't want to break things 😃

However, I did toy around a bit with possible fixes because I do think I know what needs to happen here.

1. My original thought was to only replace the first occurrence of <script. However, the $tag can have multiple <script elements that need replacing, so that did not work.

2. Next I was thinking of looking ahead in the string to see if there would be a hint of escaped data, like =\". And if so, do not match. However, this would break on additional elements like src= and simply wasn't consistent without building a monster of a regex (I think).

3. The last idea I played with, and which seems to work is to check if the <script element is preceded by a newline or start of a string:

$tag = preg_replace( '/(^|\n)<script/', "$1" . sprintf( '<script data-handle="%s"', esc_attr( $handle ) ), $tag );

Note that I had to carry over the match to the replacement with $1 to keep all newlines on the page. This code does not replace the escaped data that can be in an API response and it seems to do what the original codes intention was. But I think that is best checked by you 😃

I am also not sure how much overhead this regex generates, as there are a lot of <script elements on my pages that need replacing.

[gziolo]

@spacedmonkey, it should be non-issue once it’s integrated into WordPress core as we will be able to update the code that prints script tags to include this data attribute only when there is src set. For the existing filter, would that help to use preg replace that ensures that both src and the handle are set?

[spacedmonkey]

@spacedmonkey, it should be non-issue once it’s integrated into WordPress core as we will be able to update the code that prints script tags to include this data attribute only when there is src set. For the existing filter, would that help to use preg replace that ensures that both src and the handle are set?

We need to change to str_replace with preg_replace. This is a total none issue once we move this to core.

[mcsf]

@spacedmonkey: I was talking to @ellatrix and @tellyworth about what we need in order to remove the experimental status from the Block Directory (#23378). It's clear that this issue — given the popularity of Yoast, but also given the uncertainty on who else is affected — needs a solution if we want more people to use the Gutenberg plugin to test the Block Directory ahead of its inclusion in Core.

Switching to preg_replace and ensuring we at least have a src attribute, as you pointed out, would definitely be an improvement.

That said, before that, I'm interested in knowing why we need #22721 right now. Don't get me wrong: I think #22703 is a worthwhile achievement that I look forward to, but it doesn't seem like that functionality will be ready before WP 5.5 Beta 1, correct?

That's why I'm thinking that reverting #22721 might be the safest and quickest way to unblock @ellatrix and @tellyworth, who I understood to be racing against the clock.

[jdevalk]

The fact that we're racing to get something in 5.5 without giving plugins like our own the ability to understand and adapt to this might give us some reason for pause too.

[mcsf]

I don't think there is any blind commitment to get BD in 5.5. When I mentioned racing against the clock, I had the plugin release cycle in mind. The sooner we can get BD out of experimental, the sooner we can get a sense of how well it works for people. If it doesn't, then it won't be in 5.5.

[gziolo]

To make it clear, this change is necessary to work on performance optimizations for the block editor in general. It isn't specific to Block Directory, but in the long run this is where it would bring the most benefits.

You can learn about work started by @saramarcondes to lazy load TinyMCE in this comment #2768 (comment). If this is something that is causing so much trouble, we should revert it. In the meantime, we can safely apply the same changes in WordPress core in a proper way (without hooks) which should be enough for the sake of development in a branch. If we decide to land optimizations sooner, we will have to use a bit modified hook that works with existing plugins.

[jdevalk]

Thx for the explanations @mcsf and @gziolo!

[dd32]

After looking at this, it didn't seem like using a regular expression was needed, just a more specific match/replace.

In #23407 I've matched the full <script...> tag and ran it earlier to load before anything else that alters the script tag.
This resolves the problem with inline script tags in json, and seems to cover all the cases that would be wanted (which coincidently, is the same as what a preg_replace() suggested above would handle).

[mcsf]

Closed by #23407