Closed (fixed)
Project:
Drupal core
Version:
10.3.x-dev
Component:
base system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
13 Feb 2024 at 03:10 UTC
Updated:
18 May 2024 at 08:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
longwave#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.
Comment #5
mstrelan commentedSetting NW since there is an MR, but it's failing tests
Comment #7
quietone commentedThere are comments in \Drupal\Core\Layout\Annotation\Layout that have not been copied over. I reckon they should be.
Comment #8
quietone commentedI'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.
Comment #10
godotislateAdded 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.phplooks like this:In which case, arguments in attribute constructor would need to be changed to optional, since the deriver will be providing those properties instead.
Comment #11
godotislateMade 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.
Comment #12
needs-review-queue-bot commentedThe 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.
Comment #13
godotislateMade category required too.
Comment #14
smustgrave commentedI believe all feedback has been addressed on the MR. All 9 instances of @Layout have been replaced.
Believe this is good.
Comment #16
alexpottI made some improvements to the attribute param docs. I've just got one outstanding question on the MR about the use of ...$additional
Comment #17
alexpottOkay... 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.
Comment #18
godotislateThe 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 viaAttributeBridgeDecoratorinstead. I was assuming that existing test coverage for Layout discovery (mostly in LayoutPluginManagerTest) +Drupal\Tests\Component\Plugin\Discovery\AttributeBridgeDecoratorTestwas enough, unless there's something I'm missing.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?
Comment #19
alexpottThat 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.
Comment #20
godotislateMade label and category optional. Added tests that arbitrary properties are being set correctly via YML and PHP attributes.
Comment #21
larowlanI think this looks ready now, thanks everyone, applied one minor nit from my review
Comment #22
quietone commentedMany 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.
Comment #23
alexpottCommitted and pushed 091483c1f1 to 11.x and 36c7397511 to 10.4.x and fce131c9ae to 10.3.x. Thanks!