Problem/Motivation

In #3252386: Use PHP attributes instead of doctrine annotations we added support for attribute based plugin discovery.
As part of that issue we converted block and action plugins.

This issue is to convert \Drupal\Core\Layout\Annotation\Layout plugins to use Attributes.

Proposed resolution

  1. Add a class to represent the new Attribute - Example
  2. Update the plugin manager constructor to include both the attribute and annotation class names - example
  3. Convert all plugins that use the annotation to use the new attribute - example

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3420983-nr-bot.txt8.05 KBneeds-review-queue-bot

Issue fork drupal-3420983

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

larowlan created an issue. See original summary.

longwave’s picture

#3392572: Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts notes that the label and category properties are actually required, so we could make that change in the conversion to attributes.

Ruturaj Chaubey made their first commit to this issue’s fork.

mstrelan’s picture

Status: Active » Needs work

Setting NW since there is an MR, but it's failing tests

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

quietone’s picture

There are comments in \Drupal\Core\Layout\Annotation\Layout that have not been copied over. I reckon they should be.

quietone’s picture

I've been attempting to fix the phpstan errors.

The addition of context_definitions was made because it is used in the plugin definition in \Drupal\layout_builder_test\Plugin\Layout\TestContextAwareLayout.

kim.pepper made their first commit to this issue’s fork.

godotislate’s picture

Added a few comments to the MR. The plugin manager getDiscovery() needs to be updated to use attribute discovery classes, similar to the changes needed for #3399036: CKEditor5PluginManager: use PHP attributes instead of doctrine annotations.

Also, once that change is done, other changes might be needed for to pass tests. For example, at least one of the yaml-defined test layout plugins in web/core/tests/Drupal/Tests/Core/Layout/LayoutPluginManagerTest.php looks like this:

module_a_derived_layout:
  deriver: \Drupal\Tests\Core\Layout\LayoutDeriver
  array_based: true

In which case, arguments in attribute constructor would need to be changed to optional, since the deriver will be providing those properties instead.

godotislate’s picture

Status: Needs work » Needs review

Made the switch to Attribute discovery and got tests to all green.

Re #2:
The 'label' property is now required, but somewhere along the way category was set to optional. This might worth more consideration, depending on whether the label and category could be set by the deriver, with validation to completed in #3392572: Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new8.05 KB

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.

godotislate’s picture

Status: Needs work » Needs review

Made category required too.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

I believe all feedback has been addressed on the MR. All 9 instances of @Layout have been replaced.

Believe this is good.

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I made some improvements to the attribute param docs. I've just got one outstanding question on the MR about the use of ...$additional

alexpott’s picture

Status: Needs review » Needs work

Okay... do we have test coverage of step 3?

Also tests are failing due to a new layout in core. We need to consider the new Navigation module - that adds a layout without a label. I think we should consider a BC layer where we add a label for unlabelled layouts and trigger a deprecation.

godotislate’s picture

Okay... do we have test coverage of step 3?

The change here is that previously step 3 was converting all the plugin data back to Layout annotations via AnnotationBridgeDecorator, while now they're converted to Layout attributes via AttributeBridgeDecorator instead. I was assuming that existing test coverage for Layout discovery (mostly in LayoutPluginManagerTest) + Drupal\Tests\Component\Plugin\Discovery\AttributeBridgeDecoratorTest was enough, unless there's something I'm missing.

Also tests are failing due to a new layout in core. We need to consider the new Navigation module - that adds a layout without a label. I think we should consider a BC layer where we add a label for unlabelled layouts and trigger a deprecation.

Should we address that in this issue, or allow label and category to be optional for now and push requiring them back to #3392572: Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts?

alexpott’s picture

Should we address that in this issue, or allow label and category to be optional for now and push requiring them back to

That seems extremely reasonable... yeah let's make them optional for now. Maybe the category is not optional? But yeah let's do this work rather than more.

godotislate’s picture

Status: Needs work » Needs review

Made label and category optional. Added tests that arbitrary properties are being set correctly via YML and PHP attributes.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

I think this looks ready now, thanks everyone, applied one minor nit from my review

quietone’s picture

Many days ago I made a suggestion to the summary line in a test Layout plugin but did not submit it. I have just done so and then accepted my own suggestion. I did that because it was minor and in a test file.

alexpott’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 091483c1f1 to 11.x and 36c7397511 to 10.4.x and fce131c9ae to 10.3.x. Thanks!

  • alexpott committed fce131c9 on 10.3.x
    Issue #3420983 by sorlov, godotislate, alexpott, quietone, kim.pepper,...

  • alexpott committed 36c73975 on 10.4.x
    Issue #3420983 by sorlov, godotislate, alexpott, quietone, kim.pepper,...

  • alexpott committed 091483c1 on 11.x
    Issue #3420983 by sorlov, godotislate, alexpott, quietone, kim.pepper,...

  • alexpott committed 091483c1 on 11.0.x
    Issue #3420983 by sorlov, godotislate, alexpott, quietone, kim.pepper,...

Status: Fixed » Closed (fixed)

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