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

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

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

larowlan created an issue. See original summary.

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

godotislate’s picture

Status: Active » Needs review
Issue tags: +Needs tests

This 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():

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

I'm not sure whether this is the ideal approach, so I have not added tests yet for hasImplementingClassAttribute(). If it is OK, then perhaps hasImplementingClassAttribute() even should replace hasClassAttribute altogether, since the latter method is not used anywhere else. Pushing to Needs Review though to get feedback.

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

andypost’s picture

Status: Needs review » Needs work

Needs work to research why outdated comment was added, and not sure about tests

godotislate’s picture

Status: Needs work » Needs review

Removed @todo comment that seems to no longer apply. Added test for hasImplementingClassAttribute().

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

godotislate’s picture

Issue summary: View changes
Status: Needs work » Needs review
godotislate’s picture

Discussed with @berdir on Slack that hasClassAttribute() can be re-implemented with is_a, so updated MR.

godotislate’s picture

Status: Needs review » Postponed

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

Issue summary: View changes
Status: Postponed » Needs review

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

smustgrave’s picture

Issue summary appears to be updated.

But was still tagged for tests so leaving in review for those.

godotislate’s picture

smustgrave’s picture

Could a follow up be opened for

Maybe there should be a follow-up issue about investigating usage of DisplayVariant vs PageDisplayVariant, and whether there even needs to be the two classes?

Then probably good to mark this one.

godotislate’s picture

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Going to mark it now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Post a review to the MR - I think we should require an admin label because these things have to be listed in the UI.

godotislate’s picture

Status: Needs work » Needs review

Changed admin_label to required and updated docblock.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears feedback from @alexpott has been addressed

alexpott’s picture

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

Committed and pushed 99fc430601 to 11.x and f267065695 to 10.3.x. Thanks!

  • alexpott committed f2670656 on 10.3.x
    Issue #3420984 by godotislate, andypost, smustgrave, larowlan, alexpott...

  • alexpott committed 99fc4306 on 11.x
    Issue #3420984 by godotislate, andypost, smustgrave, larowlan, alexpott...

Status: Fixed » Closed (fixed)

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