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\Display\Annotation\DisplayVariant and \Drupal\Core\Display\Annotation\PageDisplayVariantplugins to use Attributes.
Proposed resolution
- Add a class to represent the new Attribute - Example
- Update the plugin manager constructor to include both the attribute and annotation class names - example
- Convert all plugins that use the annotation to use the new attribute - example
Also need to work out how to handle subclassing, as PageDisplayVariant extends DisplayVariant and we need to repeat the same pattern with attributes. Note that #3421439: Convert form and render element plugin discovery to attributes also needs a solution for this. This was done in #3427388: Update Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute() to allow attribute subclasses.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3420984
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:
- 3420984-convert-layout-displayvariant
changes, plain diff MR !6818
Comments
Comment #4
godotislateThis is similar to #3421439: Convert form and render element plugin discovery to attributes in that PageDisplayVariant extends DisplayVariant here (FormElement extending RenderElement in the other issue), and the same instance of VariantManager gets all the definitions for both types of plugins together.
The tricky part is this in Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations::parseClass():
hasClassAttributereturns TRUE if the Class .php file contains an attribute that matches$this->pluginDefinitionAttributeName, case-insensitive. This means that classes with thePageDisplayVariantattribute fail this check, and the class is not picked up as a plugin.The approach in the MR is to add a
hasImplementingClassAttribute()method to use here instead ofhasClassAttribute, and instead of a case-insensitive exact string match,is_a()is used to check that the attribute class is the same class or subclass of$this->pluginDefinitionAttributeName.I'm not sure whether this is the ideal approach, so I have not added tests yet for
hasImplementingClassAttribute(). If it is OK, then perhapshasImplementingClassAttribute()even should replacehasClassAttributealtogether, since the latter method is not used anywhere else. Pushing to Needs Review though to get feedback.Comment #6
andypostNeeds work to research why outdated comment was added, and not sure about tests
Comment #7
godotislateRemoved @todo comment that seems to no longer apply. Added test for
hasImplementingClassAttribute().Comment #8
smustgrave commentedSince this appears to be doing more then converting to attributes can the issue summary be updated to include what's being done/why it's needed in this ticket.
Comment #9
godotislateComment #10
godotislateDiscussed with @berdir on Slack that
hasClassAttribute()can be re-implemented withis_a, so updated MR.Comment #11
godotislatePer suggestion from @alexpott on Slack, handling the StaticReflectionClass changes by themselves #3427388: Update Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute() to allow attribute subclasses. Blocking this on that issue.
Comment #12
godotislate#3427388: Update Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute() to allow attribute subclasses was merged, so this is unblocked now. Rebased and also updated some docblocks referencing the annotation.
Comment #13
smustgrave commentedIssue summary appears to be updated.
But was still tagged for tests so leaving in review for those.
Comment #14
godotislateTests were done in #3427388: Update Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute() to allow attribute subclasses, so removing tag.
Comment #15
smustgrave commentedCould a follow up be opened for
Then probably good to mark this one.
Comment #16
godotislateCreated follow up: #3431447: Consolidate layout DisplayVariant, PageDisplayVariant plugin types
Comment #17
smustgrave commentedThanks! Going to mark it now.
Comment #18
alexpottPost a review to the MR - I think we should require an admin label because these things have to be listed in the UI.
Comment #19
godotislateChanged admin_label to required and updated docblock.
Comment #20
smustgrave commentedAppears feedback from @alexpott has been addressed
Comment #21
alexpottCommitted and pushed 99fc430601 to 11.x and f267065695 to 10.3.x. Thanks!