Problem/Motivation

I think there are some valid use cases where this could be helpful:

  • To be able to lock a section
  • To be able to hide/show a section for certain users
  • Exclude/Limit what blocks can be added
  • ...

Proposed resolution

Add third party settings to Sections

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

CommentFileSizeAuthor
#61 2942661-tps-61-interdiff.txt565 bytestim.plunkett
#61 2942661-tps-61.patch13.61 KBtim.plunkett
#58 2942661-tps-58-interdiff.txt15.13 KBtim.plunkett
#58 2942661-tps-58.patch13.06 KBtim.plunkett
#54 2942661-tps-54-interdiff.txt8.75 KBtim.plunkett
#54 2942661-tps-54.patch13.15 KBtim.plunkett
#47 2942661-47.patch18.18 KBtedbow
#47 interdiff-45-47.txt730 bytestedbow
#45 2942661-45.patch18.15 KBtedbow
#45 interdiff-43-45.txt3.63 KBtedbow
#43 2942661-43.patch16.25 KBtedbow
#43 2942661-43-test-update-FAIL.patch18.11 KBtedbow
#43 interdiff-31-43.txt1.9 KBtedbow
#32 interdiff-28-31.txt6.88 KBbkosborne
#31 interdiff-30-31.txt6.21 KBbkosborne
#31 interdiff-30-31.txt6.21 KBbkosborne
#31 2942661-31.patch15.23 KBbkosborne
#30 2942661-30.patch21.44 KBtedbow
#30 interdiff-28-30.txt6.88 KBtedbow
#28 interdiff-21-28.txt10.47 KBtedbow
#28 2942661-28.patch14.26 KBtedbow
#26 2942661-26.patch9.39 KBtedbow
#26 interdiff-21-26.txt10.47 KBtedbow
#21 2942661-21.patch9.23 KBtedbow
#21 interdiff-16-20.txt3.63 KBtedbow
#16 2942661-tps-16.patch5.54 KBtim.plunkett
#16 2942661-tps-16-interdiff.txt1.75 KBtim.plunkett
#13 2942661-tps-13.patch4.18 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

johndevman created an issue. See original summary.

johnwebdev’s picture

Issue summary: View changes
tim.plunkett’s picture

Component: layout.module » layout_builder.module
Adrian83’s picture

Could this allow a contrib module to provide the ability to add class select lists to a layout section, similar to what Classy Paragraphs or Entity Class Formatter modules do for entities?

(Of course, I'd like it in core, but that's asking pretty much!)

johnwebdev’s picture

@Adrian83

You should already be able to do that by providing your own Layout plugin. Take a look at https://www.drupal.org/project/layouts for some example code.

tim.plunkett’s picture

I'd largely agree that layout settings should be sufficient, except for the ideas presented in #2960384: Allow Concept of Section Wrappers

This might become a duplicate of that or a blocker for that.

dmsmidt’s picture

Please let me know if I grasp this right.
It seems that it is very hard to generically alter a sections config form, since all properties in a ConfigureSectionForm are protected. So a custom form submit function can't access for example delta.

Sure it is possible to define your own settings form per layout. But what if you want to alter all forms. Swapping the settings form class is possible via hook_layout_alter, but that is not something you want to do for all contributed layouts.

Does this issue address this? We are working on an AB-testing module integration with Layout Builder that precisely matches the second point in the IS (To be able to hide/show a section for certain users).

Anything to remark on this?

AaronMcHale’s picture

I can see use cases for having Third Party Settings, in the same way we have them for Entities, but then should the question be do we have third party settings for sections or third party settings for layouts, to me the latter makes more sense since when you define a layout you also define all of its base settings/configuration, and a section is basically just a layout.

tim.plunkett’s picture

Issue tags: +sprint
johnwebdev’s picture

The current issue description is not really why we might need this feature.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

This was determined to be a stable blocker.

tim.plunkett’s picture

Version: 8.6.x-dev » 8.7.x-dev
Issue summary: View changes
Issue tags: -Needs issue summary update +Needs tests
FileSize
4.18 KB

No idea if this will work, but here's a preliminary patch.
Needs tests.

tim.plunkett’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: 2942661-tps-13.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
5.54 KB

Fixing existing tests.

jibran’s picture

