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

  1. Add classes to represent the new Attributes - Example
  2. Update the plugin manager constructor to include both the attribute and annotation class names - example
  3. 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

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

longwave created an issue. See original summary.

larowlan’s picture

Thanks, 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

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

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

godotislate’s picture

#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:

The tricky part is this in Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations::parseClass():

    // Annotations use static reflection and are able to analyze a class that
    // extends classes or uses traits that do not exist. Attribute discovery
    // will trigger a fatal error with such classes, so only call it if the
    // class has a class attribute.
    if ($reflection_class->hasClassAttribute($this->pluginDefinitionAttributeName)) {
      return parent::parseClass($class, $fileinfo);
    }
   

hasClassAttribute returns TRUE if the Class .php file contains an attribute that matches $this->pluginDefinitionAttributeName, case-insensitive. This means that classes with the PageDisplayVariant attribute 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 of hasClassAttribute, 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.

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

godotislate’s picture

Per 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.

godotislate’s picture

Status: Active » Needs review

Added 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\FormElement can be deprecated and renamed \Drupal\Core\Render\Element\RenderElementBase and \Drupal\Core\Render\Element\FormElementBase? Have RenderElement and FormElement extend RenderElementBase and FormElementBase for now, then remove them altogether later?

godotislate’s picture

Issue summary: View changes
smustgrave’s picture

Left some small questions.

Do you think we need a follow up to remove "as CoreRenderElement;" from D12?

godotislate’s picture

Added declare strict.

Do you think we need a follow up to remove "as CoreRenderElement;" from D12?

I'd like to get some agreement about removing as CoreRenderElement and as CoreFormElement by replacing/deprecating the RenderElement and FormElement abstract classes with RenderElementBase and FormElementBase.

(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.

godotislate’s picture

Created 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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Sounds like a good plan to move to a follow up.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
use Drupal\Core\Render\Attribute\RenderElement;
use Drupal\Core\Render\Element\RenderElement as CoreRenderElement;

The 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

use Drupal\Core\Render\Attribute\RenderElement;
use Drupal\Core\Render\Element\RenderElement as RenderElementBase;

Same for FormElement were we are using use statement aliasing.

sorlov’s picture

Status: Needs work » Needs review

renamed

godotislate’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We're already doing #[ViewsArea("broken")] so let's make the change that @longwave suggests.

godotislate’s picture

Status: Needs work » Needs review

Updated to one line attributes

larowlan’s picture

Status: Needs review » Needs work

Left a comment/question on the MR

andypost’s picture

godotislate’s picture

Status: Needs work » Needs review

TrustedCallback attribute removed.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears all feedback has been addressed on this one.

alexpott’s picture

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

Committed and pushed d1c993acdf to 11.x and f6a07d869c to 10.3.x. Thanks!

  • alexpott committed f6a07d86 on 10.3.x
    Issue #3421439 by godotislate, sorlov, quietone, smustgrave, alexpott,...

  • alexpott committed d1c993ac on 11.x
    Issue #3421439 by godotislate, sorlov, quietone, smustgrave, alexpott,...

Status: Fixed » Closed (fixed)

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