Layout Builder overrides section storage sets local tasks block cache max-age to 0 on content entity pages without overrides enabled

Chipping-in/following this because this seems to be the reason why every single page on our Drupal 9 sites is uncacheable by our reverse proxy, platform.sh or Drupal.

If true, this is causing a huge additional load on our servers because every content page on our sites is uncacheable.

This would appear to be because https://www.drupal.org/project/cache_control_override is doing its job, but in doing so is silently killing page caching.

Disabling cache_control_override has fixed this problem for me.

/Al

Thanks to some discussion with @neclimdul in Slack, I think I can move this forward a bit.

Thanks to Tim I have the steps to reproduce.
1) Enable layout builder
2) Add overiddes to a content type(maybe optional)
3) View a node on a second type that doesn't use layout builder.

Since there are no errors and its all hidden in caches, setting the breakpoint at the exception in ParamConverterManager::convert is generally the easiest way I've found to observe the problem though a test would probably want to be testing the max-age from local task rendering or something..

Attached might actually be a fix. It basically uses the passed $path in the trait method to decide if the upcasting parameter is needed. By setting the value to false, the converter will return false in the upcasting applies method during the route building and then it won't run during normal conversion.

I'm not sure about the AccessCheck change. Maybe it shouldn't be attached either but the change made things work and the signature change should be fine since its @internal.

bah, cross posting. sorry Tim.

Whoops, stupid mistakes.

The change to when tempstore is run is breaking a lot of stuff, so that's not right. Unassigning for now, but the tests are there.
Pushed up what was in the patch to a new MR.

So sent the morning mulling over this a bit instead of just hammering on code. I think there are basically two problems here which we've kinda danced around in the slack equate to two possible solutions.

Problems:
1) Layout builder is using parameter conversion as a hidden route enhancement to inject the section storage parameters into access control and controllers. This doesn't exactly align with the intention of parameter conversion which means it kinda runs into a problem with the interface.
2) Parameter conversion treats a null value as a failed conversion which means its impossible to have optional parameters like this. A first it seems kinda crazy to exclude null but it actually has a reason baked into the assumption of the API which we see if we dig a bit deeper into the history. Initially conversion explicitly threw a NotFound 404 exception[1]. Later, a more specific exception was bolted on[2] to allow conversion in other context like access control checks during task generation. In hindsight this API feels clunky and limiting but the intention was to specifically support converting the value for the slug /node/{node}/view into a node object and if that fails it makes sense that the page doesn't exist and the controller etc shouldn't have to do a lot. It makes the design for a converter easier as well because it can just return the result of the object loading and offload all the handling of what "failing" to load the object means.

Solutions:

1. Refactor LayoutTempstoreParamConverter.
   1. Refactor _some_ of the logic out of the converter and only have the converter doing converting and maybe do some sort of other enhancement or other logic.
   2. Provide some sort of null/static/invalid section storage that could be returned from the conversion that is not null and can full fill the needs of things like the access control API even if its never injected into a controller.
2. 2) Change the upcasting/parameter conversion API to allow null values.

Now personally I kinda lean toward 1.1. There isn't a lot of logic so it should be fairly easy to recreate in places. The problem I think is going to be that other enhancement methods are going to be less "integrated" into route magic and its going to be less simple that it should be.

1.2 mostly is ugly because the class would need to implement SectionStorageInterface which is gigantic and I'm not sure how we'd make "null" versions of 90% of the methods which makes it less generic and more a special hack only for this problem.

2 seems like maybe the "right" option but there is no guarantee we even _can_ change it because the assumptions kinda work for the intended purpose. If we do change it I doubt we'd see something usable for several versions which leaves a ton of nodes pages with uncacheable tasks. Personally I don't like this because tasks can be pretty heavy right now with #2950869: Entity queries querying the latest revision very slow with lots of revisions being triggered on route checks when content moderation is installed making node pages with the two modules installed very slow on large sites.

[1] #1798214: Upcast request arguments/attributes to full objects
[2]#2185831: Split up ParamConverterManager and stop throwing NotFoundHttpException

Note also #3032433: Allow section storages to be loaded via routing without loading from the tempstore which wants to change a lot of the same code.

I believe that MR !1075 should be suitable to address the concerns in this issue in a way that doesn't create a dependency on the work being done in #3032433: Allow section storages to be loaded via routing without loading from the tempstore. Setting to Needs Review (pending test results).

I retested the patch from #10 before rereading more and realizing that didn't work at the time, let alone now.
I ran out of time to really dig in on this, I'm not sure why the two MRs are so far apart. Either way, they should likely take advantage of the new layout_builder_section_storage flag added in #3032433: Allow section storages to be loaded via routing without loading from the tempstore

Rebased on 9.4.x/HEAD.

I spent several hours reviewing your feedback and the changes in #3032433: Allow section storages to be loaded via routing without loading from the tempstore; I'm sticking with my original plan of attack with MR !1075 for the following reasons:

