- Issue created by @thejimbirch
- 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.
- Merge request !9801#3475115 Added config action for adding a component to a layout β (Open) created by penyaskito
- πͺπΈ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 forConfigEntityTypeInterface
implementations, so Navigation will need to declare its ownConfigAction
plugin.So this will only be useful as is for
LayoutBuilderEntityViewDisplay
in core, and of course contrib. - πͺπΈ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.
- πΊπΈ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'
- πͺπΈ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. - ππΊ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
about 1 month ago 11:12am 18 November 2024 - ππΊ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.
- πΊπΈ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.