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\Render\Annotation\RenderElement and \Drupal\Core\Render\Annotation\FormElement plugins to use Attributes.
Proposed resolution
- Add classes to represent the new Attributes - Example
- Update the plugin manager constructor to include both the attribute and annotation class names - example
- Convert all plugins that use the annotations to use the new attributes - example
Remaining tasks
Work out how to handle subclassing, as FormElement extends RenderElement and we need to repeat the same pattern with attributes. Note that #3420984: Convert Layout DisplayVariant, PageDisplayVariant discovery to attributes also needs a solution for this. Done in #3427388: Update Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute() to allow attribute subclasses.
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3421439
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:
- 3421439-convert-form-and-render-element
changes, plain diff MR !6934
Comments
Comment #2
larowlanThanks, there might be others I missed when adding all the issues, I looked for annotations that extended from Plugin perhaps these don't and that's why I missed them
Comment #5
godotislate#3420984: Convert Layout DisplayVariant, PageDisplayVariant discovery to attributes is related to this one in that both need the same solution to handle subclassing. Per #4 there:
Comment #8
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 #9
godotislate#3427388: Update Drupal\Component\Annotation\Doctrine\StaticReflectionParser::hasClassAttribute() to allow attribute subclasses is merged, so this is unblocked.
Comment #10
godotislateAdded FormElement attribute, fixed some namespace issues, and updated some docblocks. Tests are now passing.
One thing to note: the DX having the attributes named RenderElement and FormElement while also often needing to extend plugin classes named RenderElement and FormElement is less than ideal. Maybe
\Drupal\Core\Render\Element\RenderElement\Drupal\Core\Render\Element\FormElementcan be deprecated and renamed\Drupal\Core\Render\Element\RenderElementBaseand\Drupal\Core\Render\Element\FormElementBase? Have RenderElement and FormElement extend RenderElementBase and FormElementBase for now, then remove them altogether later?Comment #11
godotislateComment #12
smustgrave commentedLeft some small questions.
Do you think we need a follow up to remove "as CoreRenderElement;" from D12?
Comment #13
godotislateAdded declare strict.
I'd like to get some agreement about removing
as CoreRenderElementandas CoreFormElementby replacing/deprecating the RenderElement and FormElement abstract classes withRenderElementBaseandFormElementBase.(I had a similar struggle extending the Constraint class for validation plugins, but that is a Symfony class, so ¯\_(ツ)_/¯.)
If we do think it's a good idea to deprecate, I think I'd prefer to do it in this issue, but that is scope expansion, so I'm fine doing it in a follow up.
Comment #14
godotislateCreated the follow up #3431452: Rename RenderElement and FormElement plugin abstract classes to RenderElementBase and FormElementBase so that the class renaming question isn't a blocker here.
Comment #15
smustgrave commentedSounds like a good plan to move to a follow up.
Comment #16
alexpottThe as here doesn't make much sense - both the attribute and the abstract base class are provided by core. Given the follow-up created I think we should change this to
Same for FormElement were we are using use statement aliasing.
Comment #17
sorlov commentedrenamed
Comment #18
godotislateI worked on this MR, but if the only outstanding thing was to fix the alias names, I am going to move this to RTBC, since @sorlov's changes address that.
Comment #19
alexpottWe're already doing
#[ViewsArea("broken")]so let's make the change that @longwave suggests.Comment #20
godotislateUpdated to one line attributes
Comment #21
larowlanLeft a comment/question on the MR
Comment #22
andypostComment #23
godotislateTrustedCallback attribute removed.
Comment #24
smustgrave commentedAppears all feedback has been addressed on this one.
Comment #25
alexpottCommitted and pushed d1c993acdf to 11.x and f6a07d869c to 10.3.x. Thanks!