Convert Layout plugin discovery to attributes

🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts Fixed 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.

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

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

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.

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 📌 CKEditor5PluginManager: use PHP attributes instead of doctrine annotations Needs work .

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.

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

Re #2 📌 Convert Layout plugin discovery to attributes Active :
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 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts Fixed .

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.

Made category required too.

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

Believe this is good.

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

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.

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 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts Fixed ?

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.

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

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

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.

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