Block status code visibility condition should use a status code cache context

The actual condition plugin lives in system module.

When we get a 404 or 403, HttpExceptionSubscriberBase sets the exception on the request.

By the time we reach the rendering of the 404 or 403 page, this is available, so the cache context will work.

It won't do anything if someone has a response subscriber that adds a 418 or 201 status code or weird things like that, those would all count as 200.

We could rename it to ExceptionStatusCodeCacheContext and then when there is no exception status code, return '0' instead of '200'. This would match how the visibility condition works a lot closer. Did that in a new commit.

There's no bubbling of cache contexts to the page cache and won't be (we might bubble max age at some point, there's an issue) because it only ever caches by URL.

Just in case anyone lands here due to large cache tables, while this should fix the dynamic page cache, it's very easy to end up with a large internal page cache database too.

One reason is #3011426: Page cache size is unlimited when arbitrary query parameters are requested for query strings.

For 404s/403s the page cache supports much shorter TTLs, which means 404/403 pages can be expired on cron or set to not cache at all. This was added in #2699613: Set a shorter TTL for 404 responses in page_cache module.

Added a unit test and a change record.

Neither standard nor umami use the response for the breadcrumbs block, so we can't add this into an existing performance test without some extra work. However I think we could have a follow-up to use the visibility, and we should then be able to see a performance improvement for 404 pages vs both HEAD and the MR here.

I could potentially add the performance test here without taking advantage of the changes, which we could then show the difference in the follow-up issue - will see what people think first though.

Rebased.

OK new issue with test is here: #3589683: Add a performance test for 404 pages.

Assuming the pipeline comes back green (passes locally but you never know), I'll cherry-pick the two commits over here so we then get a diff on the performance test (hopefully).

I had to allow the breadcrumb block to show on the front page, since excluding on the front page is also per-path (although it could potentially use the is_front cache context - could be another follow-up from this issue to try to make that work). But the breadcrumbs don't actually render on the front page so no visual difference.

So the test works but because Umami has other blocks with page visibility conditions, dynamic page cache still ends up with the url.path cache context, which means this issue doesn't affect the test. If Umami was using layout builder for the front page and maybe node pages it would probably help.

Might try the same thing with standard performance test instead..

OK I merged in the tests from the other issue for now.

The commit that shows the difference vs. the behaviour in HEAD is https://git.drupalcode.org/project/drupal/-/merge_requests/11672/diffs?c...

Given I had to jump through a couple of hoops to get performance tests to show the problem, I think we could consider doing the following:

1. Merge this without performance test coverage.

2. Merge the test coverage issue, but without the tweaks to Olivero block config that demonstrate the problem here. They'd still pick up something else.

3. Try to fix #3589822: Try to reduce cache context granularity from path conditions which should then show a difference in Umami.

OK I removed the test changes from the MR again. Given this was RTBC before apart from the one applied suggestion, going ahead and moving back to RTBC.

Note that there is test coverage here already, it's not just a full integration test. The performance tests do show that just because it works in isolation it doesn't necessarily improve things on real sites though - although it should if they don't use path visibility otherwise.

I missed that comment in the various test excitement, added the instanceof check.

One question I have is what to do if the there is an exception but it fails the instanceof check - what should we return then? 500?

It already falls back to '0' if there's no exception status code, which I think is all we can do here. If we're rendering blocks when an exception is passed in here, and that exception isn't an http exception... I'm just not sure how that could happen at all tbh.