Problem/Motivation

Running \Drupal\Tests\layout_builder\Unit\SectionStorageManagerTest triggers deprecations due to the plugin class being NULL.

The deprecation is triggered by code in \Drupal\Core\Plugin\DefaultPluginManager::processDefinition() specifically the ltrim() here...

    // Keep class definitions standard with no leading slash.
    if ($definition instanceof PluginDefinitionInterface) {
      $definition->setClass(ltrim($definition->getClass(), '\\'));
    }

This code effectively means that other plugin definitions processed by a plugin manager can never have NULL for \Drupal\Component\Plugin\Definition\PluginDefinition::$class since after ltrim()ing we set the class again... which will result in a NULL turning into an empty string.

Steps to reproduce

Run \Drupal\Tests\layout_builder\Unit\SectionStorageManagerTest on PHP 8.1
Run \Drupal\Tests\Core\Layout\LayoutPluginManagerTest oon PHP 8.1

Proposed resolution

Set class or default plugin classes to an empty string.

Remaining tasks

Decide on fix.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3240909-10.patch4.69 KBalexpott
#10 6-10-interdiff.txt5.85 KBalexpott
#6 3240909-6.patch4.25 KBandypost
#6 interdiff.txt652 bytesandypost
#4 3240909-4.patch4.17 KBandypost
#4 interdiff.txt528 bytesandypost
#2 3240909-2.patch3.66 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: \Drupal\Tests\layout_builder\Unit\SectionStorageManagerTest triggers deprecations on PHP 8.1 » Plugin definitions with no class trigger deprecations on PHP 8.1
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3240909-2.patch3.66 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

Updated the issue summary with more details about where the deprecation is coming from.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt528 bytes
new 3240909-4.patch4.17 KB

One more hunk from 8.1 patch

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@andypost++ nice spot!

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt652 bytes
new 3240909-6.patch4.25 KB

cleaner approach with default value (could be empty string too, for broken plugin too)

daffie’s picture

daffie commented
Status: Needs review » Needs work

The patch looks good to me. When we remove the change to ore/lib/Drupal/Component/Plugin/Definition/PluginDefinition.php it will be RTBC for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinition.php
@@ -18,6 +18,10 @@ class PluginDefinition implements PluginDefinitionInterface {
+   *
+   * @todo decide if this should be fixed to have a default value of '' or
+   *   should we change tests like
+   *   \Drupal\Tests\layout_builder\Unit\SectionStorageManagerTest.
    */
   protected $class;

I think we need a discussion about this. Given the trim() in \Drupal\Core\Plugin\DefaultPluginManager::processDefinition() this is essentially never NULL in a real situation.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
  1. +++ b/core/lib/Drupal/Component/Plugin/Definition/PluginDefinition.php
@@ -18,6 +18,10 @@ class PluginDefinition implements PluginDefinitionInterface {
+   * @todo decide if this should be fixed to have a default value of '' or
+   *   should we change tests like
+   *   \Drupal\Tests\layout_builder\Unit\SectionStorageManagerTest.
    */
   protected $class;

    Putting a default of '' seems to imply that setting this is not an essential responsibility of plugins, which I dislike. I wish there was some way to indicate that a property was invalid unless it had a value set to it later... 

  2. +++ b/core/modules/layout_builder/tests/src/Unit/SectionStorageManagerTest.php
@@ -126,10 +126,10 @@ public function testLoadNull() {
+      'plugin1' => (new SectionStorageDefinition())->setClass(SectionStorageDefinition::class),

    I know these don't actually matter since it's not checked, but putting SectionStorageDefinition here is confusing to me. In reality it would be something that implements SectionStorageInterface.

    But for the sake of consistency with the rest of the changes here, why not __CLASS_?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 6-10-interdiff.txt5.85 KB
new 3240909-10.patch4.69 KB

Thanks for the review @tim.plunkett. What you say about the class being a property you have to set makes a lot of sense so I've removed that and added an assert in the DefaultPluginManager - so PHP versions < 8.1 will also warn (when asserts are turned on) if the class is not a string. For the other part of your review I decided to use the plugin interface as the string because that ties things together and __CLASS__ is also a little confusing because it is not a plugin object either.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Thanks! I like how this ended up.

larowlan’s picture

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

Committed 390e3e0 and pushed to 9.3.x. Thanks!

  • larowlan committed 390e3e0 on 9.3.x 
    Issue #3240909 by andypost, alexpott, tim.plunkett: Plugin definitions...

Status: Fixed » Closed (fixed)

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