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:

  1. BC cannot be provided for getContexts() because there is a method name collision with ContextAwarePluginInterface::getContexts() (now used), so the previous method has been renamed.

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

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.

CommentFileSizeAuthor
#81 3016473-sectionstorage-81-interdiff.txt615 bytestim.plunkett
#81 3016473-sectionstorage-81.patch96.21 KBtim.plunkett
#73 3016473-sectionstorage-no_update-73-interdiff.txt1.37 KBtim.plunkett
#73 3016473-sectionstorage-no_update-73.patch95.61 KBtim.plunkett
#71 3016473-sectionstorage-no_update-71-interdiff.txt907 bytestim.plunkett
#71 3016473-sectionstorage-no_update-71.patch95.59 KBtim.plunkett
#64 3016473-sectionstorage-no_update-64-interdiff.txt6.12 KBtim.plunkett
#64 3016473-sectionstorage-no_update-64.patch95.37 KBtim.plunkett
#61 3016473-sectionstorage-no_update-61-interdiff.txt2.31 KBtim.plunkett
#61 3016473-sectionstorage-no_update-61.patch95.07 KBtim.plunkett
#55 3016473-sectionstorage-no_update-55.patch94.98 KBtim.plunkett
#55 3016473-sectionstorage-no_update-55-interdiff-from-35.txt24.3 KBtim.plunkett
#55 3016473-sectionstorage-no_update-55-interdiff-from-48.txt22.55 KBtim.plunkett
#48 3016473-sectionstorage-full-47-interdiff.txt1.19 KBtim.plunkett
#48 3016473-sectionstorage-full-47.patch113.76 KBtim.plunkett
#46 3016473-sectionstorage-full-46-interdiff.txt13.61 KBtim.plunkett
#46 3016473-sectionstorage-full-46.patch113.64 KBtim.plunkett
#42 3016473-sectionstorage-full-42-interdiff.txt1.06 KBtim.plunkett
#42 3016473-sectionstorage-full-42.patch105.22 KBtim.plunkett
#40 3016473-sectionstorage-full-40-interdiff.txt5.09 KBtim.plunkett
#40 3016473-sectionstorage-full-40.patch105.21 KBtim.plunkett
#35 3016473-sectionstorage-full-35-interdiff.txt22.13 KBtim.plunkett
#35 3016473-sectionstorage-full-35-PASS.patch105 KBtim.plunkett
#35 3016473-sectionstorage-full-35-FAIL.patch101.65 KBtim.plunkett
#33 3016473-sectionstorage-bc-33-interdiff.txt11.34 KBtim.plunkett
#33 3016473-sectionstorage-bc-33.patch86.83 KBtim.plunkett
#32 true.jpg24.13 KBxjm
#29 3016473-sectionstorage-bc-29-interdiff.txt11.07 KBtim.plunkett
#29 3016473-sectionstorage-bc-29.patch83.99 KBtim.plunkett
#25 3016473-sectionstorage-bc-25-interdiff.txt39.64 KBtim.plunkett
#25 3016473-sectionstorage-bc-25.patch82.42 KBtim.plunkett
#10 3016473-sectionstorage-10-interdiff.txt1.03 KBtim.plunkett
#10 3016473-sectionstorage-10.patch78.53 KBtim.plunkett
#8 3016473-sectionstorage-8.patch78.53 KBtim.plunkett
#6 3016473-sectionstorage-6-interdiff.txt1.64 KBtim.plunkett
#6 3016473-sectionstorage-6.patch86.41 KBtim.plunkett
#4 3016473-sectionstorage-4-interdiff.txt908 bytestim.plunkett
#4 3016473-sectionstorage-4.patch84.58 KBtim.plunkett
#2 3016473-sectionstorage-2.patch84.54 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Needs review » Needs work

The last submitted patch, 2: 3016473-sectionstorage-2.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
84.58 KB
908 bytes

getContextValue is one of THOSE getters. Checking hasContextValue first fixes it.

Status: Needs review » Needs work

The last submitted patch, 4: 3016473-sectionstorage-4.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
86.41 KB
1.64 KB

/me glares at opinionated unit test

tim.plunkett’s picture

tim.plunkett’s picture

phenaproxima’s picture

