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
Comment | File | Size | Author |
---|---|---|---|
#10 | 3240909-10.patch | 4.69 KB | alexpott |
#10 | 6-10-interdiff.txt | 5.85 KB | alexpott |
#6 | 3240909-6.patch | 4.25 KB | andypost |
#6 | interdiff.txt | 652 bytes | andypost |
Comments
Comment #2
alexpottComment #3
alexpottUpdated the issue summary with more details about where the deprecation is coming from.
Comment #4
andypostOne more hunk from 8.1 patch
Comment #5
alexpott@andypost++ nice spot!
Comment #6
andypostcleaner approach with default value (could be empty string too, for broken plugin too)
Comment #7
daffie CreditAttribution: daffie commentedThe 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.Comment #8
alexpottI 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.
Comment #9
tim.plunkettPutting 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...
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_?
Comment #10
alexpottThanks 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.
Comment #11
tim.plunkettThanks! I like how this ended up.
Comment #12
larowlanCommitted 390e3e0 and pushed to 9.3.x. Thanks!