Title: Sections should have third party settings / settings » Sections should have third party settings

By looking at this patch I think we can remove / settings form the title now. Patch looks solid to me let's add test and get this in. Do we have any use-case in mind for this? Or is this just there so contrib can use this?

tim.plunkett’s picture

It's only for contrib, but there are already existing modules that need this.

Before Layout Builder, block plugins were only ever stored by block entities, and entities support third party settings.
Now that block plugins can be in sections, those modules need an equivalent place to store their settings.

bkosborne’s picture

I added #3015152: Support third party settings for components within a section which is to add third party settings support for components as well.

bkosborne’s picture

tedbow’s picture

rerolled and added unit test.

Status: Needs review » Needs work

The last submitted patch, 21: 2942661-21.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review

DrupalCi problem. Re-testing

tim.plunkett’s picture

Title: Sections should have third party settings » Sections and SectionComponents should have third party settings
Status: Needs review » Needs work

After discussion with someone (I think @tedbow?) I think we should merge #3015152: Support third party settings for components within a section into this issue and do it all at once.

Also that would make #2939928: Discuss renaming SectionComponent's additional to third_party_settings for consistency a duplicate as well.

If we were to do them separately, it might encourage storing data at the wrong level while waiting for the other half to land.

tim.plunkett’s picture

Fixing tags

tedbow’s picture

Status: Needs work » Needs review
FileSize
10.47 KB
9.39 KB

Here is the addition of component third party settings. Since the functionality and unit test is basically the same for Section and Components I made traits for both.

Status: Needs review » Needs work

The last submitted patch, 26: 2942661-26.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
14.26 KB
10.47 KB

whoops didn't actually add the trait files to the patch

tim.plunkett’s picture

