Problem/Motivation

SDC components have a group property: https://api.drupal.org/api/drupal/core%21modules%21sdc%21src%21Component...

But it doesn't seem this property is leveraged until now.

Proposed resolution

Implement CategorizingPluginManagerInterface in ComponentPluginManager:

It may not be possible to use CategorizingPluginManagerTrait because the property is group instead of category.

User interface changes

No. Some contrib modules can leverage the added methods to build UI widgets, but nothing will happen in Core in the scope of this ticket.

API changes

Only additions, so not breaking.

Release notes snippet

"ComponentPluginManager is implementing CategorizingPluginManagerInterface"

Issue fork drupal-3474533

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

Comments

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

I will try to propose a MR.

pdureau’s picture

pdureau commented
Version: 11.0.x-dev » 11.x-dev
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Looking forward to an MR to review! 🤓

pdureau’s picture

pdureau commented

If you wish, you can have a look on an existing and well tested implementation of this interface for SDC plugin manager: https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/Componen...

vensires’s picture

vensires
Location Greece
commented
Assigned: pdureau » vensires

vensires opened merge request !9631

vensires’s picture

vensires
Location Greece
commented
Status: Active » Needs review

Implemented the changes required in the MR. Please review.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

One test failed:

    There was 1 failure:
    
    1)
    Drupal\Tests\layout_builder\Functional\LayoutSectionTest::testLayoutSectionFormatterAccess
    Behat\Mink\Exception\ResponseTextException: The text "Hello test world" was
    not found anywhere in the text of the current page.
    
    /builds/issue/drupal-3474533/vendor/behat/mink/src/WebAssert.php:907
    /builds/issue/drupal-3474533/vendor/behat/mink/src/WebAssert.php:293
    /builds/issue/drupal-3474533/core/tests/Drupal/Tests/WebAssert.php:979
    /builds/issue/drupal-3474533/core/modules/layout_builder/tests/src/Functional/LayoutSectionTest.php:311
    /builds/issue/drupal-3474533/core/modules/layout_builder/tests/src/Functional/LayoutSectionTest.php:210
vensires’s picture

vensires
Location Greece
commented
Assigned: vensires » Unassigned

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Rebased, LayoutSectionTest passed locally for me so perhaps an unrelated random fail, let's see what happens on CI.

longwave opened merge request !10245

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Alternatively why can't we just add category as the trait requires, and copy it from group with a default? See MR!10245.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Assigned: Unassigned » wim leers

Assigning to Wim for review.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » longwave
Status: Needs review » Needs work

#14: interesting alternative! And very pragmatic 👏

Just one nit: I think the default group should be defined in \Drupal\Core\Theme\ComponentPluginManager::$defaults 😊 That makes it more explicit, less magical.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Assigned: longwave » wim leers
Status: Needs work » Needs review

#14 is not doable, we need a translatable string but you can't call t() or new TranslatableMarkup when initialising a class property.

Fixed PHPStan, the baseline addition will be removed in #3352916: Fix PHPStan L1 errors "Call to method getDefinitions()/getSortedDefinitions() on an unknown class Drupal\Core\Plugin\CategorizingPluginManagerTrait."

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

Ah, good point, @longwave! This is the most pragmatic approach, then :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

One question on the MR about the phpstan baseline changes - if someone can confirm they're needed.

Fine to self RTBC afterwards

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

See #17, the problem is in the trait, PHPStan doesn't catch it if you run it on the file alone - not sure why.

Alternatively if we land #3352916: Fix PHPStan L1 errors "Call to method getDefinitions()/getSortedDefinitions() on an unknown class Drupal\Core\Plugin\CategorizingPluginManagerTrait." first then the baseline changes can be removed.

larowlan changed the visibility of the branch 3474533-componentpluginmanager-must-implement to hidden.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Credits

Rerolled after #3352916: Fix PHPStan L1 errors "Call to method getDefinitions()/getSortedDefinitions() on an unknown class Drupal\Core\Plugin\CategorizingPluginManagerTrait.", only removed the baseline changes, if it comes back green will commit

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x - thanks!

larowlan closed merge request !10245

  • larowlan committed 7b5bb6fd on 11.x 
    Issue #3474533 by longwave, vensires, larowlan, pdureau, wim leers:...

Status: Fixed » Closed (fixed)

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