1. Unless I'm missing something obvious, the work that was committed from #3032433: Allow section storages to be loaded via routing without loading from the tempstore doesn't seem to afford us anything that is material to resolving this issue. The parameter converter still produces NULL regardless of those changes, which is the crux of this issue. The parameter converter works for both layout_builder_section_storage and layout_builder_tempstore (which is already present on the routes in question). The route enhancer only serves to replace the section storage with the copy stored in the temp store, and it won't do so if the parameter converter produces NULL.
2. It doesn't seem wise to add yet another parameter converter since we'll still end up with the exact same problem. (The access check and route enhancement doesn't happen until parameter conversion is finalized.)
3. If we opt to stop at ::loadEmpty() in the parameter converter and move the context derivation to a route enhancer, it makes us more vulnerable to future bugs that are potentially worse (e.g., passing an "empty" section storage to the controller which results in a white page -- or all pending changes being discarded), and could drastically complicate the API (since we would be missing some of the parameters that are passed to ::deriveContextsFromRoute()). Not to mention, I don't believe there's any sort of weighting mechanism for route enhancers (though I could be wrong since documentation on this subsystem is sparse).

My alternative approach is very simple:

1. Don't gate ::extractEntityFromRoute() on the entity being fieldable, or having the override layout storage field. It would seem more appropriate to add a separate method for these checks if necessary (though I don't believe it to be necessary).
2. Fix the layout_builder_is_active cache context, as it currently doesn't work and will stand in the way of achieving the goals in the issue summary. See #24 instead.
3. Only permit access to the override layout section storage if the default section storage specifies that overrides are enabled. This will appropriately prevent access to the override layout editing route since LayoutBuilderAccessCheck already defers to section storage-specific access checks.

Moving to Needs Review for @tim.plunkett's feedback.

Per #23, I removed the changes to the cache context. I agree that it's out of scope, and I think its removal shouldn't negatively affect the prospects of resolving this issue since all tests should still pass.

Not moving to Postponed on #3190541: LayoutBuilderIsActiveCacheContext not providing correct context value. unless maintainers see it as a hard blocker.

Looks like the cache context interferes with something else, so marking this as Postponed on #3190541: LayoutBuilderIsActiveCacheContext not providing correct context value. anyway.

:(

Child issue was committed, so unpostponing.

Hello, Just testing the patch for MR 1075, and I immediately get the following error:

"RuntimeException: Callable "Drupal\layout_builder\Access\LayoutBuilderAccessCheck::access" requires a value for the "$section_storage" argument. in Drupal\Component\Utility\ArgumentsResolver->handleUnresolvedArgument() (line 143 of core/lib/Drupal/Component/Utility/ArgumentsResolver.php)."

I also tested the patch in 791, and it still seems to miss the page cache.

This still needs subsystem review

But the MR should be updated for 10.1 now and left a small change on the current MR.

Thanks.

I feel this should be a higher priority. The bug completely breaks page cache on every non-layout builder page. I had to rip layout builder out of a site because of this.

Per #32

The bug completely breaks page cache on every non-layout builder page

This is slightly wrong, it clobbers it on every page with Layout builder enabled without overrides enabled.

This took me an extremely long time to track down. Kudos to @larowlan for knowing the issue off hand.

Rebased to run the test-only feature which failed as expected

There were 2 errors:
1) Drupal\Tests\layout_builder\Functional\LayoutBuilderLocalTaskTest::testLocalTaskLayoutBuilderInstalledCacheability
Behat\Mink\Exception\ExpectationException: Current response header "X-Drupal-Cache-Max-Age" is "0 (Uncacheable)", but "-1 (Permanent)" expected.
/builds/issue/drupal-3190542/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3190542/vendor/behat/mink/src/WebAssert.php:161
/builds/issue/drupal-3190542/core/modules/layout_builder/tests/src/Functional/LayoutBuilderLocalTaskTest.php:62
/builds/issue/drupal-3190542/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
2) Drupal\Tests\layout_builder\Functional\LayoutBuilderLocalTaskTest::testLocalTaskMultipleContentTypesCacheability
Behat\Mink\Exception\ExpectationException: Current response header "X-Drupal-Cache-Max-Age" is "0 (Uncacheable)", but "-1 (Permanent)" expected.
/builds/issue/drupal-3190542/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3190542/vendor/behat/mink/src/WebAssert.php:161
/builds/issue/drupal-3190542/core/modules/layout_builder/tests/src/Functional/LayoutBuilderLocalTaskTest.php:103
/builds/issue/drupal-3190542/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!

Issue summary still seems incomplete though. For sections that don't apply putting NA usually helps. Put TBD in sections

THanks! Will still need submaintainer review but think it's ready for that now.

Will still need submaintainer review

Then this isn't RTBC? 🤔😅

Subsystem maintainer @tim.plunkett worked on this until #10. In #19 he wrote that that approach cannot work.

In #22, @clayfreeman proposed a wholly new approach, resulting in the currently RTBC MR.

Therefore this should still get the sign-off from subsystem maintainer Tim Plunkett before this is RTBC.

Thanks @clayfreeman and @godotislate for keeping this one going.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!