Support third-party settings for components within a section

Let's do this in #2942661: Sections should have third-party settings after all

Splitting back out from #2942661: Sections should have third-party settings because adding third party settings to Sections is more complicated because of changes the constructor and BC

Only have functional code, does not have testing code yet. Just want to make things working. I am idiot for writing testing code.

Fix failed testing.

I don't think there is any value to additional if we can have TPS.
The problem is how to provide BC in the meantime.

Field Formatter Class module also suffers from an issue where the third party settings are somehow stripped by Layout Builder when it switches to the internal '_custom' view mode for field blocks.

The issue I'm currently battling on in contrib:

#3046372: Classes not being applied with Layout Builder

Edit: I just realized the patches on this are for SectionComponent.php class, which I assume is what "blocks" in the UI are?

Cross-post but with the patch from #5 applied to 8.7.1, I'm still unable to access the third_party_settings from hook_preprocess_field function for a field rendered by Layout Builder for anonymous users.

I've tried loading settings both the current way that the field_formatter_class module is written:

  $render_display = EntityViewDisplay::collectRenderDisplay($entity, $view_mode);
  $field_display = $render_display->getComponent($field_name);
  var_dump($field_display['third_party_settings']);

But the documentation for EntityViewDisplay::collectRenderDisplay make it sound like this way is going to be deprecated and points to using entity_view_display() which ALSO has a deprecation warning and suggests using:

  $view_display = \Drupal::entityTypeManager()
    ->getStorage('entity_view_display')
    ->load($entity->getEntityType()->id() . '.' . $entity->bundle() . '.' . $view_mode);
 

Neither of these ways above produce the correct config even with the patch applied. Layout Builder's '_custom' view mode seems to still be stripping out third party settings.

In fact, Drupal 8.8.x introduced EntityDisplayRepositoryInterface::getViewDisplay() as the way to do this, but since I'm working on 8.7.x I cannot use that yet.

Its late in the week, so I'm probably doing something wrong.

Patch from #5 is working with Block Class with additional patch on that module (https://www.drupal.org/project/block_class/issues/2998114).

I confirm too, that patch works well together with https://www.drupal.org/project/block_class/issues/2998114#comment-13137003 for add classes to layout blocks in Drupal 8.7 core.

Just tested and indeed works fine. Changing ticket status.

This is a reroll of #5 for 8.8.x.

Umami uses layout builder for the Full recipe view mode. The config test for it was failing since this patch adds in extra config for third party settings. This new patch hopefully fixes that.

This is looking good. I like that the additional property is properly deprecated :)

One note on the test modifications though:

+++ b/core/modules/layout_builder/tests/src/Functional/Update/LayoutBuilderEnableUpdatePathTest.php
@@ -45,6 +45,7 @@ public function testRunUpdates() {
+              'third_party_settings' => [],

I don't think we can just add this to the update test fixture. Rather, we need an update hook with this patch that would set this property in existing configs, and then a test to ensure that update path properly adds the property. This test could use the same fixtures as the existing update tests the module has I think. See ExtraFieldUpdatePathTest, or the test modified in this patch for examples.

Removed third_party_settings from LayoutBuilderEnableUpdatePathTest.

Sorry, I quoted the wrong code in #22. It actually needs to be removed from core/modules/layout_builder/tests/fixtures/update/layout-builder-enable.php, which is a test fixture that mimics an existing installation. It should be added back to the LayoutBuilderEnableUpdatePathTest , but for that to then pass, we'll need an update hook that adds third_party_settings to existing installations/configs.

@jhedstrom Ah, ok, no worries, my patch was meant to be just a test before more work on this during DrupalCon contribution day, but in the end I couldn't get tests working locally, so had to give up.

I'm still fighting with local tests, but meanwhile here is a reroll of the earlier #19 patch.

I'm curious, how is this issue different from #2942661: Sections should have third-party settings regarding the implementation of tests? Shouldn't this issue mimic that one, or am I missing something?

Re-rolled the patch against 8.8.0. Hope this is fine :)

Tested the re-roll and it looks good. Adding my RBTC to this one. Let's get it committed upstream so we can fix it at the specific module levels :)

This is going to need a change record: See https://www.drupal.org/node/3035096 as an example.

Added a draft change record for this patch. Please review it here and add more detail if it is needed.

--- a/core/modules/layout_builder/config/schema/layout_builder.schema.yml
+++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml

@@ -52,6 +52,11 @@ layout_builder.component:
+      sequence:
+        type: ignore

I'm not sure this is correct. I think that the module that provides the new settings should be able to add a schema here.

Also it look like we're missing dependencies as well. So when you uninstall a module that allows information to be stored here the information should be removed as it is no longer relevant.

What about the following?

@@ -52,6 +52,11 @@ layout_builder.component:
+      sequence:
+        type: 'layout_builder.component.third_party.[%key]'

This works in my testing with the following contrib schema:

layout_builder.component.third_party.my_key:
  type: mapping
  label: My label
  mapping:
    classes:
      type: string
      label: My classes

For the sake of consistency, I tried unsuccessfully to adapt the third-party settings type for sections, '[%parent.%parent.%type].third_party.[%key]'. I ended up with '[%parent.%parent.%parent.%type].third_party_settings.layout_builder.sections.[%parent.%parent.%parent.%key].components.[%parent.uuid].third_party_settings.[%key]' (and also tried countless various thereof).

It also looks like we'll need post-update code:

/**
 * Clear caches due to config schema additions.
 */
function layout_builder_post_update_component_third_party_settings_schema() {
  // Empty post-update hook.
}

