Allow the messages block to skip placeholdering

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 📌 Allow big pipe to run for session-less users Active so went ahead and opened it.

@catch I think we can use page_attachments hook to remove the js file based on condition and based on user role. So that it will work proper according to our requirement.

function mymodule_page_attachments(array &$attachments) {

  if (\Drupal::currentUser()->isAuthenticated()) {
 
    if (isset($attachments['#attached']['library']['bigpipe'])) {
     
      unset($attachments['#attached']['library']['bigpipe']);
    }
}
}

@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 like the approach and that we no longer use magic strings. Left some remarks, but nothing too drastic.

I think I was able to address all the remarks.

Looks good to me, tests go green too. Just some small doc mistakes that can be fixed upon commit for all I care. Holding off on RTBC as the issue is still marked as "Needs change record".

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.

Needs to merge in origin/11.x again for the performance tests and needs to update docs. The only thing that bothers me about both the MR and CR is that it's now painfully obvious that an allowlist can be limiting. We only want to turn off BigPipe and have to specify all other strategies. If contrib introduces another strategy, it won't apply to the messages block.

Doesn't a denylist make more sense here? You opt into all strategies out of the box, so the only possible change you'd want to make here is opt out of some of them, meaning a denylist makes more sense.

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.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

Dug up the ::class docs for people stumbling upon this issue: https://www.php.net/manual/en/language.oop5.basic.php#language.oop5.basi...

Found some. minor things but looks good to me otherwise

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

ChainedPlaceholderStrategyTest is choking on the new assert, I think because of a small mistake.

Americanizzze the code comments.

😂

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 since it was added.

It is not possible to do this, you can ignore placeholders, you can't remove them.

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? BigPipeStrategy can also return an empty array. The only difference I see is that the test did not have SingleFlushStrategy as a fallback, which ends up returning all placeholders anyway.

Might be my sickness talking but if I read ChainedPlaceholderStrategy::processPlaceholders() correctly, we reduce the set of $placeholders whenever a strategy returned some of them back. At the end of the loop we want $placeholders to be empty, no? So does that not mean that the check has to be !empty() to figure out that something was, in fact, not replaced?

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...

Oh wow, for a second there my brain processed the assert as an if-statement. Left one more suggestion, but that can be fixed upon commit.