Problem/Motivation
In order to solve #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides, there must be a mechanism for selecting a single SectionStorage plugin that is applicable for a set of available contexts.
Proposed resolution
First, section storage plugins must declare their necessary contexts. No longer will a section list be determined externally and passed in.
Introduce SectionStorageManagerInterface::findByContext($operation, array $contexts)
Passing in an array context, the manager will select the first section storage plugin whose context requirements are satisfied and which passes access checking for the given $operation
.
Section storage plugins can now also specify their weight, which affects the order in which they are checked by ::findByContext()
.
Remaining tasks
Wait for #3016458: ContextHandler should use setContext() not setContextValue()
User interface changes
N/A
API changes
Only within beta code (marked @internal
). Spelled out in the CRs. Two hard breaks are included:
-
BC cannot be provided for
getContexts()
because there is a method name collision withContextAwarePluginInterface::getContexts()
(now used), so the previous method has been renamed. -
Having an explicit public setter for
setSectionList()
is incompatible with deriving and setting the sections from context, and could have unpredictable results or introduce data integrity problems if the method is used with an incorrect section list (even one of the correct type).The method is removed from the interface so that
SectionStorage
plugins no longer need to implement it. Simply removing the method from the base class as well would produce a fatal that would not inform developers how they need to change their code, so we throw an exception linking the change record in the base class (and deprecate this dead code for removal in 9.0.0).
Additionally, #2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode) introduced access checking to SectionStorage plugins, but these access calls were only enforced at the routing level. Only with these changes does the new SectionStorageManagerInterface::findByContext()
enforce access checking.
Full details can be found in the change records:
- Layout Builder SectionStorage plugins no longer support having their section list injected, and must rely on context instead
- Layout section storage plugins should be loaded based on a list of available contexts
Data model changes
N/A
Release notes snippet
The Layout Builder module is designed to support extensible storage for layouts, so that layouts can be created and saved for various usecases. (For example, the core module has two storages, one for default content layouts and another for individual entity layouts.) However, a serious bug with the the beta API prevented other storages from being used. API changes were required to resolve this issue. Layout Builder section storage plugins must adapt to changes to their interface and rely on the context system instead of external setter calls. Additionally, those interacting with the Layout Builder API must use the new methods for loading section storage plugins.
Comment | File | Size | Author |
---|---|---|---|
#81 | 3016473-sectionstorage-81-interdiff.txt | 615 bytes | tim.plunkett |
#81 | 3016473-sectionstorage-81.patch | 96.21 KB | tim.plunkett |
#73 | 3016473-sectionstorage-no_update-73-interdiff.txt | 1.37 KB | tim.plunkett |
#73 | 3016473-sectionstorage-no_update-73.patch | 95.61 KB | tim.plunkett |
#71 | 3016473-sectionstorage-no_update-71-interdiff.txt | 907 bytes | tim.plunkett |
Comments
Comment #2
tim.plunkettIncorporates patch from #3016458: ContextHandler should use setContext() not setContextValue()
Comment #4
tim.plunkettgetContextValue is one of THOSE getters. Checking hasContextValue first fixes it.
Comment #6
tim.plunkett/me glares at opinionated unit test
Comment #7
tim.plunkettComment #8
tim.plunkett#3016458: ContextHandler should use setContext() not setContextValue() went in, straight reroll
Comment #9
phenaproximaThis patch makes sense. I can only find two minor things in tests; everything else is entirely sane. RTBC as far as I'm concerned.
This doc comment is wrong; setUp() instantiates DefaultsSectionStorage.
Why is the plugin ID 'overrides'?
Should we also add test cases for if $display->isOverridable() is false?
Comment #10
tim.plunkett#9
1) Copy/paste error; fixed
2) Copy/paste error; fixed
3) That feedback is valid for #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides where the isOverridable check is
Comment #11
phenaproximaThanks, @tim.plunkett.
I don't see any reason not to go ahead here. Godspeed.
Comment #12
xjmReviewing this...
After reading over the slimmed-down #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides and this issue's IS, I asked @tim.plunkett whether there's an access bypass in HEAD where users with view access could update layouts. According to @tim.plunkett this was not exposed anywhere in HEAD.
HEAD hardcodes the two trusted core storages (the entity defaults and individual entity overrides), and due to #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides it was not possible to use a different storage. Core checked update access in the routing layer when presenting the Layout Builder UI, rather than checking it when loading the sections from storage. (The two HEAD storages also don't do anything with the
$operation
at present, so essentially HEAD does not currently need to distinguish betweenview
andupdate
.)The potential for an access bypass would only affect contributed modules writing their own replacement implementations of the Layout Builder UI (unlikely at this point) that also did not do any of their own access checking, and using storage plugins that also didn't perform access checking or that included other security issues. Furthermore, even if a module did exist with such an access bypass (we don't know of any with stable releases), it would be likely to require some administrative or editorial permission to use anyway, so the impact would be very low. (Even if it we had a known access bypass along those lines, it would probably be approved for handling as a public issue.)
So, there's no security issue here.
When it becomes possible for contributed modules to provide their own storages and have those storages loaded contextually (after this fix and the followup), there will be an API for loading the storages, so that API must contain the correct access checking. For that reason, the access checking changes are included in this issue and its followup, rather than being scoped as a separate issue.
Comment #17
xjmAdding the contributors from the original issue.
Comment #18
xjmComment #19
xjmAdding my high-level text from #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides to the beginning of the release notes snippet. The release note needs to be informative to people who don't know what a "Layout Builder section storage plugin" is (so that they know when they can disregard it rather than having to figure out whether it applies to them).
Comment #20
xjmI'm still not convinced
extractIdFromRoute()
needs to be removed, even if there are other breaks for the storage plugins. It's a public method. Why not leave it in place and deprecate it? Actually, as far as I can see,extractEntityFromRoute()
could simply call it with a couple small tweaks to the logic.Comment #21
xjmBTW the split between the two issues makes it much easier to see what's going on, +1.
Comment #22
xjmComment #23
xjmIn the "API changes" section at the end of https://www.drupal.org/node/3016262, I think it's inaccurate to say
getSectionList()
has been added. It hasn't; it's been converted to abstract. Also, I think we should say of the originalgetContexts()
that it's been renamed togetContextsDuringPreview()
.Comment #24
xjmDone reviewing for now.
Comment #25
tim.plunkettAddressing #20 and all of the similar concerns. Excluding the required rename of getContexts() to getContextsDuringPreview(), a BC layer can be provided for everything except the base class implementation of
setSectionList()
. It now throws an exception in addition to the deprecation error. I provided BC for it on the two child classes.As far as #23 goes, I meant it was added in the sense that it's added to the list of things a SectionStorage plugin class has to do now. But I can reword.
And yes, will clarify about the fact that getContexts/getContextsDuringPreview is a rename.
I am opposed to this change because it will make it less clear to plugin authors what exactly they need to change. And if @internal wasn't enough to protect us from having to provide this BC, I don't know why we bother with it at all. But I will make the change anyway in order to get this issue resolved.
Comment #26
xjmWRT the end of #25. If the module had remained alpha, we of course could have made these changes. Unfortunately, we saw with earlier experimental modules that marking the alpha module's code
@internal
wasn't sufficient to keep people from using it and then having their stuff break when we neeeded to change it (even for very real reasons like this bug this is blocking). That's why alpha modules are no longer included in releases at all and why beta modules are required to meet the same stability expectations as stable core wherever possible.We also have plenty of experience at this point that even when things are internal, changing them breaks people's stuff. This is why we started deprecating it instead of removing it whenever possible. We have two problems with
@internal
in core at present -- 90% of the things that are listed as being@internal
by default in the BC policy aren't marked as such as core, so people have no idea unless they're core developers who've spent hours studying that huge page. And then half of the things in core that are currently marked internal are done so in a CYA way, things that are actually intended to be public API but then a core dev puts an@internal
on it because they think they might want to change it later. So the most@internal
developers see are actually possibly lies, so how can they trust it? This will get better once core's code is explicitly marked@internal
wherever possible. Even after that, though, we still need to deprecate first when possible whether or not something's internal.Reviewing #25.
Comment #27
xjm#25 looks great to me. I understand that it means there's a lot of deprecated baggage on the interfaces and base classes, but now that the change records are really clear and the deprecations are explicit, I think that developers will still understand how to update their plugins.
I do think we can and should have a hard break for
setSectionList()
. We can remove it from the interface without actually breaking implementations, so let's do that so its continued existence on the interface does not confuse people.I think that the exception thrown in the base class is a good idea. I'm not sure about the deprecated-but-semi-operable implementations on the two core storage plugins, though. What if someone passes in a section list that's of the right type, but the wrong section list for the actual contexts present? That could have unexpected results. So I'd let the child implementations inherit the exception-throwing parent.
Comment #28
xjmAlso, as a followup to this issue, I think we should discuss a way of deprecating the "public-ness" of all these excessively public methods. For stuff we want to keep around but just want to reduce the visibility of, we could come up with a standard format of deprecation message that says "
foo()
as a public method is deprecated and will become protected before Drupal 9.0.0" with a@trigger_error()
when the caller is not a parent or child.Comment #29
tim.plunkettReverted the docs fix to loadEmpty and opened a follow-up #3018021: Rename $id parameter to $type in SectionStorageManager
Discussed #27 with @xjm and agreed to remove the attempts to subclass setSectionList, but did leave it on the interface for those subclasses not extending SectionStorageBase. Clarified the message in the exception to not accidentally encourage workarounds.
Additionally wrote a BC layer for the constructor change to SectionStorageManager.
Comment #30
xjmThis comment does not describe what happens on the next line. Maybe this comment is supposed to go at the end of the method rather than the top? Or it should say "Only convert blahblah when there is a valid section storage type specified" or something.
Comment #31
xjmUpdating the IS with the BC breaks that remain (and why).
Comment #32
xjmSo this is a test fixture so it doesn't really matter, but these should be sentence-case, not title-case.
(Also "Cancel layout" should really be "Cancel layout changes" so that it's not to be confused with "Delete layout". That exists in HEAD; I can't remember if I ever filed an issue for it. Anyway.)
So the test implementation is thus suppressing the exception message that the base class throws.
I'm leaning toward removing this one method entirely. That doesn't help direct implementations or subclasses that already overrode the method, but I think that's like a 10% case, and better than having the codebase littered with variously broken implementations of this method that will come to exist in the future.
Usually we put inline comments on the providers that explain each scenario, so that people don't have to keep referring back to the test method to figure out what's going on. I guess the array keys kind of do here but I did have to keep scrolling back and forth. E.g.: "Is what enabled? Does what have data or not?" Parameter documentation on the test methods also helps with this.
Is this in scope?
Same note about provider scenarios.
This is an especially good example. ;)
Comment #33
tim.plunkett#30
I wrote that comment when I switched to a guard condition and accidentally undid the code change when trying to restore BC (but did not undo the comment change). Restoring the code flow to how it was in #10, matching the comment. Sorry the diff looks a messy, but it's switching from
if ($condition) { do_stuff(); }
toif (!$condition) { return; } do_stuff();
which makes the end result easier to understand.#32
1)
Those are copied from the others local tasks. I think 90% of LB's UI text uses Title Case for no apparent reason. Opened #3018073: Stop Using Title Case When Not Appropriate in Layout Builder UI
2)
The test class doesn't extend from the base class (on purpose) so this is required to exist.
3)
Improved docs
4)
It's needed for the expanded test coverage later in the class where we need to manipulate the SampleEntityGenerator mock
5)
Improved docs
6)
Improved docs and dataProvider keys
Comment #34
tedbowI get a fatal error when I try apply this patch after Layout Builder is already installed
I think the problem is
\Drupal\Core\Plugin\Context\ContextHandler::applyContextMapping()
line 86if (!empty($contexts[$context_id])) {
$context_id here is 'layout_builder.entity' but in $contexts the key is 'context'. Since this is require context we get the error.
If export my config I see core.entity_view_display.node.article.default.yml has
Several places.
The same error happens if you have pre-existing overrides because they also reference 'layout_builder.entity'
Should we update all overrides and defaults in an update hook or provide a BC layer?
Comment #35
tim.plunkettI looked into removing that change from this issue but it didn't work as well as I hoped. Instead I wrote a full update path and update path test.
Interdiff also includes some PHPCS fixes.
Finally, @catch and @xjm apparently discussed the setSectionList bit and would prefer to break the interface for this setter (but not for the getters).
Comment #37
xjmYep, this version has release management signoff. I updated the IS describing it. (Still will need to review the final patch again once we're RTBC, but the disruptions are reduced to what we can reasonably manage.)
Can we change this to:
And for the deprecation message:
Comment #38
xjmI updated the CR:
https://www.drupal.org/node/3016262/revisions/view/11215596/11218245
Comment #39
xjmComment #40
tim.plunkettI made both of the changes from #37, added @throws, and removed an extra use statement
Comment #41
tedbowCan we call
array_unique
on $entity_type_ids other wise we could have duplicates in the array if there are multiple bundles for one entity type that have overrides enabled.Otherwise looks good!
Comment #42
tim.plunkettAha! I had the array_filter but not unique.
As that change doesn't affect the outcome but just the number of extra loops, it's not testable.
Comment #43
tedbowLooks good! RTBC 🎉
Comment #44
xjmIsn't this potentially a massively huge list of data? If someone has content editors customizing entity layouts left and right.
50 here is magical. Is it a batch size? and if so when do we run the next batch? I'm missing something.
Some manual testing would be good (both of the upgrade path and probably just general misc. edgecases through the UI with the patch applied to make sure nothing's obviously broken.) I don't know that this has been manually tested recently.
Comment #45
tim.plunkettNW for 44. Will respond in full with a patch.
Comment #46
tim.plunkettThe update path for content entities was flawed and only worked for the first 50 entities of a single entity type :D
The new one works for all of them.
In order to do this properly, the config and content updates must run separately.
Config entities have the DX win of ConfigEntityUpdater which handles all the oddness of updates.
Also that magic 50 should have been
Settings::get('entity_update_batch_size', 50)
which is now used.Comment #48
tim.plunkettGot the syntax of that @covers wrong.
Comment #49
tim.plunkettRealized I never addressed #44.1
Yes this list could be absolutely massive. But this is an EntityQuery, so the list is all entity IDs only.
Like that. Hence the later loadMultiple() on a slice of that set, per batch run.
Comment #50
xjmRe: #49, 👍, thought they were objects.
Haven't fully reviewed #46 / #48 yet. One thing I noticed though in the test is that it's intended to test multiple batches, but the inline docs in the test don't really explain this; it's just implicit from the fact that it says "Run the first batch", "Run the next batch", etc.
Comment #51
xjmAlso should we have a test that has both config and content entities in the same test?
Comment #52
xjmCould have a followup to port
ConfigEntityUpdater
to content entities.Comment #53
tim.plunkettWe have
\Drupal\Tests\layout_builder\Functional\Update\LayoutBuilderContextMappingUpdatePathTest
for the generic case of "I have config and content that needs update, do both and make sure both UIs still work".Since the config update is a one-liner with ConfigEntityUpdater, that's enough.
For the content update we have
\Drupal\Tests\layout_builder\Kernel\RemoveContextMappingUpdateTest
that tests the actual update function as a pseudo-unit test.Comment #54
tedbowNit: Below you explicitly state the users have layout overrides. but not here.
Also wording confusing, sounds like each not each node is "of two bundles"
Maybe
"Create 5 nodes for each bundle."
If the test used a const for batch size it might make what is happening with each patch more obvious.
For instance
$this->assertSame(4, $sandbox['progress']);
would become
$this->assertSame(static::BATCH_SIZE, $sandbox['progress']);
Then
$this->assertSame(static::BATCH_SIZE * 2, $sandbox['progress']);
per @xjm's point maybe before the first batch explain the purpose of running multiple batches.
layout_builder_post_update_remove_context_mapping_from_content
so if you have any previous revisions of that have layout values these are not being update and will still have the old context mapping key.I just test manually and this does cause the same fatal error when you try to view old revisions.
Comment #55
tim.plunkettOkay this update path is getting out of hand. I went back and figured out how to NOT change
layout_builder.entity
here and will move the work done on that to #3018782: Remove extraneous context mapping of layout_builder.entity.Here's a patch restoring the
layout_builder.entity
functionality in HEAD, with interdiffs to before I started down this path and from the last patch.Moved the update related issue to the follow-up.
Comment #58
xjm"As expected" isn't ever really a good comment because it depends on the developer's assumptions or (mis)understanding. :)
Per @tim.plunkett
$assert_session->elementExists('css', '.field--name-body');
is actually specifically verifying that the body field is still visible on both the default form and on node/1 itself. We can put that in the clearerissueinline comment.Comment #59
xjmNeeds the NID of the followup here.
Comment #60
tedbowI tested this manually both with starting from 8.7.x and then switching to the patched version and starting from the patched version
Tested:
Comment #61
tim.plunkettAddressing #58 and #59
Thanks for #60!
Comment #62
tedbowLooks good to me! RTBC(again)
Comment #63
xjmThanks @tedbow for comprehensive manual testing!
Very minor nit: An
@see
to\Drupal\Core\ParamConverter\ParamConverterInterface::convert()
was added to this in the other issue for the$value
parameter, but I don't see it here. If this is all I find, it can be a followup I think....Oh, there's the @see.
So it looks like we are relying here on defaults being heavier than overrides, but we don't actually specify a weight for overrides. It might be better to explicitly set the overrides' weight to 0? And document on the respective plugins that they need those relative values.
I'm assuming we already have test coverage in HEAD that the overrides, well, override. :) So that would start failing if it were broken. But still better to be explicit with weights.
This isn't inheriting docs from anything.
Erg, comma splice. :P
Do we test the "for those with matching weights" part? That seems new as of this API addition, since weight is new.
Message doesn't match the method.
That's down to the test fixture, so I'm going to save this now. All minor stuff, but just enough to pass the threshold from "eff-it-ship-it" territory. ;) I'll continue reviewing the tests.
Comment #64
tim.plunkett1) Fixed it here anyway. And made these private! See, I'm learning.
2) Hmm, actually using the weights isn't needed until the follow-up. So, removing that part for now!
3) AFAICS it is inheriting from
\Drupal\layout_builder\SectionStorageInterface::getContextsDuringPreview
...4) They were good enough for Jane Austen! But I changed it for you.
5) We absolutely do, see
\Drupal\Tests\layout_builder\Unit\SectionStorageManagerTest::testFindDefinitions()
6) Sloppy c/p on my part.
Comment #66
tim.plunkettCKEditorIntegrationTest?
EDIT random fail, retest passed
Comment #67
xjmRe: #64.3, you're right, sorry. (I think this is a combined fail of my command key when searching through the patch and me being thrown off because the interface is in a different namespace from the base class.)
I'm thinking about the implications of making those methods private. If we have a contrib module that (e.g) extends the defaults to add some feature, but still acts on the same entity, do we really not want it to be able to use the method?
Comment #68
tim.plunkettThey can still call parent in deriveWhatever and use the results. I think this is correct
Comment #69
tim.plunkettTo clarify, they should absolutely not have access to this. Either they should implement their own or call the parent during deriving, or they should be using getContextValue elsewhere in the class
Comment #70
xjmCan we document #69 on the method docblock?
Comment #71
tim.plunkettDone. This matches the old warning on the similar methods but with more helpfulness.
Comment #72
tedbowHere we triggering a warning and saying
\Drupal\layout_builder\SectionStorageInterface::deriveContextsFromRoute()
should be used instead.But
SectionStorageInterface::deriveContextsFromRoute()
is @internal. Are telling code outside this module and core to use an @internal method?Should it not be internal but just include the existing comment about when it should not be called?
Comment #73
tim.plunkettAhhh fair. I'll move the comment up as an expansion of the docblock but not make it an @internal thing, as the corollary in HEAD is not @internal
Comment #74
xjmInterdiff in #73 lgtm.
So this obviously is not the fixture testing the access checking. :P
(This is likely fine for a test fixture that's not testing the access; just caused a kneejerk "whoops a bypass" reaction.)
...Yep, there's the access test in a kernel test with a whole data provider, as discussed in #12 etc., so it's fine for the other fixture to overshare.
Did we add a functional test for the access control to the followup's IS/scope? (Getting quite a list of those so want to make sure we don't forget stuff. The access, the weights, verifying whether we actually need the update path, ?)
On a skim it looks like these two tests share a good bit of code; should they share a base class?
(There we are. Hadn't gotten this far when asking #63.5 etc.)
Comment #75
tim.plunkett#74
1) The delegation patch has functional coverage as well as expanding this testAccess method to cover the changes to ::access in that patch
2) They do look to share a lot on the surface, but the differences between them are the important part, and the backflips to try to share code would make the test unreadable.
providerTestAccess
is identical right now, but that will change in the delegation patch, as referenced aboveComment #76
tedbow#73 looks
Also #75 seems to answer #74
Looks good to me
Comment #78
xjmCommitted and pushed to 8.7.x. Bam! Also published the CRs and now unpostponing the main followup.
Comment #80
xjm@tedbow discovered that this breaks existing layouts. STR:
This is due to the changed annotation definition. We just need to add a cache clear in a post-update. This can't be tested because the test will not have a cache of the old definitions.
Comment #81
tim.plunkettNeeded a post_update to clear caches for those plugin annotation changes.
Copied the docblock/comment style from other empty post_updates
Comment #82
tedbowTested this out and fixes problem documented in #80
Comment #84
xjmOK, recommitted. Thanks @tim.plunkett and @tedbow for the quick fix!
Comment #86
drclaw CreditAttribution: drclaw at Fuse Interactive commentedThe fix in #80 works for existing layouts, but any SectionStorage objects that previously existed in temp storage will still have the ContextException thrown. I posted a new issue and potential fix for this #3044211: SectionStorage objects in tempstore are broken when updating from 8.6.x to 8.7.x. Hopefully I'm not too far off base on the fix...