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\layout_builder\Annotation\SectionStorageplugins 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

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3421019

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

DanielVeza’s picture

DanielVeza
he/him
Location Brisbane, AU
CreditAttribution: DanielVeza as a volunteer and at PreviousNext commented
Assigned: Unassigned » DanielVeza

DanielVeza opened merge request !6593

DanielVeza’s picture

DanielVeza
he/him
Location Brisbane, AU
CreditAttribution: DanielVeza as a volunteer and at PreviousNext commented
Status: Active » Needs review

I've set up the SectionStorage Attribute and converted all existing plugins over to the Attribute. Tests green, Ready for review

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Needs work

For the open threads. Leaving currently assigned assuming @DanielVeza you wanted to work on this one.

DanielVeza’s picture

DanielVeza
he/him
Location Brisbane, AU
CreditAttribution: DanielVeza as a volunteer and at PreviousNext commented

Pushed up a commit that fixes most of the MR feedback. This is the leftover item, need to look into it more.

I think you can use EntityContextDefinition::fromEntityTypeId('entity_view_display') here to simplify

PHPStorm doesn't like it. I wonder if static functions aren't allowed here

DanielVeza’s picture

DanielVeza
he/him
Location Brisbane, AU
CreditAttribution: DanielVeza as a volunteer and at PreviousNext commented
Status: Needs work » Needs review

Setting back to needs review. Resolved all threads except one, and left a comment about why that one may not work. Unless I'm misunderstanding something

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Needs work

For the 1 thread.

DanielVeza’s picture

DanielVeza
he/him
Location Brisbane, AU
CreditAttribution: DanielVeza as a volunteer and at PreviousNext commented
Status: Needs work » Needs review

Agreed with the the last feedback item. Pushed up a commit, back to NR

mstrelan’s picture

mstrelan CreditAttribution: mstrelan at PreviousNext commented
Status: Needs review » Needs work

I think we need to document the params as per the annotation class.

DanielVeza’s picture

DanielVeza
he/him
Location Brisbane, AU
CreditAttribution: DanielVeza as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
mstrelan’s picture

mstrelan CreditAttribution: mstrelan at PreviousNext commented
Status: Needs review » Needs work

As per @larowlan I think we need to add public readonly ?string $deriver = NULL to the constructor params.

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

sorlov’s picture

sorlov CreditAttribution: sorlov at Skilld commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Needs work

Seems to have a check failure

sorlov’s picture

sorlov CreditAttribution: sorlov at Skilld commented
Status: Needs work » Needs review

fixed phpstan issue

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Feedback appears to be addressed.

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 4fcae3ca53 to 11.x and 31ec7d645e to 10.3.x. Thanks!

  • alexpott committed 31ec7d64 on 10.3.x 
    Issue #3421019 by DanielVeza, sorlov, andypost, smustgrave, larowlan,...

  • alexpott committed 4fcae3ca on 11.x 
    Issue #3421019 by DanielVeza, sorlov, andypost, smustgrave, larowlan,...
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper at PreviousNext commented

Can someone close the MR?

Status: Fixed » Closed (fixed)

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