Thank you very much @Chris Burge, @alexpott and the others! I just ran into a situation in "fences_blocks" where we have the same requirement (#3117170). Sadly I'm not experienced enough with the schema topic in #34 to help to answer this.

I'm willing to help writing tests to get this done, if someone could teach me what to test and perhaps show a first example so I can do the time consuming further test implementations perhaps?

The current patch in #26 is applying cleanly in 8.9.0.

New features will only go into 9.1.x now.

This is a feature, we're hoping to keep the Bug Smash tag just for Bugs. But feel free to help move this along!

Added reroll of patch #26 for Drupal 9.1.x.

Still needs work for #33.

Looking at how we generally do third-party settings for config-objects - core's entity schema for config_entity type has this

third_party_settings:
      type: sequence
      label: 'Third party settings'
      sequence:
        type: '[%parent.%parent.%type].third_party.[%key]'

E.g. looking at how contact_storage provides schema for the third-party settings it adds to contact forms, it has this in its schema

contact.form.*.third_party.contact_storage:
  type: mapping
  label: 'Contact form redirection'
  mapping: {details here}

So I think we need to do similar but also

• Add a test module that interacts with components via third-party
• Add a schema to that module
• verify in a test that the schema is correctly applied

The 'type' value in the schema is the major hurdle at this point IMHO. (As I commented in #34, I have no idea what the 'type' value should be for the schema. I tried a bunch of stuff that didn't work.)

It also looks like we'll need post-update code:

/**
 * Clear caches due to config schema additions.
 */
function layout_builder_post_update_component_third_party_settings_schema() {
  // Empty post-update hook.
}

Agreed we need to add test coverage. (Tag added to issue).

The only test-related code in the patch is below:

--- a/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
+++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
@@ -115,7 +115,7 @@ public function providerTestAccess() {
         'layout_onecol',
         [],
         [
-          'first-uuid' => new SectionComponent('first-uuid', 'content', ['id' => 'foo'], ['harold' => 'maude']),
+          'first-uuid' => new SectionComponent('first-uuid', 'content', ['id' => 'foo'], ['harold' => 'maude'], ['layout_builder_defaults_test' => ['harold' => 'kumar']]),
         ],
         ['layout_builder_defaults_test' => ['which_party' => 'third']]
       ),

> The 'type' value in the schema is the major hurdle at this point

It must be a definition that allows the third parties to actually define the schema. Look how it's done in other places. So here it is:

    third_party_settings:
      type: sequence
      label: 'Third party settings'
      sequence:
        type: '[%parent.%parent.%type].third_party.[%key]'

(Note that the "[%parent.%parent.%type]" here resolves to "layout_builder.component". (We could have written that.)
So if 'mymodule' adds some TPS, it will define layout_builder.component.third_party.mymodule.

HTH!

@geek-merlin - Thanks for your comment. That's 100% correct. When I was testing, I used patch #67 from Block Class #2998114: Integration with Drupal core's new Layout Builder. Upon reviewing your comment, I discovered that the necessary schema was missing from that patch. When I account for the missing schema, everything works perfectly. SO - no schema issues on this core issue.

That just leaves the post-update code and test coverage. Re test coverage, I'd look to #2942661: Sections should have third-party settings for guidance.

+++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
@@ -52,6 +52,11 @@ layout_builder.component:
+        type: ignore

I think this line has to be changed to:

        type: '[%parent.%parent.%type].third_party.[%key]'

Updated patch as per the changes suggested in #46, please review.

Now that we have a schema, the test fails as it should.

+++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
@@ -115,7 +115,7 @@ public function providerTestAccess() {
+          'first-uuid' => new SectionComponent('first-uuid', 'content', ['id' => 'foo'], ['harold' => 'maude'], ['layout_builder_defaults_test' => ['harold' => 'kumar']]),

This adds a TPS for which no schema exists.

The above needs a schema like

layout_builder.component.third_party.layout_builder_defaults_test:
  type: mapping
  mapping:
    harald:
      type: string
      label: Some arbitrary string.

+1 for #51, but that only addresses half of the test failure. (The schema needs added in layout_builder_defaults_test.schema.yml btw.)

You'll still get the following failure:

2) Drupal\Tests\layout_builder\Kernel\DefaultsSectionStorageTest::testAccess with data set "view, enabled, data" (true, 'view', true, array(Drupal\layout_builder\Section Object (...)))
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.entity_test.entity_test.default with the following errors: core.entity_view_display.entity_test.entity_test.default:third_party_settings.layout_builder.sections.0.components.first-uuid.third_party_settings._layout_builder missing schema

It's the result of how we're dealing with the $additional parameter in the SectionComponent constructor:

-  public function __construct($uuid, $region, array $configuration = [], array $additional = []) {
+  public function __construct($uuid, $region, array $configuration = [], array $additional = [], array $third_party_settings = []) {
     $this->uuid = $uuid;
     $this->region = $region;
     $this->configuration = $configuration;
     $this->additional = $additional;
+    if ($additional) {
+      $third_party_settings[$this->legacyAdditionalModuleKey] = $additional;
+    }
+    $this->thirdPartySettings = $third_party_settings;

When we pass a value as the $additional parameter, it gets added as a TPS with '_layout_builder' as the key.

My vote is that don't do this. We have no idea what's in $additional. If anyone is using $additional, then it's up to them to figure how to migrate data to TPS. That's the position we've taken with the Layout Builder Component Attributes module.

We also need to add a deprecation tag to the $additional property:

  /**
   * Any additional properties and values.
   *
+  * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.
+  *   Additional component properties should set via ::setThirdPartySetting().
+  *
+  * @see https://www.drupal.org/node/3100177
+  *
   * @var mixed[]
   */
  protected $additional = [];

I did not research how we want to handle BC of $additional, bu i suppose we have to keep it.
So let's do:

layout_builder.component.third_party._layout_builder:
  type: ignore
  label: 'Deprecated: Additional data'
  # TODO Add deprecated key once https://www.drupal.org/project/drupal/issues/2997100 lands.

and remove that chunk instead:

+++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
@@ -52,6 +52,11 @@ layout_builder.component:
     additional:
       type: ignore
       label: 'Additional data'

It's probably best that we leave the schema alone for 'additional'. I think the bigger issue is how the patch attempts to merge 'additional' into TPS with the '_layout_builder' provider key.

Let's assume we have a module that is currently using the 'additional' property. Currently, it writes its data using SectionComponent->set(), and it reads its data with SectionComponent->get(). Assume that there is data in 'additional' when we apply patch #48.

When we apply the patch, the module will no longer be able to retrieve its data. Before, the get() method would look in $this->additional for a given property. Now it will look in TPS using the '_layout_builder' provider key. It won't find any data because the data is still in 'additional' and we didn't perform any type of data migration from 'additional' to TPS. Patch #48 would break existing modules that are using 'additional'.

I think the better route to take would be to deprecate 'additional' and otherwise leave it alone. This means removing the $legacyAdditionalModuleKey private property and all of its uses.

I can follow the reasoning in #55 and it makes sense to me.

I'm going to re-roll #48 with the code changes discussed since then and also add test coverage based on section third-party settings tests.

Updated patch is attached, along with an interdiff. Everything since #48 should be incorporated.

I updated the deprecation notice because Coder wanted a particular syntax:

Coder error:

$ phpcs --standard=Drupal,DrupalPractice src/SectionComponent.php

FILE: ...-lb-component-tps/web/core/modules/layout_builder/src/SectionComponent.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 148 | ERROR | The trigger_error message 'Additional component properties
     |       | should set via ::setThirdPartySetting().' does not match the
     |       | relaxed standard format: %thing% is deprecated in
     |       | %deprecation-version% any free text %removal-version%.
     |       | %extra-info%. See %cr-link%
--------------------------------------------------------------------------------

Time: 133ms; Memory: 10MB

Code change:

- @trigger_error('Additional component properties should set via ::setThirdPartySetting().', E_USER_DEPRECATED);
+ @trigger_error('Setting additional properties is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Additional component properties should set via ::setThirdPartySetting(). See https://www.drupal.org/node/3100177', E_USER_DEPRECATED);

I think I'm okay with the removal of the attempted additional->TPS code. Unless that is needed to fix the constructor, then I'd consider it...

1. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -65,13 +80,16 @@ class SectionComponent {
   +  public function __construct($uuid, $region, array $configuration = [], array $additional = [], array $third_party_settings = []) {
   

   Is there a good way to swap the order here so that eventually $additional can be dropped?
   Other times it's been done via instanceof checks, but that's not possible here.

2. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -65,13 +80,16 @@ class SectionComponent {
        $this->additional = $additional;
   

   This should be wrapped in a !empty and get a trigger_error too

3. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -20,6 +21,51 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface;
   +  protected $component;
   ...
   +    $this->component = new SectionComponent(
   

   This is never reused outside the method.

4. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -20,6 +21,51 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface;
   +        'Chotchkie\'s' => [
   

   I'm all for fun test content, but can you pick something that doesn't need escaping? Makes every line it's on harder to read.

5. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -20,6 +21,51 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface;
   +    $this->section = new Section(
   +      'layout_onecol',
   +      [],
   +      [
   +        $this->component,
   +      ],
   +    );
   

   That's some excessive wrapping. Could probably just be a one-liner.

6. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -68,4 +114,147 @@ class SectionComponentTest extends UnitTestCase {
   +  public function providerTestGetThirdPartySettings() {
   ...
   +    $data[] = [
   ...
   +  public function providerTestGetThirdPartySetting() {
   ...
   +    $data[] = [
   

   Please add explanatory array keys for the data providers, explaining what each is testing.

I think I'm okay with the removal of the attempted additional->TPS code. Unless that is needed to fix the constructor, then I'd consider it

It's not needed to fix the constructor. The reason for removing the attempted additional->TPS code is because that code will break modules currently using $additional.

#59.1: Is there a good way to swap the order here so that eventually $additional can be dropped?
Other times it's been done via instanceof checks, but that's not possible here.

As long as we add $third_party_settings to the end of the constructor signature, we don't need a BC layer. We're designating $additional as deprecated. When D10 is released, the constructor signature will change (right?). Won't this be handled the same as The signature of the constructor of Drupal\file\FileUsage\DatabaseFileUsageBackend has changed. but without the BC piece?

D9 constructor:

  public function __construct($uuid, $region, array $configuration = [], array $additional = [], array $third_party_settings = []) {

D10 constructor:

  public function __construct($uuid, $region, array $configuration = [], array $third_party_settings = []) {

#59.6: Please add explanatory array keys for the data providers, explaining what each is testing.

Do you mean something like?

-   $data[] = [
+   $data['Verify non-existing key returns NULL'] = [
      'Chotchkie\'s',
      'non_existing_key',
      NULL,
    ];

(Adopting numbering based on my comment)

1)
There absolutely was a BC layer for DatabaseFileUsageConstructor. Compare the following:
https://git.drupalcode.org/project/drupal/-/blob/8.7.x/core/modules/file...
https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/modules/file...
https://git.drupalcode.org/project/drupal/-/blob/9.0.x/core/modules/file...

The second one (8.8.x) is what I was referring to when I said "Other times it's been done via instanceof checks" in #59.1

Whatever we do, the calling code in 9.1 has to be able to be the same as it is in both 9.0 and 10.0
$additional must be supported for all of 9.x, but there must be a way to write code that is valid for both.
What I wish we had done was go the route of a protected/private constructor, and used static factory methods.
Adding a ::create() method might be the only option to achieve this, but I'm open to ideas.

6)
Skip the "verify", but yes. Consider those keys to be like the one-liner if they were individual test methods (but not as full-sentence-y)

I understand the constructor issue now.

Adding a ::create() method might be the only option to achieve this, but I'm open to ideas.

I don't have a better solution. Is there an example out there in core to use for a reference for this use case?

\Drupal\update\ModuleVersion
\Drupal\update\ProjectSecurityData
\Drupal\update\ProjectSecurityRequirement

That's a trio recently added to core. Note the private constructors and the static create* methods.

If we wanted to switch to a private constructor, would that transition look something like this?

• Drupal 9.0 - no changes
• Drupal 9.1 - constructor is still public and static ::create() method is added; instantiating SectionComponent object directly is supported but deprecated
• Drupal 10.0 - constructor becomes private and static ::create() method becomes the only way to instantiate a SectionComponent object

A module that is compatible with both D9 and D10 would use the ::create() method.

Yep that's exactly what I was thinking, thanks for writing it out so clearly

The tricky part is how to trigger_error in 9.1 when the constructor is called directly. Idk the best way to handle that, either have ::create() set a private property to TRUE and then check that? Or inspect the backtrace 🤢

I'll roll code using a create method with a private property into the next patch. It seems like the least bad option.

If we wanted to stick with a public constructor, I believe it'd take us until D11 to completely wrap that up:

  // Drupal 9.0
  public function __construct($uuid, $region, array $configuration = [], array $additional = []) {
    $this->uuid = $uuid;
    $this->region = $region;
    $this->configuration = $configuration;
    $this->additional = $additional;
  }
      
  // Drupal 9.1
  public function __construct($uuid, $region, array $configuration = [], array $additional = [], array $third_party_settings = []) {
    $this->uuid = $uuid;
    $this->region = $region;
    $this->configuration = $configuration;
    $this->additional = $additional;
    if ($additional) {
      @trigger_error('Setting additional properties is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Additional component properties should set via ::setThirdPartySetting(). See https://www.drupal.org/node/XXX', E_USER_DEPRECATED);
    }
    $this->thirdPartySettings = $third_party_settings;
  }

  // Drupal 10.0
  public function __construct($uuid, $region, array $configuration = [], array $additional = [], array $third_party_settings = []) {
    $this->uuid = $uuid;
    $this->region = $region;
    $this->configuration = $configuration;
    if ($third_party_settings) {
      $this->thirdPartySettings = $third_party_settings;
      @trigger_error('Passing third party settings as the fourth argument to ' . __METHOD__ . ' is deprecated in drupal:10.0.0 and will throw a fatal error in drupal:11.0.0. Pass third party settings as the third argument. See https://www.drupal.org/node/XXX', E_USER_DEPRECATED);
      if ($additional) {
        // Do something here or just ignore $additional?
      }
    }
    elseif ($additional) {
      $this->thirdPartySettings = $additional;
    }
  }

  // Drupal 11.0
  public function __construct($uuid, $region, array $configuration = [], array $third_party_settings = []) {
    $this->uuid = $uuid;
    $this->region = $region;
    $this->configuration = $configuration;
    $this->thirdPartySettings = $third_party_settings;
  }

Updated patch, along with interdiff, is attached. All changes from #59 and #61 have been made.

I ended up going the backtrace route. The SectionComponent object needs to exist before setting the private property. At the point I can set the private property, it's too late to trigger a deprecation notice (in __construct()) because the object has already been created.

[Note: We don't have a follow-up issue, so I'm using https://www.drupal.org/node/123456 for now in @todo comments.]

Re the test results for #67, we're seeing a bunch of deprecation notices because I didn't replace new SectionComponent with SectionComponent::create outside of the existing work of this issue. I figured I see what feedback there is on the current approach first.

Looks good!

I'm a bit concerned though about the performance and memory footprint of this, given we may have lots of sections on a page.

The only alternative i can see is yet another temporary constructor argument named something like $isCalledViaCreateMethodInD9...

@geek-merlin - I share your concern. I was trying not to introduce another constructor argument; however, it may be the lesser evil. We'll be checking how the SectionComponent object was created for each component for a given Layout Builder-enabled entity.

Adding related issue: #3159712: SectionComponent should provide an unset() method. Modules using $additional can't clean up after themselves without an unset() method provided by SectionComponent. This will be needed once $additional is deprecated by this issue.

I tested the #67 patch with two modules. The first is Block Style Plugins which utilizes the new Third Party Settings and also Layout Builder Styles which utilizes the deprecated "additional" key. Both worked fine and both passed their tests on my local.

I would RTBC this, but I'm not sure if #68 needs to be addressed first.

@segovia94 - Thanks for testing and posting your findings. I should have an updated patch the takes care of the remaining tasks in the next 1-2 days.

Per #3159712: SectionComponent should provide an unset() method, we're not going to be adding an unset() method. This is already possible using a combination of get() and set(). This means the tools are there for an upgrade path from $additional to third party settings. Once $additional is removed, we'll no longer have a need for the get() and set() methods, so they can be deprecated, too.

Remaining tasks:

• Create a follow-up issue
• Replace 'https://www.drupal.org/node/123456' with follow-up issue URL in comments
• Replace all instances of new SectionComponent with SectionComponent::create
• Remove backtrace code and replace with additional constructor argument
• Deprecate get() and set() per #3159712: SectionComponent should provide an unset() method
• Update change record

Updated patch is attached.

A D10 follow-up issue has been created: #3160644: Remove deprecations and update constructor for \Drupal\layout_builder\SectionComponent.

At this point, everything is completed, except for updating the change record. I think we should wait for that step until the subsystem maintainers have weighed in.

Consider this sign-off as the relevant subsystem maintainer.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -436,7 +436,7 @@ class LayoutBuilderEntityViewDisplay extends BaseEntityViewDisplay implements La
   +      $new_component = (SectionComponent::create(\Drupal::service('uuid')->generate(), $region, $configuration));
   

   Nit: all of these leading parens were because of calling a method on the object created by new and that is no longer relevant and they should be removed.

2. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -309,12 +377,71 @@ class SectionComponent {
        return (new static(
   

   Regardless of the fact that this could be used even after it is private, let's switch this to explicitly use ::create() as well.

3. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -20,6 +21,37 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface;
    class SectionComponentTest extends UnitTestCase {
   

   One thing that is missing are explicit tests for each of the @trigger_error calls

Updated patch attached with the following change:

• 76.1 - The extra parentheses have been removed.
• 76.2 - Maybe handle this with a D10 comment? The :::create() method cannot accept the $additional parameter. Existing code that calls ::fromArray() and passes an 'additional' value would not execute as expected because ::create() cannot provide that data to the constructor.
• 76.3 - Test coverage is added.
• Added some additional comments that I missed earlier.

Rerolled patch addresses new committed code that's causing deprecation notices. It also adds a comment to address 76.2.

Per #3161300: Improve test coverage of \Drupal\Tests\layout_builder\Unit\SectionTest::testUnsetThirdPartySetting(), ::testUnsetThirdPartySetting() needs updated to run assertions on non-existing providers and non-existing keys. It also needs to use a data provider for consistency with other tests in its class.

Updated patch attached.

Updated patch with typo fix.

#82 worked for me along with one of my modules to support third party settings. Moving it to RTBC

Test failure was an intermittent and unrelated failure of Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest. Setting back to RTBC.

#82 does not apply on Drupal 9.0.x because 9.0 has no layout_builder_element_test module. Removing that part on patch makes it apply. Uploading the patch adapted to Drupal 9.0.x

Re #86, this issue is targeted for 9.1.x, not 9.0.x. New features cannot go in 9.0.x.

So current RTBC patch is #82.

Reuploading the actual patch from #82. Please don't queue patches against the non-development branch, it just derails the issue by taking it out of the RTBC queue.

+++ b/core/modules/layout_builder/src/SectionComponent.php
@@ -309,12 +382,75 @@ public function toArray() {
+    // Ensure expected array keys are present.
+    $component += [
+      'uuid' => '',
+      'region' => [],
+      'configuration' => [],
+      // @todo Remove below key/value when the drupal:10.0.x branch is opened.
+      // @see https://www.drupal.org/project/drupal/issues/3160644
+      'additional' => [],
+      'third_party_settings' => [],
+    ];
+    // @todo Use create() method when the drupal:10.0.x branch is opened.

So this is called heavily on deserializing from stored values in the DB right?

Do we have a plan for removing saved values in the additional key from stored overrides?

When we get to D10, will those just be silently ignored?

Other than that, I've got no other questions with this patch, and have been running it in production for some time.

So this is called heavily on deserializing from stored values in the DB right?

I'm not sure I follow. That code is part of the fromArray() method, which creates a SectionComponent object from an array.

Do we have a plan for removing saved values in the additional key from stored overrides?

When I was working on this patch, it was with the belief that anyone using 'additional' would be responsible for their own clean up.

I'm the maintainer of the Layout Builder Component Attributes module. All data stored in 'additional' is removed on uninstall: layout_builder_component_attributes.install.

When we get to D10, will those just be silently ignored?

That seems reasonable.

Note that \Drupal\layout_builder\Section::fromArray() has a similar += construct to the one added here.

This is called via \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplayStorage::mapFromStorageRecords() when reconstructing an entity view display out of the DB. Interestingly enough, we don't have any custom code that runs for the field-based values. \Drupal\layout_builder\Plugin\Field\FieldType\LayoutSectionItem::schema() specifies 'serialize' => TRUE, and that's enough. The only other relevant call is \Drupal\layout_builder\Field\LayoutSectionItemList::preSave(), which is a fromArray wrapped around a toArray call.

As far as cleaning up, I'm not sure we need to do more. I added a note to the D10 follow-up about removing the schema definition, but it should stay.

I think maybe we can do a little bit more to cleanup.

1. +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
   @@ -52,6 +52,11 @@ layout_builder.component:
        additional:
          type: ignore
          label: 'Additional data'
   

   We can deprecate this now. See https://www.drupal.org/node/3129881

2. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -65,13 +81,53 @@ class SectionComponent {
   +    // @todo Remove below conditional when the drupal:10.0.x branch is opened.
   +    // @see https://www.drupal.org/project/drupal/issues/3160644
   +    if (!$instantiated_by_create) {
   +      @trigger_error('Instantiating a SectionComponent object directly is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. SectionComponents should be instantiated using the SectionComponent::create() method instead. See https://www.drupal.org/node/3100177', E_USER_DEPRECATED);
   +    }
   

   This seems icky but pragmatic and will help custom and contrib update. Nice.

3. +++ b/core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
   @@ -211,6 +220,7 @@ third_party_settings:
                additional: {  }
   

   I think we should be removing this when it is empty so it doesn't end up in configuration unnecessarily. That way we'll produce config valid for Drupal 10 if additional is not used.

   Additionally we need an update path to remove this when it is empty. i.e. we should use the ConfigEntityUpdater to re-save them as this should remove the empty ones.

We were using a patch from this thread that disappeared completely:
https://www.drupal.org/files/issues/2020-11-20/3015152-tps-96.patch

... is it possible to get it back? :)

thanks

Your comment is #96. And the patch you're looking for is numbered 96...
And the file path is November 20th, but no one here commented then.
So I'm confused.

If you look at the Files section, you can show all files and look through the ones there? But yeah no idea

We added it to your project at 20'th of November. 30 days later it was deleted. Sounds like Drupal files cleanup. The patch is tied to comment and if you'd forget to actually comment then it gets deleted?

Orphaned files that have been uploaded, but are not related to any entity (e.g. the comment was not saved) or not marked as permenant in another way, will be automatically cleaned up and removed.

That behavior you can also experience in every Drupal installation.

+++ b/core/modules/layout_builder/src/SectionComponent.php
@@ -309,12 +382,75 @@ public function toArray() {
+  /**
+   * {@inheritdoc}
+   */
+  public function getThirdPartySetting($provider, $key, $default = NULL) {
+    return isset($this->thirdPartySettings[$provider][$key]) ? $this->thirdPartySettings[$provider][$key] : $default;
+  }

Not wanting to derail, but should this and the following functions not really be in a trait?

Given it it the exact same code as in:

https://www.drupal.org/project/drupal/issues/2942661

etc.

---

Overall +1 to the issue though

https://www.drupal.org/project/drupal/issues/3158694

1. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -65,13 +81,53 @@ class SectionComponent {
   +   * @param string $uuid
   ...
   +   * @param string $region
   ...
   +  public static function create($uuid, $region, array $configuration = [], array $third_party_settings = []) {
   

   uuid and region needs string types declared

2. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -309,12 +382,75 @@ public function toArray() {
   +      'uuid' => '',
   +      'region' => [],
   

   region is the string type (not array)

Addressing #102 and #94, but still missing update path suggested in #94. Hope I understood the remarks correctly.
Agree with #100 but suggest to make follow-up issue for addressing it once the trait is in.

As much as I appreciate the Office Space / TPS references, we have cspell now and the following words aren't in the dictionary:
Initech
Lumbergh
Waddams
Chotchkies

That's why it's failing tests.

Added words to the dictionary.txt file.

Looks like the added deprecatedkey is flooding those deprecation notices within the CI.
Besides that, unfortunately I forgot to add the link ("See https://...") into that deprecation message, this needs to be added (sorry :/).

I have no clue why the diff assertion for core.entity_view_display.node.article.full fails in Demo_umami.Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest, it shouldn't have been touched within this patch? I added the third_party_settings: { } to layout_builder_defaults_test/config/install/core.entity_view_display.entity_test.bundle_with_extra_fields.default.yml though.

Just want to get rid of the flooded deprecation notices first, can re-add it properly later if really needed. Just want to see what else might be wrong.

Replaced deprectated @expectedDeprecation annotation (the inception of deprecations...).

I guess the removal of the additional: { } entries in the config installation files need to be moved to an extra D10 port, because some tests check for config diffs. Reverted the removals in this patch now. Not sure how to handle the deprecated key in the config schema without breaking all the tests (see test results from #105). Alternatively, the config schema could get a @todo comment in the D9 patch.

Sorry for not being clear in #104: I meant "remove those strings" not "add them to the dictionary". But I'll leave that to a committer to decide about pushing back on.

I think the failure in #114 is because #3108503: Enable layout builder by default on all content types in Umami was only committed to 9.2, and now there's an additional config file to fix: core.entity_view_display.node.article.full

Thanks for the hint, trying to create the 9.2.x port of #114 (which stands for 9.1.x) with this patch now.

It would be helpful to have a more clear policy on how to decide what should be added into that dictionary and when it should be changed / replaced by existing words instead.

At the current state of this patch, there is no form handling regards TPS inlcuded (see ConfigureBlockFormBase::submitForm and its ::validateForm).
On entity forms, TPS can be included and may be saved without a custom submit handler (I guess due to "magical" entity build processing within EntityForm). Also no custom validation callback is required e.g. when using a simple select widget. Should SectionComponent instances also have a way of auto-including TPS form values?

Another thing that's kind of weird is that SectionComponent implements ThirdPartySettingsInterface from the Drupal\Core\Config\Entity namespace, but SectionComponent is not part of the entity system.

Hi @mxh re:#117 the patch in #116 works well for us so far and tests are green which might be RTBC+1 but from your points in #117 I guess we'd have to set this back to "Needs work" as you'd expect the "magic" to happen consistently for all areas?

What steps do you think have to be done and what decisions to be made by whom?

Hi @Anybody, I'd say a core maintainer needs to decide whether that concern stated in #117 should be addressed or not. Just noticed that the Section class already includes TPS officially in core, so it's probably not a problem to also include TPS for SectionComponent.

Thank you @mxh, so I will change status to Needs work. Do we have to add any tags for that decision?

ThirdPartySettingsInterface is used in 4 places in core, and only one of those is within the Entity system. It *is* definitely a misleading namespace, but I think it's reuse is more valuable.

Re-roll against 9.3.x

Re-roll against 9.3.0 (applies against 9.4.x and 10.0.x), I'm not sure how to roll an inter-diff on this since the patch in 124 doesn't apply against 9.3.0 anymore. I'm pretty sure that this is a correct re-roll, but would be happy to have someone else take a look (and get an inter diff working) to confirm.

@segovia94, @hswong3i and @Feng-Shui thank you for rerolling.

@tim.plunkett you set this back to "Needs review" in #123 - can we see this as reply to #120? If yes, who should have a look at this next? The missing integration is blocking several contrib modules from layout builder support and forces using workarounds, as the list of related issues shows.

Hope it's okay to bring this back to the attention for that reason.

Patch from #124 failed to apply on my 9.3.x, anyone has any other patch that works on 9.3.x?

@thiagomoraesp #128 is a reroll against 9.3, did you try it?

@Feng-Shui, the #128 didn't work too.
[EDIT]
Sorry, my mistake, it is passing on my 9.3.x, i was applying the same patch 2 times.

Added further contrib module not working correctly in LB due to this.

@tim.plunkett could you perhaps have a look at #129 => #120 (if you find the time)? I guess this needs core maintainer feedback about how to proceed?

@thiagomoraesp could you please clarify in #132 if it works now after correcting your mistake?

I think the change record could be more specific to how to accommodate third party on component blocks themself. Even if it is a copy/paste and rename of variables, it isn't clear where I need to adjust my custom code that let's me add CSS classes on any LB block.

There's also still quite a few references to $additional in the code base. And we should open a follow-up to component_library (if it doesn't exist), to swap out $additional for tps. Actually, this wouldn't need to be a follow-up. It could happen now and as we'll want to do a instanceof check on the SectionComponent object to see if it implements ThirdPartySettingsInterface and swap out additional for tps when constructing the section component.

+++ b/core/modules/layout_builder/src/SectionComponent.php
@@ -65,13 +81,53 @@ class SectionComponent {
+  public static function create(string $uuid, string $region, array $configuration = [], array $third_party_settings = []) {

I not fully convinced this will help with BC. We can deprecate on __construct if anything is passed to $additional. Then entirely remove $additional in Drupal 11. In PHP 8.1 we have named parameters and we can handle deprecation a lot easier then. And this is likely to be deprecated in Drupal 10 and sunset in Drupal 11 at this point. Can we evaluate another approach for deprecation here?

There might be some test failures here, but this converts back to the more traditional new SectionComponent() approach and updates things for removal in Drupal 11. There might be a few places I missed in cleaning up. Testbot will help here. But the revert did show a lack in fully testing TPS in that DefaultsSectionStorageTest::providerTestAccess was formatted incorrectly for the config schema validation.

I also updated the CR, so cleaning up tags.

Another benefit to removing the ::create() logic is this is a very reviewable issue now. Much smaller footprint. And the amount of BC adjustments needed should be much smaller for folks who weren't passing around TPS anyway. In some ways, that make it an easier adjustment for most people (no change for simple use cases). Rather than adding the new constructor mechanism and everyone would need to change their constructor. Some minor notes below from a formal review of the patch.

1. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -52,9 +53,24 @@ class SectionComponent {
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.
   

   Oops, I missed a version adjustment here.

2. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -99,6 +126,12 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.
   

   This version should also get updated.

3. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -309,12 +353,75 @@ public function toArray() {
   +    return isset($this->thirdPartySettings[$provider][$key]) ? $this->thirdPartySettings[$provider][$key] : $default;
   

   return $this->thirdPartySettings[$provider][$key] ?? $default;

4. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -309,12 +353,75 @@ public function toArray() {
   +    return isset($this->thirdPartySettings[$provider]) ? $this->thirdPartySettings[$provider] : [];
   

   return $this->thirdPartySettings[$provider] ?? [];

Last test failures were random js test failures. Otherwise, it was green. Here we fix a few self review nits (mentioned in #141).

This looks good, like the pivot away from ::create

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -67,6 +67,13 @@ function layout_builder_post_update_section_storage_context_mapping(&$sandbox =
   +function layout_builder_post_update_component_third_party_settings_schema() {
   +  // Empty post-update hook.
   +}
   

   shouldn't this load and resave any view display that is LB enabled so the new third_party_settings keys are added?

2. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -52,9 +53,24 @@ class SectionComponent {
   +   * @deprecated in drupal:9.4.0 and is removed from drupal:11.0.0.
   

   I think we can still target 10.0.0 for removals

3. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -52,9 +53,24 @@ class SectionComponent {
      protected $additional = [];
   

   Should we make this private, and implement __get and trigger a deprecation if someone accesses it?

4. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -99,6 +126,12 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
   +   * @deprecated in drupal:9.4.0 and is removed from drupal:11.0.0.
   +   *   Additional component properties should be gotten
   +   *   via ::setThirdPartySetting().
   +   *
   +   * @see https://www.drupal.org/node/3100177
   

   We will need methods for getConfiguration and getWeight then, as there's no way to get them without using get - if we intend to deprecate the whole function.

   If that's not the intention then we should remove this annotation as IDEs will mark the whole method as deprecated

5. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -120,6 +154,12 @@ public function get($property) {
   @@ -129,6 +169,7 @@ public function set($property, $value) {
   
   @@ -129,6 +169,7 @@ public function set($property, $value) {
        else {
          $this->additional[$property] = $value;
        }
   +    @trigger_error('Setting additional properties is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. Additional component properties should be set via ::setThirdPartySetting(). See https://www.drupal.org/node/3100177', E_USER_DEPRECATED);
   

   I think we can safely deprecate any calls to this method as there is setConfiguration and setWeight already, so there's no reason to use it if additional is deprecated

6. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -293,7 +334,10 @@ public function toArray() {
   +      // @todo Remove below key/value when the drupal:10.0.x branch is opened.
   

   This will go into 10.0.x first, so we probably want another branch/patch where that is already gone

7. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -20,6 +21,38 @@
   +        'Initech' => [
   +          'Bill Lumbergh' => 'TPS reports',
   +          'Milton Waddams' => 'Red Stapler',
   +        ],
   +        'Chotchkies' => [
   +          'flair' => TRUE,
   

   ;-)

8. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -68,4 +101,217 @@ public function testToRenderArray() {
   +  public function testGetThirdPartySettings($provider, $expected) {
   

   let's add a :void while we're here (and the other methods)

9. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
   @@ -68,4 +101,217 @@ public function testToRenderArray() {
   +   * Provides test data for ::testGetThirdPartySettings().
   

   I think we still expect a return-type hint and @return docs here (same for other methods)

10. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
    @@ -68,4 +101,217 @@ public function testToRenderArray() {
    +    }
    +    else {
    

    micronit: can return early and avoid else

Thanks for the feedback in #143.

143.1: done
143.2: done, but see my comments for 143.6.
143.3: I'm mainly worried about serialization and private properties biting us. See #3110266: Support serialization of private properties. I don't think there is a strong benefit to do this.
143:4: addressed. there were already getters. Just needed to make configuration's public.
143.5: fixed
143.6: before we do that, I recently had to switch everything in #3267870: Order image mappings by breakpoint ID and numeric multiplier to deprecated in 9.4 and removal in 11.0.0. Are we sure we want to remove already in Drupal 10.0.0?
143.7: nice
143.8: done
143.9: fixed
143.10: fixed

Behold, test coverage of the update path and a few other fixes.

This is looking great, nice to see it coming together

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -67,9 +68,58 @@ function layout_builder_post_update_section_storage_context_mapping(&$sandbox =
   +/**
   + * Clear caches due to config schema addition.
   + */
   +function layout_builder_post_update_component_third_party_settings_schema() {
   +  // Empty post-update hook.
   +}
   

   we don't need this anymore, now we have an actual post update hook, the caches will be cleared

2. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -52,9 +53,24 @@ class SectionComponent {
      protected $additional = [];
   

   Should we remove this and use something like \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait to trigger a deprecation on access to it? Or are we safe to assume that SectionComponent shouldn't be extended. Might be a question for @tim.plunkett

SectionComponent has no interface, it is a value object. Had usage of final and/or private been permitted at the time, we would have used that (final for the class, private for the property).

It is typehinted directly in several places, and is always created via new, with no opportunity to be subclassed.

So I think this is fine, and the deprecation trait would be overkill.

Happy to RTBC after the first point of #148 is addressed

Standardized on Drupal 11 deprecation removal. See #144 and 143.6. Happy to flip that to Drupal 10 if I'm misreading things.

I also addressed #148.1

Fantastic!

Re-queued 3015152-150.patch to check if #155 is an intermittent FunctionalJavascript failure:

Drupal\Tests\quickedit\FunctionalJavascript\LayoutBuilderQuickEditTest

fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-148.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.20 �[44m#StandWith�[0m�[43mUkraine�[0m

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\quickedit\FunctionalJavascript\LayoutBuilderQuickEditTest
F....                                                               5 / 5 (100%)

Time: 01:46.855, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\quickedit\FunctionalJavascript\LayoutBuilderQuickEditTest::testQuickEditIgnoresDuplicateFields
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'node/1/title/en/layout_builder-default-non_component-5426f50c51044a2daaab1ca8b2a4095a6c709a616f9aa8a171c75eb8e13178c1' => 'inactive'
+    'node/1/body/en/layout_builder-default-0-cd90a19a_81c4_4f4f_8f23_af63b2be7a3d-1-5426f50c51044a2daaab1ca8b2a4095a6c709a616f9aa8a171c75eb8e13178c1' => 'inactive'
 )

Any news on this?
It would be ok to have it on next release in order to extend functionalities.

Added reroll of patch #150 for Drupal 10.0.x, as this will go to Drupal 10 also.

#150 and #159 both fail with the following strange error:

Drupal\Tests\layout_builder\Functional\Update\LayoutBuilderSectionComponentUpdatePathTest::testRunUpdates
require(compress.zlib:///var/www/html/core/modules/layout_builder/tests/src/Functional/Update/../../../../../system/tests/fixtures/update/drupal-9.3.0.bare.standard.php.gz): Failed to open stream: operation failed

Added reroll of patch #150 on Drupal 9.5.x. as It's not getting applied.

Added reroll of patch #150 on Drupal 9.5.x. as It's not getting applied.

Please ignore patch #164, there was an extra file that got added by mistake.

@ravi.shankar can we get an interdiff? If this is just a reroll with no code content changes, we can set this back to RTBC as of #157 (@heddn)

Added reroll diff of patch #150 and #165.

Thanks, so I'm setting this RTBC again. Patch for Drupal 10 would also be useful again. Hopefully this will be committed soon so we don't need further rerolls :)

Thanks a lot @ravi.shankar!!

@hswong3i could you tell us the reason you created a separate identically named Merge Request with over 1000 changes, changed both MRs target branch to 9.4.x and created patches which do not apply to 9.4.x?

RTBC is still for #165.

@ravi.shankar sorry, reading the patch one more time I saw these lines:

@deprecated in drupal:9.4.0 and is removed from drupal:11.0.0.

This is just a textual issue, but should be drupal:9.5.0?

Or should this be updated to the matching version right before commit?

Leaving this RTBC for maintainers to decide, would be duplicate work if we have to update the versions again and again. So I'd vote to update the string right before commit.

If you decide to update it now, please provide an interdiff. Thanks! :)