Passing an $options array to constraint constructors is deprecated, use named arguments instead

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 #3443882: Allow attribute-based plugins to discover supplemental attributes to set additional properties could be amended to include this.

It would look something like:

1. In parseClass(), call a new method, something like parseAdditionalAttributes() that takes the ReflectionClass object as one of its parameters
2. In parseAdditionalAttributes(), check whether the plugin class implements a (new) Interface, maybe something like AdditionalAttributesPluginInterface (name TBD), that has its own public static function parseAdditionalAttributes() method
3. 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
4. For relevant constraint plugins, the implementation of parseAdditionalAttributes() would look at the constructor for the the HasNamedArguments 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.

Never mind, obviously additional interfaces can't be added Symfony classes, and several of the Symfony constraint plugins are added directly in the plugin manager and not through attribute discovery.

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 for addConstraint($constraint_name, $options = NULL) have this type for the options @param array|null $options, there are cases such as this in EntityDataDefinition::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 12291 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 various addConstraint($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.

Anyway, as mentioned this additional work is done in a separate MR (12291), and MR 12268 is the one that handles only deprecation warnings from the Symfony constraints.

One more thought: with named parameters, does this start to make constraint plugin classes or constructors API?

Re #10. Discussed with @longwave on Slack, and best way forward here to get this in for 11.3 is to reduce scope to address only the Symfony constraints for now (MR 12268). Can split off using named parameters on all constraints to a follow up.

We should also try to include our own deprecation message somehow as well, since most people using constraints do not instantiate directly via the constructor but through addConstraint(), so the message telling them to use named parameters instead is not that helpful.

I will try to work on 12268 either today or tomorrow.

Updated MR 12268 with a small refactoring. Still WIP for some tests of the logic in ConstraintFactory, and this needs a CR.

Re #16, I think reducing scope here to minimum to get tests passing with the Symfony deprecation ignore removed makes sense, and handle everything else in follow ups. I'll do my best to get that done today.

Made changes per MR feedback. Removed the deprecation warning as well.

One thing I wonder if we need to do here: in ConstraintManager::create(), there's code to convert $options not passed as an array to an array with a value key. This worked because when passed as an $options array to the plugin constructor, if the plugin classes returns something from getDefaultOption(), the value at the value key gets set as the value for the default option.

This doesn't necessarily work with named arguments, because $value might not be a constructor argument. I did some handling for this in MR 12291, by adding an internal boolean flag value to the $options array to indicate that options were not originally passed as an array, so that they shouldn't be passed as named arguments. Nothing is breaking in core tests, but I wonder if we need this possibly for contrib/custom code use of the Symfony Constraint subclasses. Or maybe it's fine to leave to a follow up?

Do we need any tests here now?

Made minor tweaks the IS and issue title. Might also need a plan on how to break up follow up work, if it needs to be broken up at all.

Stubbed a follow up #3555134: Deprecate passing options array to all constraint plugin parameters. Re #21 about a plan, maybe it can look something like this:

First, carry over most if not all the work from MR 12291, including

• Deprecate passing anything other than an associative array to the addConstraint() methods
• Deprecate passing an options array to any Drupal constraint plugin class
• To handle this with BC, introduce an abstract class ConstraintBase with a constructor that uses the spread operator, so it can take either named arguments or the options array.

Then later, in Drupal 13(?):

• Make changes to all Drupal Constraint plugin classes so that their constructors have arguments for all properties
• Figure out what to do with the ConstraintBase class at that time (if it's still needed or if it needs changing)

Committed to 11.x and backported to 11.3.x
Thanks!

Published the change record

Since we didn't do any deprecation here, linked https://www.drupal.org/node/3554746 to #3555134: Deprecate passing options array to all constraint plugin parameters instead.

Not sure if we need to do anything with \Drupal\file\Validation\Constraint\UploadedFileConstraint ?

Did we come to any conclusion about what do for

    // If the plugin provides a factory method, pass the container to it.
    if (is_subclass_of($plugin_class, ContainerFactoryPluginInterface::class)) {
      return $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition);
    }

This change currently breaks backwards compatibility with the Regex constraint, and throws this fatal exception when inspecting configurations using constraints like:

    alert_styles:
      type: 'string'
      label: 'Available alert styles'
      constraints:
        Regex: '/^(.*)|(.*)$/'

Error: Unknown named parameter $value in Drupal\Core\Validation\ConstraintFactory->createInstance() (line 66 of core/lib/Drupal/Core/Validation/ConstraintFactory.php).

Drupal\Component\Plugin\PluginManagerBase->createInstance() (Line: 89)
Drupal\Core\Validation\ConstraintManager->create() (Line: 123)
Drupal\Core\TypedData\TypedData->getConstraints() (Line: 45)
Drupal\Core\TypedData\Validation\TypedDataMetadata->getConstraints() (Line: 38)
Drupal\Core\TypedData\Validation\TypedDataMetadata->findConstraints() (Line: 170)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 190)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 119)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate() (Line: 93)
Drupal\Core\TypedData\Validation\RecursiveValidator->validate() (Line: 132)
Drupal\Core\TypedData\TypedData->validate() (Line: 355)
Drupal\config_inspector\ConfigInspectorManager->validateValues() (Line: 176)
Drupal\config_inspector\Controller\ConfigInspectorController->overview()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 634)
Drupal\Core\Render\Renderer::Drupal\Core\Render\{closure}()
Fiber->start() (Line: 635)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 118)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 92)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 54)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 745)
Drupal\Core\DrupalKernel->handle() (Line: 19)

This is because the Regex constraint uses $parameter as its first argument rather than $value, so special handling might be necessary for it (as well as any other non-standard constraint)

Not sure whether it should be addressed here, or opened as a separate issue.

@codebymikey, I noticed the same issue, and I had to apply a few patches. This depended on the project’s modules; however, in my case, it was addressed by the following patch: https://www.drupal.org/i/3567470, https://www.drupal.org/i/3567470