This patch makes sense. I can only find two minor things in tests; everything else is entirely sane. RTBC as far as I'm concerned.

  1. +++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
    @@ -0,0 +1,139 @@
    +  /**
    +   * The plugin.
    +   *
    +   * @var \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage
    +   */
    +  protected $plugin;
    

    This doc comment is wrong; setUp() instantiates DefaultsSectionStorage.

  2. +++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
    @@ -0,0 +1,139 @@
    +    $this->plugin = DefaultsSectionStorage::create($this->container, [], 'overrides', new SectionStorageDefinition());
    

    Why is the plugin ID 'overrides'?

  3. +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
    @@ -0,0 +1,124 @@
    +  public function providerTestAccess() {
    +    $section_data = [
    +      new Section('layout_default', [], [
    +        'first-uuid' => new SectionComponent('first-uuid', 'content', ['id' => 'foo']),
    +      ]),
    +    ];
    +
    +    $data = [];
    +    $data['view, disabled, no data'] = [FALSE, 'view', FALSE, []];
    +    $data['view, enabled, no data'] = [TRUE, 'view', TRUE, []];
    +    $data['view, disabled, data'] = [FALSE, 'view', FALSE, $section_data];
    +    $data['view, enabled, data'] = [TRUE, 'view', TRUE, $section_data];
    +    return $data;
    +  }
    

    Should we also add test cases for if $display->isOverridable() is false?

tim.plunkett’s picture

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @tim.plunkett.

I don't see any reason not to go ahead here. Godspeed.

xjm’s picture

Assigned: Unassigned » xjm

Reviewing 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 between view and update.)

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.

xjm credited EclipseGc.

xjm credited larowlan.

xjm credited tedbow.

xjm’s picture

Adding the contributors from the original issue.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

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

xjm’s picture

Status: Reviewed & tested by the community » Needs review

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

xjm’s picture

BTW the split between the two issues makes it much easier to see what's going on, +1.

xjm’s picture

Issue summary: View changes
xjm’s picture

In 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 original getContexts() that it's been renamed to getContextsDuringPreview().

xjm’s picture

Assigned: xjm » Unassigned

Done reviewing for now.

tim.plunkett’s picture

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

xjm’s picture

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

xjm’s picture

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

xjm’s picture

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

tim.plunkett’s picture

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

xjm’s picture

