Problem/Motivation

Per How to register layouts:

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

Command icon 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:

Comments

godotislate created an issue. See original summary.

godotislate’s picture

Title: Make label and category properties for layout plugins » Require label and category properties for layout plugins
godotislate’s picture

Issue summary: View changes

godotislate’s picture

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

Added change record for deprecation.
Pushed a draft MR with the changes, will add tests later.

dcam made their first commit to this issue’s fork.

dcam’s picture

Are there uses cases where layout plugin derivers provide label or category?

The 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.

dcam’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

I added tests. I think this one is ready for review.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

So this one is odd because one of the failing test only tests show

Using @Layout annotation for plugin with ID plugin_provided_by_annotation_layout is deprecated and is removed from drupal:13.0.0. Use a Drupal\Core\Layout\Attribute\Layout attribute instead. See https://www.drupal.org/node/3395575

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.

dcam’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

I updated the change record with information about the automatically-assigned categories.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Feedback appears to be addressed.

godotislate’s picture

Ty @dcam for picking up the tests. I'd forgotten about this issue.

longwave’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, too late for 11.3.0.

Do we really need a deprecation/enforcement here? Is the fallback good enough?

godotislate’s picture

Do we really need a deprecation/enforcement here? Is the fallback good enough?

I think so. The fallback is for the category. The MR doesn't have a fallback for the label, and we're making both required.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

The deprecation versions have been updated to 11.4. Since this is a minor change I'm re-RTBCing it.

longwave’s picture

Status: Reviewed & tested by the community » Fixed

New deprecations mean this lands in 11.x only and will be in 11.4.0.

Committed and pushed 95f7025b386 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed 95f7025b on 11.x
    fix: #3463592 Require label and category properties for layout plugins...

Status: Fixed » Closed (fixed)

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