Provide a Config Action to add/edit a component in a layout

Created on 18 September 2024, 9 months ago

Problem/Motivation

There is currently no way to add or alter items in a layout. We should have a config action that allows recipe to add or edit items in a layout.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

โœจ Feature request
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component
Layout builderย  โ†’

Last updated 4 days ago

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @thejimbirch
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone
  • First commit to issue fork.
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    Thought that this could fit in SectionListTrait, but timplunkett pointed out that this would only make sense in config based section lists, so will be its own trait.

  • Pipeline finished with Failed
    8 months ago
    Total: 110s
    #306122
  • Pipeline finished with Success
    8 months ago
    Total: 546s
    #306319
  • ๐Ÿ‡ช๐Ÿ‡ธSpain plopesc Valladolid

    Great step forward @penyaskito. Really looking forward to have this in core!

    Tried to test it locally to integrate it with navigation module with no luck. Probably I missed one or more steps.

    Steps I followed:

    • Clone the branch
    • Add use ConfigSectionListTrait; to NavigationSectionListStorage
    • Implement uuidGenerator() method
    • Clear cache at least once
    • Execute ddev . php core/scripts/drupal recipe modules/my_recipe
    • Get the error: The "addComponent" plugin does not exist. Valid plugin IDs for Drupal\Core\Config\Action\ConfigActionManager are: ....

    Create a recipe.yml like this:

    name: Navigation Test Recipe
    type: Examples
    description: 'Adds a block to navigation.'
    install:
      - navigation
    config:
      actions:
        navigation.block_layout:
          addComponent:
            section: 0
            position: 1
            value:
              uuid: 30000000-0000-1000-a000-000000000000
              id: 'navigation_menu:content'
              label: Content From Recipe
              label_display: 1
              provider: navigation
              region:
                content: 'content'
              default_region: content
              level: 1
              depth: 2

    Could you please indicate the steps I missed?

    Thank you!

  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    Apparently I was wrong and looking at \Drupal\Core\Config\Action\Plugin\ConfigAction\Deriver\EntityMethodDeriver::getDerivativeDefinitions, ActionMethods are only valid for ConfigEntityTypeInterface implementations, so Navigation will need to declare its own ConfigAction plugin.

    So this will only be useful as is for LayoutBuilderEntityViewDisplay in core, and of course contrib.

  • Pipeline finished with Failed
    8 months ago
    Total: 558s
    #312398
  • Pipeline finished with Failed
    8 months ago
    Total: 724s
    #312405
  • Pipeline finished with Failed
    8 months ago
    Total: 187s
    #312433
  • Pipeline finished with Failed
    8 months ago
    Total: 139s
    #312435
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    MR is ready for review. We might want more test coverage for the weight recalculation, but I'd prefer to get feedback on the direction.
    I'm not sure how are we testing config actions. Ideally we would have test recipes instead of just testing the method.

    We might want to implement this in LayoutBuilderEntityViewDisplay, but not sure if part of the same MR.

  • Pipeline finished with Canceled
    8 months ago
    Total: 166s
    #312464
  • Pipeline finished with Canceled
    8 months ago
    Total: 173s
    #312466
  • Pipeline finished with Canceled
    8 months ago
    Total: 78s
    #312472
  • Pipeline finished with Failed
    8 months ago
    Total: 230s
    #312473
  • Pipeline finished with Failed
    8 months ago
    Total: 511s
    #312481
  • Pipeline finished with Failed
    8 months ago
    Total: 385s
    #312494
  • Pipeline finished with Canceled
    8 months ago
    Total: 230s
    #312512
  • Pipeline finished with Failed
    8 months ago
    Total: 398s
    #312515
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Drupal CMS is going to need this, tagging accordingly.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Kicking back for addressing my remaining feedback. :) But I think this is a great start on a helpful action, and I'd really like to see it in core.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    Didn't remove the changed I had before yet.

    This means that setSection needs to change its visibility from protected to public in SectionListTrait.

    Not sure if I implemented the deriver right, but this worked for me with a slightly different version of the previous recipe:

    config:
      actions:
        dashboard.dashboard.test:
          addComponent:
            section: 0
            position: 4
            component:
              uuid: 01929a0b-056f-72c1-b390-c9bdc3f6fd5e
              id: dashboard_text_block
              label: 'My new dashboard with weight from a plugin'
              label_display: 'visible'
              provider: 'dashboard'
              region:
                layout_twocol_section: 'second'
              default_region: content
              context_mapping: { }
              text:
                value: '<p>My new dashboard with weight from plugin</p>'
                format: 'basic_html'
    
    
  • Pipeline finished with Failed
    8 months ago
    Total: 185s
    #313107
  • Pipeline finished with Failed
    7 months ago
    Total: 159s
    #337828
  • Pipeline finished with Failed
    7 months ago
    Total: 242s
    #337841
  • Pipeline finished with Failed
    7 months ago
    Total: 233s
    #337879
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    This needs better test coverage.

    We only cover adding new components. I'd expect we want a separate action for adding multiple components, and a separate one of editing existing components, as @phenaproxima said it would be better to have them separated.
    Not sure if we should cover these here or in follow-ups.

  • Pipeline finished with Failed
    7 months ago
    Total: 213s
    #337896
  • Pipeline finished with Failed
    7 months ago
    Total: 148s
    #337903
  • ๐Ÿ‡ญ๐Ÿ‡บHungary Gรกbor Hojtsy Hungary

    The MR only seems to add the "add layout component" action, so I think its far that it only has tests for that. @phenaproxima indicated that these improvements to the tests would be good, that still looks like needs work:

    • We need to be sure the region stuff works as expected, including when the default region is used, or the region can't be resolved.
    • We need to be sure the position stuff is figured out correctly, every way it could be passed in.
    • We need to be sure that the additonal key is preserved if passed.
    • The test...isn't using config actions. That obviates the entire point of this MR. We need to test that we can accomplish the addition of the component using config actions. (We don't need to use a recipe, but we probably need ConfigActionManager::applyAction(...).
  • Status changed to Needs work 7 months ago
  • ๐Ÿ‡ญ๐Ÿ‡บHungary Gรกbor Hojtsy Hungary

    Reached out to phpstan folks. Learned that ๐Ÿ› Add missing @return types for StringTranslationTrait::formatPlural() and ::getNumberOfPlurals() Active is in the way, until that lands, the phpstan baseline needs updating in the MR.

  • Pipeline finished with Success
    7 months ago
    Total: 2191s
    #343070
  • Pipeline finished with Success
    7 months ago
    Total: 580s
    #343117
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia pameeela
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Discussed with @penyaskito and we agreed that this is nice to have, and would accelerate Drupal CMS's dashboard work track, but is not a stable blocker.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia pameeela

    Tagging as a target so it's on the radar somewhere, it's intended to be used for nice-to-haves.

  • Pipeline finished with Failed
    5 months ago
    Total: 601s
    #376621
  • Pipeline finished with Failed
    5 months ago
    Total: 108s
    #376652
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    Added moar test coverage as requested.

    The only remaining comments are about reusing the existing array, but don't think creating a new one and copying every key value (as we don't know the specific block schema) would make it more clear.

  • Pipeline finished with Failed
    5 months ago
    Total: 519s
    #376686
  • Pipeline finished with Success
    5 months ago
    Total: 2484s
    #377785
  • ๐Ÿ‡ญ๐Ÿ‡บHungary Gรกbor Hojtsy Hungary

    The added test coverage looks good IMHO. This part needs a change record I think?

    diff --git a/core/modules/layout_builder/src/SectionListTrait.php b/core/modules/layout_builder/src/SectionListTrait.php
    index 7a049dec6c40cabdf30343afedd0477d81e4786d..b55fe12ca5432871741e3b26e116d8634cd11148 100644
    --- a/core/modules/layout_builder/src/SectionListTrait.php
    +++ b/core/modules/layout_builder/src/SectionListTrait.php
    @@ -54,7 +54,7 @@ public function getSection($delta) {
        *
        * @return $this
        */
    -  protected function setSection($delta, Section $section) {
    +  public function setSection($delta, Section $section) {
         $sections = $this->getSections();
         $sections[$delta] = $section;
         $this->setSections($sections);
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts

    Is it possible for 2 change records? We have been adding them for new and altered config actions. Something like:

    Recipes can now add a component in a Layout Builder layout using the addComponent config action

  • The Needs Review Queue Bot โ†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Rajab Natshah Jordan

    rajab natshah โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    4 months ago
    Total: 127s
    #420875
  • Pipeline finished with Canceled
    2 months ago
    Total: 82s
    #458695
  • Pipeline finished with Success
    2 months ago
    Total: 648s
    #458696
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    This should be ready, we still need the change record for making public setSections.

  • Pipeline finished with Failed
    2 months ago
    Total: 411s
    #459064
  • Pipeline finished with Failed
    2 months ago
    Total: 592s
    #459092
  • Pipeline finished with Success
    2 months ago
    Total: 3404s
    #459183
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ
  • Assigned to penyaskito
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ
  • Pipeline finished with Success
    about 2 months ago
    Total: 584s
    #471024
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    Back to NR.

  • Pipeline finished with Success
    about 2 months ago
    Total: 1610s
    #471060
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Looks great. I have basically no complaints about the action itself. My only outstanding issues:

    • Needs two change records before I can RTBC: one to document the action itself, the other to mention the publicizing of setSection.
    • The test is a bit repetitive and could be refactored for clarity by using a data provider.

    I had a couple of minor nitpicks as well. But this doesn't feel far off to me.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts

    Draft change record for the action exists: https://www.drupal.org/node/3519491 โ†’

    Can you add a little more detail with what your want with "the other to mention the publicizing of setSection."? I can try to add it.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States thejimbirch Cape Cod, Massachusetts
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Pretty simple - I think we just want to mention that \Drupal\layout_builder\SectionListTrait::setSection() is now a public method and will be available on every class that uses SectionListTrait.

  • Pipeline finished with Success
    about 2 months ago
    Total: 423s
    #475099
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 176s
    #475162
  • Pipeline finished with Failed
    about 2 months ago
    Total: 372s
    #475165
  • Pipeline finished with Failed
    about 2 months ago
    Total: 136s
    #475182
  • Pipeline finished with Failed
    30 days ago
    Total: 292s
    #488516
  • Pipeline finished with Failed
    30 days ago
    Total: 247s
    #488548
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    Fixed change record and IS with a (valid and tested) example.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    I'd hope to have this on 11.2 for any further development around providing/adjusting dashboards in Drupal CMS for the next months.

  • Pipeline finished with Failed
    30 days ago
    Total: 1697s
    #488551
  • Pipeline finished with Success
    30 days ago
    #488561
  • Pipeline finished with Success
    30 days ago
    Total: 3977s
    #488571
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    This looks great to me. There are a couple of minor gaps here, such as:

    Testing that a preassigned UUID is preserved.
    Testing that the action only works with config entities that implement SectionListInterface, and is not found for any other entity type (i.e., coverage of the deriver).

    Since I agree that we'd really like this in 11.2, I think it is okay to add that coverage in a follow-up. Tagging for that, but otherwise I say we should ship this.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Looking good, asked a question of the maintainers about the API addition.
    It would be good to add some docs to the action. We have some example YML in comments above - something like that would be really valuable.

  • Pipeline finished with Failed
    29 days ago
    Total: 815s
    #489692
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    #36 โœจ Provide a Config Action to add/edit a component in a layout Active Created the test for the first point, added follow-up ๐Ÿ“Œ [PP-1] Add unit test for Drupal\layout_builder\Plugin\ConfigAction\Deriver\AddComponentDeriver Active for the second one.

    Added phpdoc with example and docs for the config action.

  • Pipeline finished with Failed
    29 days ago
    Total: 192s
    #489719
  • Pipeline finished with Failed
    29 days ago
    Total: 270s
    #489723
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ
  • Pipeline finished with Failed
    29 days ago
    Total: 211s
    #489725
  • Pipeline finished with Success
    28 days ago
    Total: 548s
    #489728
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand danielveza Brisbane, AU

    MR is looking good. I've done a couple of code reviews and left a few minor comments. re: #37 I'm ok with the API change as outlined in the MR, but I'll leave the tag for now just in the case the other LB maintainers want to weigh in.

  • Pipeline finished with Failed
    28 days ago
    Total: 493s
    #489755
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States phenaproxima Massachusetts

    Couple minor points but I'm happy with this. If the subsystem maintainer signs off, ship it. Call this a soft RTBC.

  • Pipeline finished with Canceled
    28 days ago
    Total: 192s
    #490600
  • Pipeline finished with Failed
    28 days ago
    Total: 502s
    #490605
  • Pipeline finished with Failed
    28 days ago
    Total: 267s
    #490714
  • Pipeline finished with Failed
    28 days ago
    Total: 125s
    #490716
  • Pipeline finished with Failed
    28 days ago
    Total: 157s
    #490719
  • ๐Ÿ‡ช๐Ÿ‡ธSpain penyaskito Seville ๐Ÿ’ƒ, Spain ๐Ÿ‡ช๐Ÿ‡ธ, UTC+2 ๐Ÿ‡ช๐Ÿ‡บ

    All feedback resolved. Only detail is confirming that we're fine leaving ๐Ÿ“Œ `\Drupal\layout_builder\Section::setComponent` should verify the region in that layout exists Active as a follow-up, as I don't think we should do that in the action itself.

  • Pipeline finished with Success
    28 days ago
    Total: 360s
    #490726
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Credits

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Didn't mean to change status

    • larowlan โ†’ committed 66d8b1b5 on 11.x
      Issue #3475115 by penyaskito, gรกbor hojtsy, thejimbirch, phenaproxima,...
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    Committed to 11.x
    Published both of the change records
    Thanks all

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

Production build 0.71.5 2024