Thanks!

  1. +++ b/core/modules/layout_builder/src/Section.php
    @@ -349,10 +357,18 @@ public function toArray() {
       public static function fromArray(array $section) {
    +    // Ensure expected array keys are present.
    
    +++ b/core/modules/layout_builder/src/SectionComponent.php
    @@ -319,11 +326,20 @@ public function toArray() {
    +    // Ensure expected array keys are present.
    +    $component += [
    

    Why is this needed now but wasn't before

  2. +++ b/core/modules/layout_builder/src/SectionComponent.php
    @@ -76,12 +79,15 @@ class SectionComponent {
    -  public function __construct($uuid, $region, array $configuration = [], array $additional = []) {
    +  public function __construct($uuid, $region, array $configuration = [], array $additional = [], array $third_party_settings = []) {
    

    Let's rename additional to TPS instead of adding it alongside

  3. +++ b/core/modules/layout_builder/tests/src/Unit/SectionComponentTest.php
    @@ -68,4 +70,11 @@ public function testToRenderArray() {
    +  private function getThirdPartySettingsObject() {
    
    +++ b/core/modules/layout_builder/tests/src/Unit/SectionTest.php
    @@ -179,4 +186,11 @@ protected function assertComponents(array $expected, Section $section) {
    +  private function getThirdPartySettingsObject() {
    
    +++ b/core/modules/layout_builder/tests/src/Unit/ThirdPartySettingsTestTrait.php
    @@ -0,0 +1,56 @@
    +  abstract protected function getThirdPartySettingsObject();
    

    Private vs protected

  4. +++ b/core/modules/layout_builder/tests/src/Unit/ThirdPartySettingsTestTrait.php
    @@ -0,0 +1,56 @@
    +   * @covers ::getThirdPartyProviders
    +   * @covers ::getThirdPartySetting
    +   * @covers ::getThirdPartySettings
    +   * @covers ::setThirdPartySetting
    +   * @covers ::unsetThirdPartySetting
    +   */
    +  public function testThirdPartySettings() {
    

    This could be split into 5 consecutive methods with @depends

  5. +++ b/core/modules/layout_builder/tests/src/Unit/ThirdPartySettingsTestTrait.php
    @@ -0,0 +1,56 @@
    +    $container = $this->getThirdPartySettingsObject();
    

    s/container/object, I did a double-take thinking this was a DI Container

tedbow’s picture

  1. Why is this needed now but wasn't before

    hmmmm. For Section.php his seems to have been added back #16 by a contributor named tim.plunkett. Unfortunately they only left the comment

    Fixing existing tests.

    😜

    I know that SectionDependenciesUpdatePathTest will fail without it

    So I assume it has to do with existing sections/components not having this key.

  2. This seem like this would be a BC break. $additional not keyed by module and right now \Drupal\layout_builder\SectionComponent::set supports any property key here.
  3. Fixed. but...
    You can't make an abstract method private but you change the visibility of an abstract method when implementing. My thought here is there would be no reason for a class extending one of the non-abstract class to change this method. But will just change all to protected.
  4. Fixed. yes much better.
  5. fixed
bkosborne’s picture

I think patch in #30 accidentally included unrelated patched code for node module. I removed it and provided an interdiff so you can see what I removed. I believe original interdiff in #30 is still accurate, so uploading it again for easier review.

bkosborne’s picture

FileSize
6.88 KB

Correct interdiff.

bkosborne’s picture

I'm testing this out by trying to make a practical use out of it in contrib: Adding a form field to the component add/update form that allows a site builder to add an arbitrary CSS class to the component.

So I think I need to do two things:

  • Alter the forms to add the form field for collecting CSS classes
  • Add a custom submit callback to the form that takes the value and stores it in TPS for the component object
  • Alter the component/block rendering to CSS class from from TPS and add it to container div

I think I already hit a roadblock though for the first step. If I form alter AddBlockForm, I can get access to the component object just fine because AddBlockForm::buildForm stores it in $form_state. However, UpdateBlockForm doesn't do this.

Alternatively, if I had access to the section storage, section delta, and component UUID, then I could easily get the component object. All of that data is stored in both AddBlockForm and UpdateBlockForm, but they are stored as protected properties so I cannot access it from form alters or custom form submit handlers.

I think a last ditch effort is to completely replace the form classes with my own that extend AddBlockForm and UpdateBlockForm, but that seems extreme?

What I'm describing may require a follow up issue, but I'd appreciate getting a maintainers opinion on my approach before I create one.

bkosborne’s picture

It looks like the problems I ran into were already mentioned in #7 above, but for sections and not components.

As I look into this even more, I'm confused about the additional property that already exists on components. What's it's purpose, and how does it differ from TPS I wonder?

tim.plunkett’s picture

See #29.2

bkosborne’s picture

Regarding #33, I opened #3023334: Layout Builder's block forms should expose its various protected properties.

I've created a proof of concept contrib module that makes use of the most recent patch from this issue: Layout Builder Styles. To get around the issue I mentioned in #33, I created replacements for LB's AddBlockForm and UpdateBlockForm.

tedbow’s picture

@bkosborne thanks testing this out in your module.

You can get the component for both edit and update forms like this.

/**
 * Implements hook_form_BASE_FORM_ID_alter().
 */
function mymodule_form_layout_builder_configure_block_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
  /** @var \Drupal\layout_builder\Form\ConfigureBlockFormBase $form_object */
  $form_object = $form_state->getFormObject();
  $component = $form_object->getCurrentComponent();
}

Let me know that works for your case.

bkosborne’s picture

Yep, that works. I was testing in 8.6.x initially and that was my problem.

segovia94’s picture

I'm a little unclear on #29.2 and #30.2. Will "additional" be deprecated at some point in favor or Third Party Settings? Or will it remain on for other uses besides just not breaking backwards compatibility? I am assuming that once this patch is applied we should be using TPS for any custom settings.

bkosborne’s picture

In #2939928: Discuss renaming SectionComponent's additional to third_party_settings for consistency it was decided to repurpose $additional and store it as TPS.

I think what's needed is an upgrade path that updates all section component configs, but it's a challenge because of the different way that they need to be stored. TPS has the concept of a module, key, and value. The $additional config property has just the concept of key and value it seems. Not sure on how that could work with an upgrade path.

I wonder how many people are actually making use of $additional.

Is it OK to make this a BC break?

tim.plunkett’s picture

for BC the update path could store all of additional under a fake module name like _layout_builder and make set() use that too, with @trigger_error.

segovia94’s picture

The patch in #31 as it currently stands is working well and passes all tests set up within my own module when I switched from additional. So I think the upgrade from additional is the only thing blocking this. I like the approach in #41.

tedbow’s picture

When I was looking at the $additional problem I found a problem with schema updates for third party settings. The validation wasn't get triggered at least in \Drupal\Tests\layout_builder\Kernel\DefaultsSectionStorageTest because it didn't have actually use third_party_settings in the test data.

Here are 2 patches. One that just updates the test to send the data to show it fails and one with the test and fix.

The last submitted patch, 43: 2942661-43-test-update-FAIL.patch, failed testing. View results

tedbow’s picture

  1. Changing to store additional in third_party_settings.
  2. +++ b/core/modules/layout_builder/src/SectionComponent.php
    @@ -76,12 +79,15 @@ class SectionComponent {
    -  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;
    

    Not sure what to do about the constructor.
    Re #29.2 since this would be a BC break.

    Could write and update path and then throw a deprecation warning if $additional is not empty?

    Are we free to change the constructor in 9.0? If not should we do it now before the module is stable?

Status: Needs review » Needs work

The last submitted patch, 45: 2942661-45.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
730 bytes
18.18 KB

Fixing the test. We don't need to set the _layout_builder key in third_party_settings in the constructor if is empty.

jidrone’s picture

I'm using Drupal 8.6.4 and the patch #47 cannot be applied.

jidrone’s picture

I was able to use it with 8.7.x, and also I provided a patch for block_class module #2998114: Integration with Drupal core's new Layout Builder.

Kristen Pol’s picture

Thanks for the patch. I reviewed as best I could and only noticed a possible casing change (below). As for testing this, other than trying one of the contrib module above, are there thoughts on testing this manually?

  • +++ b/core/modules/layout_builder/src/Section.php
    @@ -54,13 +58,16 @@ class Section {
    +    $this->third_party_settings = $third_party_settings;
    

    Use camel case for property?

  • +++ b/core/modules/layout_builder/src/SectionComponent.php
    --- /dev/null
    +++ b/core/modules/layout_builder/src/ThirdPartySettingsTrait.php
    

    Is this trait applicable to anything else?

  • +++ b/core/modules/layout_builder/src/ThirdPartySettingsTrait.php
    @@ -0,0 +1,66 @@
    +  protected $third_party_settings = [];
    

    Use camel case?

  • tim.plunkett’s picture

    Status: Needs review » Needs work

    NW for #50, as well as all the "Needs ____" tags

    tim.plunkett’s picture

    Okay so I know I was the one who combined (re-combined?) this issue and #3015152: Support third party settings for components within a section.
    But the BC implications of SectionComponent::$additional are stalling this issue.
    While not perfect, the $additional parameter does provide _some_ capabilities to contrib.

    What about re-splitting that issue out and focusing this stable blocking issue on giving Sections the ability to have additional data (via third party settings)?

    tedbow’s picture

    I agree re-opened the other issue

    tim.plunkett’s picture

    Title: Sections and SectionComponents should have third party settings » Sections should have third party settings
    Issue summary: View changes
    Status: Needs work » Needs review
    Issue tags: -Needs issue summary update, -Needs update path, -Needs update path tests
    FileSize
    13.15 KB
    8.75 KB

    Here's a new version of the patch with only the Section portion, as well as the fix to the config schema.

    phenaproxima’s picture

    1. +++ b/core/modules/layout_builder/src/Section.php
      @@ -22,7 +24,9 @@
      +  use ThirdPartySettingsTrait;
      

      I love that this is finally defined in a trait! Any chance of a follow-up to move it out of Layout Builder and use it in ConfigEntityBase?

      Actually, on second thought, is there any reason we're not just putting this directly in Drupal\Core somewhere? I ask because the code in the trait appears to be a direct copy n' paste from ConfigEntityBase, and therefore it might make more sense to have this trait in Drupal\Core from the get-go rather than have to deprecate the Layout Builder one.

      But, if we must keep it in Layout Builder for now, I'd still like to have a follow-up linked in the code.

    2. +++ b/core/modules/layout_builder/tests/modules/layout_builder_defaults_test/config/schema/layout_builder_defaults_test.schema.yml
      @@ -0,0 +1,5 @@
      +    which_party:
      +      type: string
      

      No label?

    phenaproxima’s picture

    Status: Needs review » Needs work
    Issue tags: +Needs tests

    Oh, one more thing -- is LayoutBuilderEntityViewDisplay going to take the sections' third-party settings into account when calculating config dependencies? Either way, we should probably have an explicit test to be sure that the calculated dependencies are what we expect them to be.

    Kristen Pol’s picture

    Since curious about manual testing and feedback in #50.

    I do see that #36 has a contrib module (Layout Builder Styles) but I'm wondering if there is some other way to manually test this.

    tim.plunkett’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs tests
    FileSize
    13.06 KB
    15.13 KB

    #50

    1) This was lower_snake_case due to other previous implementations. Let's fix as part of the next point

    2) Not currently, let's not abstract it early.

    3) Same as 1, fixed!

    #55
    1) Decided against putting this in as a trait, early abstractions are bad.

    2) Labels are optional for config schema, and this is a test. But I'll add one anyway.

    #57
    This is an API addition only. Other than the automated tests there are no consumers of this API in core. This means there is nothing to be manually tested.

    Interdiff is bigger than the patch, but it's there.

    tedbow’s picture

    Status: Needs review » Reviewed & tested by the community
    1. +++ b/core/modules/layout_builder/src/Section.php
      @@ -26,8 +26,6 @@
      -  use ThirdPartySettingsTrait;
      -
      

      Agreed we no longer need the trait.

    2. +++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
      @@ -103,9 +103,14 @@ public function testAccess($expected, $operation, $is_enabled, array $section_da
      +        ['layout_builder_defaults_test' => ['which_party' => 'third']]
      

      Very clever!

    Looks good! RTBC!

    xjm’s picture

    Status: Reviewed & tested by the community » Needs review
    +++ b/core/modules/layout_builder/tests/src/Unit/SectionTest.php
    @@ -179,4 +187,175 @@ protected function assertComponents(array $expected, Section $section) {
    +  public function providerTestGetThirdPartySettings() {
    +    $data = [];
    +    $data[] = [
    +      'bad_judgement',
    +      ['blink_speed' => 'fast', 'spin_direction' => 'clockwise'],
    +    ];
    +    $data[] = [
    +      'hunt_and_peck',
    +      ['delay' => '300ms'],
    +    ];
    +    $data[] = [
    +      'non_existing_provider',
    +      [],
    +    ];
    +    return $data;
    

    Was this Ted? :P

    This looks like a good improvement to me. Do we need any kind of upgrade path?

    tim.plunkett’s picture

    That was 100% Grade-A Ted :)

    The config schema expansion deserves a post_update hook to clear caches, but the rest is just adding a new thing, not migrating anything to it or adjusting anything else.

    tedbow’s picture

    Status: Needs review » Reviewed & tested by the community

    re #60

    yep this is just different test cases based on the Section that was created in setUp() with some attempts at humor included.

    If you would like to learn about the hunt_and_peck module https://www.drupal.org/project/hunt_and_peck
    But I am afraid isn't much documentation there and all the commit messages are emojis, 🎉. So fun but not super helpful.

    #61 looks good!

    xjm’s picture

    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs change record

    Needs change record though?

    phenaproxima’s picture

    Status: Needs work » Reviewed & tested by the community
    Issue tags: -Needs change record
    tim.plunkett’s picture

    Thanks @phenaproxima! RTBC indeed.

    xjm’s picture

    Title: Sections should have third party settings » Sections should have third-party settings
    +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
    @@ -28,6 +28,11 @@ layout_builder.section:
    +      label: 'Third party settings'
    

    Technically "third-party settings" is grammatically correct; they are settings from third parties, not the third settings from parties. But this label is wrong throughout core already, so out of scope.

    xjm’s picture

    Saving issue credit.

    • xjm committed d8b8a0a on 8.7.x
      Issue #2942661 by tedbow, tim.plunkett, bkosborne, johndevman, xjm,...
    xjm’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed to 8.7.x and published the change record. Thanks!

    Status: Fixed » Closed (fixed)

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

    dsnopek’s picture

    Third-party settings on Sections are great, but less useful for contrib than they could be, because there is no way to alter how the section as a whole is rendered (only it's individual components, because there is an event to alter that for SectionComponent).

    I made a new issue to allow altering the Section's rendering too:

    #3062862: Third party should be allowed to alter the section render array

    geek-merlin’s picture