Allow section storages to be loaded via routing without loading from the tempstore

This patch makes perfect sense to me.

Discussed with @tim.plunkett. In our API policy, paramconverters are listed as an example of the @internal things that people shouldn't extend (to the point that people want to put final on them). But this issue is definitely adding one paramconverter that extends another, which is either a counterpoint to "all paramconverters are internal" because we want to do something useful here for it, or it means we need to change what this patch does to match the current BC policy and set a good example.

@tim.plunkett is going to change this to use decoration instead. 🎄

Yup, this is a lot better; it's free of questionable patterns and the tests make sense to me. For whatever my rather promiscuous word is worth, I say let's go for it.

So the new approach is splitting up the old LayoutTempstoreParamConverter, renaming it to LayoutSectionStorageParamConverter, and adding LayoutTempstoreRouteEnhancer.

This is internal code, but maybe a CR would be in order?

Needs a reroll as well as a change record.

Only patch reroll

Moving to Needs Review & Triggering bots.

re-roll around #3106666: Remove post updates added prior to 8.8.0

CR posted here. Restoring RTBC.

Committed 6eee1db and pushed to 9.3.x. Thanks!

Hey past me, do you remember why you where interested in this and what you can fix going forward?

@neclimdul

I unironically had the same question. I believe it was due to caching misbehaving for the layout override local task, but I could be misremembering. (Is that even a separate issue? Or is this enough to solve that problem?)

Found it.

This seems to have caused a regression in custom access checks for layout builder routes, like the access checks in the Layout Builder Advanced Permissions module.

The access checks used to receive the SectionStorage from the temp store, since the AccessManager calls $this->paramConverterManager->convert($parameters + $route->getDefaults()) in Drupal\Core\Access\AccessManager::checkNamedRoute. Since the AccessManager does not call route enhancers, the code in LayoutTempstoreRouteEnhancer is never called, which leads to the "wrong" version of the SectionStorage and an OutOfBoundsException when calling $section_storage->getSection($delta) for a newly added section (only available in the tempstore).

I don't understand route enhancers and param converters well enough to decide if this is a bug or not, so I'm not sure we should open a new issue, revert this change, or just accept that access checks for layout builder should fetch the section storage from the temp store themselves using the LayoutTempstoreRepository service?

Created #3253819: In 9.3.x the SectionStorage in the AccessManager is no longer loaded from the temp store as a workaround for the Layout Builder Advanced Permissions module.

Per our conversation in Slack, I believe the original concern of this issue would be better served by moving the enhancement functionality back to the param converter. This way routes can still use the new flag that was introduced (the "public" portion of the API changes) to avoid the tempstore, while existing code shouldn't need to change.

Will need a maintainer to reopen this and set it to Needs Review.

@tim.plunkett isn't that already covered by the combination of \Drupal\Tests\Core\Access\AccessManagerTest and \Drupal\Tests\layout_builder\Unit\LayoutSectionStorageParamConverterTest? The former test class ensures that the access manager uses parameter conversion for route access checks, where the latter ensures that the Layout Builder flags produce the correct value during conversion. If both tests pass, then it would logically follow that the correct behavior is observed in Layout Builder access checks, no?

I'm down to add this additional test case if needed, but I also want to avoid duplicating any test functionality. I'm also not sure how I would test the currently-released 9.3.0 behavior after making the changes contained within my proposed revision if that's what you mean instead.

Adding test-only patch which should fail against 9.3.x/HEAD. Will follow-up by including this in the MR, which should hopefully then pass.

Whoops -- accidentally attached an empty patch.

Ready for review!

The added test LGTM.

This issue was committed in 9.3.x and then re-opened a few months later. Should the work after the commit be in a separate issue? I ask because from working in the Bugsmash Initiative issues that are committed and then re-opened (without a revert of the commit) tend to linger and be difficult to triage. It is also extra work when searching git logs when more that one commit has the same issue number and different changes.

The work on the MR, this will need to be on 10.0.x.

This is tagged as 'Needs tests' but I took a brief view of the MR and see that there are tests. Are the tests here sufficient or do they need work?

@quietone

At least in my mind, I'm treating this issue as a "revert and fix" since the original issue summary is still valid, even if reverted. (The original issue isn't "fixed" if you will.)

Rebased on 10.0.x.

I think "Needs tests" can be removed, subject to additional review by Tim.

D10 version needed
At this time we need a D10.1.x patch or MR for this issue.

Patch or MR doesn't apply anymore
The last patch or MR doesn't apply to the target branch, please reroll the code so that it can be reviewed by the automated testbot.

@clayfreeman : Can you update the target branch to 10.1.x ?

Seems there were some erros in the MR 1549

Did not test or review yet.

Here is a patch for Drupal 10.2 based on latest MR.

@clayfreeman : Can you update the target branch to 11.x ?