BigPipeStrategy test coverage + remaining @todos

1. +++ b/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php
   @@ -0,0 +1,163 @@
   +      '#markup' => '<div data-big-pipe-selector="callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&amp;args[0]&amp;token=a8c34b5e"></div>',
   

   This placeholder makes a ton of sense, and it's legible.

2. +++ b/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php
   @@ -0,0 +1,163 @@
   +      '#markup' => '<div data-big-pipe-selector-nojs="drupal-render-placeholder-callbackdrupalcorerenderelementstatusmessagesrendermessages-arguments0-tokena8c34b5edrupal-render-placeholder"></div>',
   

   This placeholder on the other hand makes little sense, and is not really legible.

   Let's fix that: let's make no-JS BigPipe placeholders also make sense.

#4 let me down a bit of a rabbit hole of inconsistencies.

I refactored the code generating #4.1 into a protected helper method so I can use it also for the no-JS case. No problem.

Then I noticed it was generating a "selector". But it's not really generating a selector, it's generating an identifier. As is evident from the fact that we send these generated selectors to the client side using drupalSettings.bigPipePlaceholders, so that the JS has a whitelist to verify against:

   // Ignore any placeholders that are not in the known placeholder list.
   // This is used to avoid someone trying to XSS the site via the
   // placeholdering mechanism.;
   if (typeof drupalSettings.bigPipePlaceholders[placeholderName] !== 'undefined') {

But note that this JS doesn't call it "selectors" anymore! This is very inconsistent and confusing.

If all this were called "placeholder IDs" instead of "selectors", none of this confusion would arise. So, let's do that renaming first.

And here we go, much saner placeholders + placeholder IDs for BigPipe no-JS placeholders, this addresses #4.2.

   * @todo Figure out how to simplify this. Perhaps no new placeholder is in fact necessary?
   * @todo Related, perhaps distinguish between "HTML" and "non-HTML (attr value)" use cases? Because right now, this *breaks* HTML and therefore breaks response filters: this indiscriminately uses a <div> as a placeholder, which is invalid inside a HTML attribute, and thus breaks DOM parsing.
   */
  protected static function createBigPipeNoJsPlaceholder($original_placeholder, array $placeholder_render_array) {

Those two @todos are technically out of scope here.

But…

+++ b/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php
@@ -0,0 +1,163 @@
+      '#markup' => '<div data-big-pipe-nojs-placeholder-id="callback=form_builder%3ArenderPlaceholderFormAction&&token=e8cb6dae"></div>',
...
+      '#markup' => '<div data-big-pipe-nojs-placeholder-id="callback=route_processor_csrf%3ArenderPlaceholderCsrfToken&args[0]=admin/config/user-interface/shortcut/manage/default/add-link-inline&token=77d1dce6"></div>',

These three expectations cement the current implementation in the test expectations: they cement the expectation that these placeholders which live in attribute values can in fact be HTML, which breaks response filters as the @todo indicates.

Conclusion: I think it makes sense to fix those todos here, to ensure the test expectations actually make sense.

#2674126: BigPipeResponseAttachmentsProcessor test coverage landed in the mean-time, so re-testing #6.

The refactoring I did in #5 resulted in a new helper method (generateBigPipePlaceholderId()), which also made me realize that one test case is still missing: one for a placeholder that does not use #lazy_builder. See #2469431-173: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.

So, first adding test coverage for that.

In doing so, I also noticed that the number of passes did not reflect the number of individual things being tested. So I changed the assertions around a bit: they're now per-placeholder instead of for all placeholders at once. This also greatly simplifies debugging of this test/understanding what exactly is broken.

IS update for #9.

And finally, this implements #7.

I worked on #9 while working on #12, I didn't split them up correctly. Hence the failure in #9.

And in #12, I apparently introduced a typo at the very last minute.

Sorry for the confusion.

--- /dev/null
+++ b/src/Tests/Render/BigPipeResponseAttachmentsProcessorTest.php

Oops, this leftover file snuck in! It's not being run (so it's a "dead test" effectively), but it's making the patch looking much bigger than it is.

#2670462: Clean up, document & improve big_pipe.js landed, this needed to be rebased.

After a clean rebase, there was one more small change needed, and only that change is visible in this interdiff.

Per #7 and since #12, this fixes all remaining @todos in BigPipeStrategy, updating title accordingly.

I waited 24 hours to allow for reviews to come in. None happened, that's okay. I don't think there was any controversial change here. Just test coverage, clean-up and renaming.

Excellent, thanks!