Problem/Motivation
The 'label' and 'category' keys are required.
This came up in #3392572: Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts and #3450760: Add missing 'label' key to navigation.layouts.yml. There was a thought to make the two keys required in the annotation to attribute conversion in #3420983: Convert Layout plugin discovery to attributes, per #2 and #17:
I think we should consider a BC layer where we add a label for unlabelled layouts and trigger a deprecation.
, but then it was kicked back to be done in #3392572: Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts, and finally decided to do a separate follow up, which is this issue.
The most direct way to address would be to make the Layout attribute label and category properties required, but some considerations:
- Are there uses cases where layout plugin derivers provide label or category?
- CategorizingPluginManagerTrait::processDefinitionCategory() provides a default category of the provider module name or machine name when the plugin definition is an array. Layout plugin definitions are objects, not arrays, but something similar can be implemented so that the same default category is assigned to the plugin definition when the plugin manager processes the definition
Steps to reproduce
Proposed resolution
- Add label to any current existing core layouts without one
- Trigger deprecation on any layouts that do not have a label
- In LayoutPluginManger:: processDefinition(), add a default category of the plugin provider (see CategorizingPluginManagerTrait::processDefinitionCategory())
- Investigate whether there are use cases for derivers providing plugin labels - There are. See #7.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3463592
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3463592-require-label-and
changes, plain diff MR !8947
Comments
Comment #2
godotislateComment #3
godotislateComment #5
godotislateAdded change record for deprecation.
Pushed a draft MR with the changes, will add tests later.
Comment #7
dcam commentedThe Dynamic Layouts module does. See https://git.drupalcode.org/project/dynamic_layouts/-/blob/8.x-1.x/src/Pl.... I know there are other similar modules out there too. I expect that any similar module would derive the label and possibly the category from user-entered data, which would then be added to the layout definition by the deriver.
Comment #8
dcam commentedI added tests. I think this one is ready for review.
Comment #9
smustgrave commentedSo this one is odd because one of the failing test only tests show
Reference https://git.drupalcode.org/issue/drupal-3463592/-/jobs/7224252
Which I know we aren't checking
Both scenarios do appear to be covered.
But the CR doesn't mention a default category is now being used, think it would be worth mentioning.
Comment #10
dcam commentedI updated the change record with information about the automatically-assigned categories.
Comment #11
smustgrave commentedThanks! Feedback appears to be addressed.
Comment #12
godotislateTy @dcam for picking up the tests. I'd forgotten about this issue.
Comment #13
longwaveSorry, too late for 11.3.0.
Do we really need a deprecation/enforcement here? Is the fallback good enough?
Comment #14
godotislateI think so. The fallback is for the category. The MR doesn't have a fallback for the label, and we're making both required.
Comment #15
dcam commentedThe deprecation versions have been updated to 11.4. Since this is a minor change I'm re-RTBCing it.
Comment #16
longwaveNew deprecations mean this lands in 11.x only and will be in 11.4.0.
Committed and pushed 95f7025b386 to 11.x. Thanks!