Support a placeholder strategy denylist for #lazy_builders

StandardPerformanceTest is missing an assertion on ScriptBytes for authenticated users. Added that and ran the tests only job:

https://git.drupalcode.org/project/drupal/-/jobs/4436719

1) Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testStandardPerformance
Asserting ScriptBytes 122689 is greater or equal to 6000 and is smaller or equal to 7000
Failed asserting that 122689 ( is equal to 6000 or is greater than 6000 ) and ( is equal to 7000 or is less than 7000 ).

115kb of JavaScript reduction for authenticated users out of the box.

@fabianx suggested '#placeholder_strategy => ['single_flush'] as a generic way to avoid bigpipe for a placeholder. That would allow other blocks/elements to opt out of bigpipe too which could be a good thing.

I couldn't see a straightforward way to allow a specific placeholder to target a specific placeholder_strategy - because placeholder strategies don't have IDs or anything, they're just tagged services, and they could theoretically be removed by contrib etc.

However, if we add support in big_pipe specifically for 'no_big_pipe', and allow #placeholder_strategy to be used in general, then that gives a way for different placeholder implementations to support different kinds of placeholder_strategy hints. This might be overly flexible but also it was very, very easy to implement and doesn't require any API changes.

This is working.

The main remaining test failures are because big pipe's test coverage heavily relies on messages being placeholdered by big pipe, which they no longer are even on cold caches.

The messages placeholder is handy for testing because it's easy to put arbitrary content in it, behaves differently depending session etc. so I can see why it was used in the first place. Easiest way to address this was to add an element info alter to big_pipe_test module to remove the #placeholder_strategy. Only alternative I could think of that wouldn't result in less test coverage was subclassing or copying the element over, or coming up with something superficially different but similar.

Probably still a couple of test failures left but getting close.

Will need a change record to document the new capability so tagging for that, would be nice to get a review before that though.

Needed to split the messages alter out to a dedicated module because big_pipe_test adds an http header for every placeholder and that is fun (as in 502 error fun) in BigPipeRegressionTest which creates 2000 placeholders.

Tests should be green now.

I think this will allow us to do #3508508: Allow big pipe to run for session-less users so went ahead and opened it.

@supriyagupta that's not necessary because the big pipe js is already added conditionally (and when it's added it's needed). This issue is just reducing some of the conditions under which it gets added.

I think I was able to address all the remarks.

Added a change record.

In the process of writing it, I realised we should specify the CachedStrategy for the messages element even though it won't be used (the block is cached with its own keys, the placeholder element is not) - because we only want to prevent big pipe from kicking in, not anything else.

Doesn't a denylist make more sense here?

Yeah it does, I started out with that with the no_big_pipe magic string but went to an allowlist when we added the classnames.

I didn't fully reallise that ClassName::class works whether or not the class is defined or not, so that should actually be fine to just reverse the logic and still use classnames.

Rebased and switched to a denylist.

Rebased.

Think I got to all the feedback, agreed on the more general assertion message.

I think because of a small mistake.

I thought I'd missed the exclamation mark when you pointed this out, but it was actually fine, the problem was in the test.

The unit test sets up a 'placeholder removing strategy' that returns an empty array. It is not possible to do this, you can ignore placeholders, you can't remove them. So I just removed this from the data provider, it's testing something that's not supported and pretty sure the unit test would have choked on the new assertion if it was added in isolation without the rest of the patch.

If CachedStrategy does not find any hits, it will also return an empty array. Isn't that the same as what the test strategy did?
The only difference I see is that the test did not have SingleFlushStrategy as a fallback, which ends up returning all placeholders anyway.

Yes exactly - SingleFlushStrategy always replaces the remaining placeholders, which is why we can do the assert() to make sure there's none left to replace.

The unit test has no fallback, just a no-op strategy, and in that situation the 'removed' placeholder is not actually removed, but just ignored and not replaced, which we are now validating should never be allowed to happen.

So does that not mean that the check has to be !empty() to figure out that something was, in fact, not replaced?

No because the failure condition for an assert() call is when it returns FALSE.

https://www.php.net/manual/en/function.assert.php

Assertions should be used as a debugging feature only. One use case for them is to act as sanity-checks for preconditions that should always be true...

Added to $pre_bubbling_elements - it won't make any difference in practice due to CachedPlaceholderStrategy but Renderer itself doesn't know this, so we should do it. Also applied the other suggestions and added the extra comment to the test pre_render.

Since these are all minor, going to self-RTBC for the changes.