- Issue created by @longwave
Note from @longwave about this on Slack:
i see Symfony adds a HasNamedArguments attribute to constraints that have been converted.. so we could check for that at runtime although i hope reflection of that isn't too expensive
I don't think reflection on the class will have much performance penalty, since the plugin class is being instantiated right after anyway, so it's getting loaded in memory regardless.
But if it does turn out to be a performance issue, one idea I have is to add functionality to
AttributeClassDiscovery::parseClass()
to be able to pick up additional attributes like this and add them to the plugin definition, since the plugin class is already being reflected in discovery. The effort in ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active could be amended to include this.It would look something like:
- In
parseClass()
, call a new method, something likeparseAdditionalAttributes()
that takes the ReflectionClass object as one of its parameters - In
parseAdditionalAttributes()
, check whether the plugin class implements a (new) Interface, maybe something likeAdditionalAttributesPluginInterface
(name TBD), that has its ownpublic static function parseAdditionalAttributes()
method - Plugin classes implementing the interface and method can then use the reflection class object to look at other attributes on the class or its methods, properties, etc. specific to the plugin class
- For relevant constraint plugins, the implementation of
parseAdditionalAttributes()
would look at the constructor for the theHasNamedArguments
attribute, and if so, that gets added back to the plugin definition
Note that the above would have to account for attributes possibly being defined in other modules and the attribute discovery filecache, so there are complications.
And I'm not completely sure this would be worth the effort for this use case, but just throwing it out there as a possibility.
- In
- 🇬🇧United Kingdom longwave UK
Making this active and major, as this will likely require new deprecations and will be removed by Symfony 8/Drupal 12, we should aim to land this in 11.3.
- Merge request !12268Draft: Issue #3522497: Pass named arguments to HasNamedArguments plugins. → (Open) created by godotislate
Removed the entry in .deprecation-ignore.txt and finally got all tests passing.
The deprecation layer still needs doing. It might be a bit complex and probably needs some thinking through. Among other things, even though all the docblocks the options passed to
addConstraint($constraint_name, $options = NULL)
typed@param array|null $options
, there are cases such as this inEntityDataDefinition::setEntityType()
where$options
is a string:public function setEntityTypeId($entity_type_id) { return $this->addConstraint('EntityType', $entity_type_id); }
The
ConstraintManager
even accounts for this by turning non-array $options into array in::create()
:public function create($name, $options) { if (!is_array($options)) { // Plugins need an array as configuration, so make sure we have one. // The constraint classes support passing the options as part of the // 'value' key also. $options = isset($options) ? ['value' => $options] : []; } return $this->createInstance($name, $options); }
Problem is that with named arguments, an exception is thrown with invalid argument names, and it can't be assumed that
value
will always be a valid argument name.Put up a new separate MR to explore deprecating an array options to Drupal constraints. To take a bit of shortcut in not having to add constructors to every Drupal constraint plugin that doesn't have one, (or at least to put off doing them all now), I created a new base constraint class that looks like this:
use Symfony\Component\Validator\Attribute\HasNamedArguments; use Symfony\Component\Validator\Constraint; /** * Base class for constraint plugins. * * This provides generic support for named option parameters passed to the * class constructor. */ abstract class ConstraintBase extends Constraint { #[HasNamedArguments] public function __construct(...$args) { if (!empty($args) && array_is_list($args)) { @trigger_error(sprintf('Passing an array of options to configure the "%s" constraint is deprecated in drupal:11.3.0 and will not be supported in drupal:12.0.0. Use named arguments instead. See https://www.drupal.org/node/3522497', get_class($this)), E_USER_DEPRECATED); parent::__construct(...$args); return; } parent::__construct($args); } }
Then I set all Drupal constraints that were directly extending
Symfony\Component\Validator\Constraint
to extend this base class instead.In addition, I deprecated passing string or list
$options
to the variousaddConstraint($name, $options)
methods. Generally people won't be calling constraint plugin constructors directly, and will be using addConstraint to pass the options, and this is the cleanest way to make sure that the $options passed line up with constraint plugin class properties.That all said, doing all this seems pretty cumbersome. As is it's not really taking advantage of typed values you get named constructor parameters, since right now there's just a lot of trickery leveraging the
...
operator.It might be possible to continue to allow string and list arrays to be passed to
addConstraint()
, and removes a lot of the need to add keys to all the arrays, but if and when the constraint classes actually have named parameters in their constructors, that might not work anymore.