PlaceholderGenerator::createPlaceholder() generates invalid markup; causes placeholders to not be replaced if processed by DOMDocument

#2569371: Update Renderer::createPlaceholder() to not use Attribute and SafeMarkup::format() switches away from using Attribute(), which then means we no longer get the attribute-level escaping, which means we end up with arguments="arg1&arg2" instead of arguments="arg1&arg2".

This has not been a problem so far because nothing in core parses such placeholders using PHP's DOMDocument, to then serialize it again. Furthermore, this only happens when there are multiple arguments for a lazy builder. Because $arguments = UrlHelper::buildQuery($placeholder_render_array['#lazy_builder'][1]); already URL-encodes each individual parameter, which leaves only the ampersand as a problem. So other characters that should be escaped (think < and ") are not a problem.

Therefore, HEAD is currently broken right now, because placeholder markup is invalid HTML, which means DOMDocument is unable to parse it. Well, it is able, but automatically fixes it, but then #attached[placeholders] contains the placeholder without the escaped ampersand, whereas the HTML response contains it with the escaped ampersand, hence there is a mismatch, hence the placeholder is not actually replaced.

The solution is simple: ensure PlaceholderGenerator::createPlaceholder() generates valid HTML markup.

When I was trying to reproduce this problem for Berdir, I failed. Because I wanted to not set up my entire environment to reproduce this, and I tried to use a shortcut: I modified entity_embed's filter to use \Drupal\filter\FilterProcessResult::createPlaceholder(). But using that approach, I was not able to reproduce it.

That's because #2569371: Update Renderer::createPlaceholder() to not use Attribute and SafeMarkup::format() only modified PlaceholderGenerator::createPlaceholder(), and not FilterProcessResult::createPlaceholder(). Therefore, FilterProcessResult::createPlaceholder() is still using new Attribute() and therefore still has the automatic attribute escaping. If that were broken too, then FilterTestPlaceholders would have had a failure anyway.

Therefore, the entire solution for this issue is:

1. Add a regression test for PlaceholderGenerator::createPlaceholder() generating invalid HTML markup.
2. Fix PlaceholderGenerator::createPlaceholder() so it generates valid HTML markup.
3. Update FilterProcessResult::createPlaceholder() — it already has the necessary test coverage.

Here's step one: a regression test that should fail.

Here's step two: the fix.

And here's step 3: updating FilterProcessResult::createPlaceholder() to be in sync.

+++ b/core/modules/filter/src/FilterProcessResult.php
@@ -138,12 +138,14 @@ public function setProcessedText($processed_text) {
     $attributes['token'] = hash('sha1', serialize([$callback, $args]));
...
+    $token = hash('crc32b', serialize([$callback, $args]));
+    $placeholder_markup = '<drupal-filter-placeholder callback="' . Html::escape($callback) . '" arguments="' . Html::escape($arguments) . '" token="' . Html::escape($token) . '"></drupal-filter-placeholder>';

Maybe its just me that this is super confusing, having two times the same hashing with a different algorithm, maybe we could document what is going on?

#9: because I'm stupid. I forgot to delete some code, lol. Good catch.

+++ b/core/tests/Drupal/Tests/Core/Render/PlaceholderGeneratorTest.php
@@ -0,0 +1,59 @@
+ * @group Render
+ */
+class PlaceholderGeneratorTest extends RendererTestBase {
+

The only thing which is a bit confusing is that this doesn't test the actual output of the code, should we check that as well? It is just checking whether the placeholder is valid, but I guess we have other places which at least explicitly check that?

#13: \Drupal\Tests\Core\Render\RendererPlaceholdersTest tests it implicitly. The thing is that the exact placeholder doesn't matter and shouldn't matter; it's not an API; it's merely a placeholder that must be reliable. It's the reliability that was broken and that we fix and test here.

If people feel strongly we should explicitly test the generated markup, happy to add test coverage for that.

I was just wondering ...

Discussed with other committers, and agreed this is a sensible fix in RC.

Committed/pushed to 8.0.x, thanks!

Yay, thanks! :)