+++ b/core/modules/layout_builder/src/Routing/LayoutTempstoreParamConverter.php
@@ -45,8 +45,12 @@ public function __construct(LayoutTempstoreRepositoryInterface $layout_tempstore
+    // If no section storage type is specified or if it is invalid, return.
     if (isset($defaults['section_storage_type']) && $this->sectionStorageManager->hasDefinition($defaults['section_storage_type'])) {

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

xjm’s picture

Issue summary: View changes

Updating the IS with the BC breaks that remain (and why).

xjm’s picture

Issue summary: View changes
FileSize
24.13 KB
  1. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/src/Plugin/SectionStorage/SimpleConfigSectionStorage.php
    @@ -0,0 +1,225 @@
    +      'title' => $this->t('Save Layout'),
    ...
    +      'title' => $this->t('Cancel Layout'),
    

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

  2. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/src/Plugin/SectionStorage/SimpleConfigSectionStorage.php
    @@ -0,0 +1,225 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setSectionList(SectionListInterface $section_list) {
    +    @trigger_error('\Drupal\layout_builder\SectionStorageInterface::setSectionList() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. The section list should be derived from context. See https://www.drupal.org/node/3016262.', E_USER_DEPRECATED);
    +    return $this;
    +  }
    

    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.

  3. +++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
    @@ -0,0 +1,152 @@
    +    $data['view, disabled, no data'] = [FALSE, 'view', FALSE, []];
    +    $data['view, enabled, no data'] = [TRUE, 'view', TRUE, []];
    +    $data['view, disabled, data'] = [FALSE, 'view', FALSE, $section_data];
    +    $data['view, enabled, data'] = [TRUE, 'view', TRUE, $section_data];
    
    +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
    @@ -0,0 +1,137 @@
    +    $data['view, disabled, no data'] = [FALSE, 'view', FALSE, []];
    +    $data['view, enabled, no data'] = [TRUE, 'view', TRUE, []];
    +    $data['view, disabled, data'] = [FALSE, 'view', FALSE, $section_data];
    +    $data['view, enabled, data'] = [TRUE, 'view', TRUE, $section_data];
    

    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.

  4. +++ b/core/modules/layout_builder/tests/src/Unit/DefaultsSectionStorageTest.php
    @@ -37,6 +38,13 @@ class DefaultsSectionStorageTest extends UnitTestCase {
    +  /**
    +   * The sample entity generator.
    +   *
    +   * @var \Drupal\layout_builder\Entity\LayoutBuilderSampleEntityGenerator
    +   */
    +  protected $sampleEntityGenerator;
    +
       /**
        * {@inheritdoc}
        */
    @@ -45,13 +53,13 @@ protected function setUp() {
    
    @@ -45,13 +53,13 @@ protected function setUp() {
     
         $this->entityTypeManager = $this->prophesize(EntityTypeManagerInterface::class);
         $entity_type_bundle_info = $this->prophesize(EntityTypeBundleInfoInterface::class);
    -    $sample_entity_generator = $this->prophesize(LayoutBuilderSampleEntityGenerator::class);
    +    $this->sampleEntityGenerator = $this->prophesize(LayoutBuilderSampleEntityGenerator::class);
     
         $definition = new SectionStorageDefinition([
           'id' => 'defaults',
           'class' => DefaultsSectionStorage::class,
         ]);
    -    $this->plugin = new DefaultsSectionStorage([], '', $definition, $this->entityTypeManager->reveal(), $entity_type_bundle_info->reveal(), $sample_entity_generator->reveal());
    +    $this->plugin = new DefaultsSectionStorage([], '', $definition, $this->entityTypeManager->reveal(), $entity_type_bundle_info->reveal(), $this->sampleEntityGenerator->reveal());
    

    Is this in scope?

  5. +++ b/core/modules/layout_builder/tests/src/Unit/DefaultsSectionStorageTest.php
    @@ -194,6 +218,99 @@ public function testGetSectionListFromIdCreate() {
    +  /**
    +   * @covers ::extractEntityFromRoute
    +   *
    +   * @dataProvider providerTestExtractEntityFromRoute
    +   */
    +  public function testExtractEntityFromRoute($success, $expected_entity_id, $value, array $defaults = []) {
    ...
    +  /**
    +   * Provides data for ::testExtractEntityFromRoute().
    +   */
    +  public function providerTestExtractEntityFromRoute() {
    

    Same note about provider scenarios.

  6. +++ b/core/modules/layout_builder/tests/src/Unit/SectionStorageManagerTest.php
    @@ -74,27 +125,130 @@ public function testLoadFromStorageId() {
    +  /**
    +   * Provides test data for ::testFindByContext().
    +   */
    +  public function providerTestFindByContext() {
    +    $data = [];
    +    $data['true'] = [TRUE];
    +    $data['false'] = [FALSE];
    +    return $data;
    +  }
    

    This is an especially good example. ;)

tim.plunkett’s picture

#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(); } to if (!$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

tedbow’s picture

Status: Needs review » Needs work

I get a fatal error when I try apply this patch after Layout Builder is already installed

  1. Install Drupal on 8.7.x
  2. Enable layout builder
  3. Enable layout builder on Articles
  4. (optional step, error either way) Make change and save the layout
  5. Apply patch
  6. Rebuild Cache (also can run drush updb but there are no updates)
  7. Click Manage Layout
  8. Error
The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Component\Plugin\Exception\ContextException</em>: Assigned contexts were not satisfied: entity in <em class="placeholder">Drupal\Core\Plugin\Context\ContextHandler-&gt;applyContextMapping()</em> (line <em class="placeholder">121</em> of <em class="placeholder">core/lib/Drupal/Core/Plugin/Context/ContextHandler.php</em>). <pre class="backtrace">Drupal\layout_builder\SectionComponent-&gt;getPlugin(Array) (Line: 74)
Drupal\layout_builder\Event\SectionComponentBuildRenderArrayEvent-&gt;__construct(Object, Array, 1) (Line: 99)
Drupal\layout_builder\SectionComponent-&gt;toRenderArray(Array, 1) (Line: 80)
Drupal\layout_builder\Section-&gt;toRenderArray(Array, 1) (Line: 229)
Drupal\layout_builder\Controller\LayoutBuilderController-&gt;buildAdministrativeSection(Object, 0) (Line: 103)
Drupal\layout_builder\Controller\LayoutBuilderController-&gt;layout(Object, )
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache-&gt;pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 669)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)
</pre>

I think the problem is \Drupal\Core\Plugin\Context\ContextHandler::applyContextMapping() line 86
if (!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

              context_mapping:
                entity: layout_builder.entity

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?

tim.plunkett’s picture

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

The last submitted patch, 35: 3016473-sectionstorage-full-35-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

xjm’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs release manager review

Yep, 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.)

  1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
    @@ -21,7 +21,17 @@ abstract class SectionStorageBase extends ContextAwarePluginBase implements Sect
    +   * @internal
    +   *   This should only be called during section storage instantiation.
    

    Can we change this to:

    As of Drupal 8.7.0, this method should no longer be used. It previously should only have been used during storage instantiation.

  2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
    @@ -21,7 +21,17 @@ abstract class SectionStorageBase extends ContextAwarePluginBase implements Sect
    +   * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. The
    +   *   section list should be derived from context. See
    +   *   https://www.drupal.org/node/3016262.
    

    And for the deprecation message:

    [...] before 9.0.0. This method should no longer be used. The section list [...]

xjm’s picture

xjm’s picture

Issue summary: View changes
tim.plunkett’s picture

I made both of the changes from #37, added @throws, and removed an extra use statement

tedbow’s picture

Status: Needs review » Needs work
+++ b/core/modules/layout_builder/layout_builder.post_update.php
@@ -58,3 +63,57 @@ function layout_builder_post_update_add_extra_fields(&$sandbox = NULL) {
+    return $display->isOverridable() ? $display->getTargetEntityTypeId() : FALSE;
+  }, LayoutBuilderEntityViewDisplay::loadMultiple()));

Can 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!

tim.plunkett’s picture

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

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! RTBC 🎉

xjm’s picture

  1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
    @@ -58,3 +63,57 @@ function layout_builder_post_update_add_extra_fields(&$sandbox = NULL) {
    +      $sandbox[$entity_type_id]['entities'] = $storage->getQuery()
    +        ->exists(aOverridesSectionStorage::FIELD_NAME)
    +        ->accessCheck(FALSE)
    +        ->execute();
    

    Isn't this potentially a massively huge list of data? If someone has content editors customizing entity layouts left and right.

  2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
    @@ -58,3 +63,57 @@ function layout_builder_post_update_add_extra_fields(&$sandbox = NULL) {
    +    $entities = $storage->loadMultiple(array_splice($sandbox[$entity_type_id]['entities'], 0, 50));
    

    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.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing

NW for 44. Will respond in full with a patch.

tim.plunkett’s picture

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

Status: Needs review » Needs work

The last submitted patch, 46: 3016473-sectionstorage-full-46.patch, failed testing. View results

tim.plunkett’s picture

Got the syntax of that @covers wrong.

tim.plunkett’s picture

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

[
  '2' => '2',
]

Like that. Hence the later loadMultiple() on a slice of that set, per batch run.

xjm’s picture

Re: #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.

xjm’s picture

Also should we have a test that has both config and content entities in the same test?

xjm’s picture

Could have a followup to port ConfigEntityUpdater to content entities.

tim.plunkett’s picture

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

tedbow’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/layout_builder/tests/src/Kernel/RemoveContextMappingUpdateTest.php
    @@ -0,0 +1,223 @@
    +    // Create 5 nodes each of two different bundles.
    

    Nit: 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."

  2. +++ b/core/modules/layout_builder/tests/src/Kernel/RemoveContextMappingUpdateTest.php
    @@ -0,0 +1,223 @@
    +    $settings['entity_update_batch_size'] = 4;
    

    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']);

  3. +++ b/core/modules/layout_builder/tests/src/Kernel/RemoveContextMappingUpdateTest.php
    @@ -0,0 +1,223 @@
    +    // Run the first batch.
    

    per @xjm's point maybe before the first batch explain the purpose of running multiple batches.

  4. 😱 Just realized we are not taking into consideration revisions of content entities in 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.

tim.plunkett’s picture

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

xjm’s picture

+++ b/core/modules/layout_builder/tests/src/Functional/Update/LayoutBuilderContextMappingUpdatePathTest.php
@@ -0,0 +1,44 @@
+    // Ensure that both defaults and overrides continue to function as expected.
...
+    $assert_session->elementExists('css', '.field--name-body');

"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 clearer issue inline comment.

xjm’s picture

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -257,9 +298,14 @@ public function getLayoutBuilderUrl($rel = 'view') {
+    // @todo Remove the magic string 'layout_builder.entity'.

Needs the NID of the followup here.

tedbow’s picture

Issue tags: -Needs manual testing

I tested this manually both with starting from 8.7.x and then switching to the patched version and starting from the patched version

Tested:

  1. Enabled layout builder and overrides for articles
  2. altering default layout
  3. creating article
  4. overriding layout
  5. editing article to create new revision
  6. editing layout override
  7. view revisions
  8. reverting to a revision
tim.plunkett’s picture

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! RTBC(again)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @tedbow for comprehensive manual testing!

  1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
    @@ -278,15 +278,73 @@ public function getSectionListFromId($id) {
    +  /**
    +   * Extracts an entity from the route values.
    +   *
    +   * @param mixed $value
    +   *   The raw value from the route.
    +   * @param array $defaults
    +   *   The route defaults array.
    +   *
    +   * @return \Drupal\Core\Entity\EntityInterface|null
    +   *   The entity for the route, or NULL if none exist.
    +   */
    

    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.

    +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
    @@ -115,16 +107,41 @@ public function getLayoutBuilderUrl($rel = 'view');
    +   * @see \Drupal\Core\ParamConverter\ParamConverterInterface::convert()
    +   */
    +  public function deriveContextsFromRoute($value, $definition, $name, array $defaults);
    

    ...Oh, there's the @see.

  2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
    @@ -25,6 +23,10 @@
      * @SectionStorage(
      *   id = "defaults",
    + *   weight = 10,
    + *   context_definitions = {
    + *     "display" = @ContextDefinition("entity:entity_view_display"),
    + *   },
      * )
    
    +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
    @@ -23,6 +21,10 @@
      * @SectionStorage(
      *   id = "overrides",
    + *   context_definitions = {
    + *     "entity" = @ContextDefinition("entity"),
    + *     "view_mode" = @ContextDefinition("string", required = FALSE),
    + *   }
      * )
    

    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.

  3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
    @@ -103,4 +101,11 @@ public function removeSection($delta) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getContextsDuringPreview() {
    +    return $this->getContexts();
    +  }
    +
    

    This isn't inheriting docs from anything.

  4. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
    @@ -28,14 +43,67 @@ class SectionStorageManager extends DefaultPluginManager implements SectionStora
    +      @trigger_error('The context.handler service must be passed to \Drupal\layout_builder\SectionStorage\SectionStorageManager::__construct(), it was added in Drupal 8.7.0 and will be required before Drupal 9.0.0.', E_USER_DEPRECATED);
    

    Erg, comma splice. :P

  5. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
    @@ -28,14 +43,67 @@ class SectionStorageManager extends DefaultPluginManager implements SectionStora
    +    // Sort the definitions by their weight while preserving the original order
    +    // for those with matching weights.
    

    Do we test the "for those with matching weights" part? That seems new as of this API addition, since weight is new.

  6. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
    @@ -47,25 +115,18 @@ public function loadEmpty($id) {
       public function loadFromStorageId($type, $id) {
    -    /** @var \Drupal\layout_builder\SectionStorageInterface $plugin */
    -    $plugin = $this->createInstance($type);
    -    return $plugin->setSectionList($plugin->getSectionListFromId($id));
    +    @trigger_error('\Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface::loadFromRoute() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface::load() should be used instead. See https://www.drupal.org/node/3012353.', E_USER_DEPRECATED);
    

    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.

tim.plunkett’s picture

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

Status: Needs review » Needs work

The last submitted patch, 64: 3016473-sectionstorage-no_update-64.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review

CKEditorIntegrationTest?

EDIT random fail, retest passed

xjm’s picture

Re: #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?

tim.plunkett’s picture

They can still call parent in deriveWhatever and use the results. I think this is correct

tim.plunkett’s picture

To 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

xjm’s picture

Can we document #69 on the method docblock?

tim.plunkett’s picture

Done. This matches the old warning on the similar methods but with more helpfulness.

tedbow’s picture

+++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
@@ -47,25 +115,18 @@ public function loadEmpty($id) {
+    @trigger_error('\Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface::loadFromRoute() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. \Drupal\layout_builder\SectionStorageInterface::deriveContextsFromRoute() and \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface::load() should be used instead. See https://www.drupal.org/node/3012353.', E_USER_DEPRECATED);
+    $contexts = $this->loadEmpty($type)->deriveContextsFromRoute($value, $definition, $name, $defaults);

Here 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?

tim.plunkett’s picture

Ahhh 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

xjm’s picture

Interdiff in #73 lgtm.

  1. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/src/Plugin/SectionStorage/SimpleConfigSectionStorage.php
    @@ -0,0 +1,216 @@
    +  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
    +    $result = AccessResult::allowed();
    +    return $return_as_object ? $result : $result->isAllowed();
    +  }
    
    +++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
    @@ -0,0 +1,166 @@
    +  /**
    +   * @covers ::access
    +   * @dataProvider providerTestAccess
    +   *
    +   * @param bool $expected
    +   *   The expected outcome of ::access().
    +   * @param string $operation
    +   *   The operation to pass to ::access().
    +   * @param bool $is_enabled
    +   *   Whether Layout Builder is enabled for this display.
    +   * @param array $section_data
    +   *   Data to store as the sections value for Layout Builder.
    +   */
    +  public function testAccess($expected, $operation, $is_enabled, array $section_data) {
    

    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, ?)

  2. +++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
    @@ -0,0 +1,166 @@
    +class DefaultsSectionStorageTest extends KernelTestBase {
    
    +++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
    @@ -0,0 +1,151 @@
    +class OverridesSectionStorageTest extends KernelTestBase {
    

    On a skim it looks like these two tests share a good bit of code; should they share a base class?

  3. +++ b/core/modules/layout_builder/tests/src/Unit/SectionStorageManagerTest.php
    @@ -74,27 +125,135 @@ public function testLoadFromStorageId() {
    +   * @covers ::findDefinitions
    +   */
    +  public function testFindDefinitions() {
    +    $this->discovery->getDefinitions()->willReturn([
    +      'plugin1' => new SectionStorageDefinition(),
    +      'plugin2' => new SectionStorageDefinition(['weight' => -5]),
    +      'plugin3' => new SectionStorageDefinition(['weight' => -5]),
    +      'plugin4' => new SectionStorageDefinition(['weight' => 10]),
    +    ]);
    

    (There we are. Hadn't gotten this far when asking #63.5 etc.)

tim.plunkett’s picture

#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 above

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

#73 looks
Also #75 seems to answer #74

Looks good to me

  • xjm committed 9c77f61 on 8.7.x
    Issue #3016473 by tim.plunkett, xjm, tedbow, phenaproxima, EclipseGc,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.7.x. Bam! Also published the CRs and now unpostponing the main followup.

  • xjm committed 5a929b6 on 8.7.x
    Revert "Issue #3016473 by tim.plunkett, xjm, tedbow, phenaproxima,...
xjm’s picture

Status: Fixed » Needs work

@tedbow discovered that this breaks existing layouts. STR:

1. `git checkout 4f02a38c3041c1f02cb651c9b1a2418160d9ce5c`
2. `drush si -y && drush en layout_builder -y && drush upwd admin pass`
3. make layouts
4. `git checkout 8.7.x`
5. view override(didn’t try defaults)

The website encountered an unexpected error. Please try again later.Drupal\Component\Plugin\Exception\ContextException: The entity context is not a valid context. in Drupal\Component\Plugin\ContextAwarePluginBase->getContextDefinition() (line 92 of core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php).

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
96.21 KB
615 bytes

Needed a post_update to clear caches for those plugin annotation changes.

Copied the docblock/comment style from other empty post_updates

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Tested this out and fixes problem documented in #80

  • xjm committed 88ad013 on 8.7.x
    Issue #3016473 by tim.plunkett, xjm, tedbow, phenaproxima, EclipseGc,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

OK, recommitted. Thanks @tim.plunkett and @tedbow for the quick fix!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

drclaw’s picture

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