Since symfony/validator 7.4: Support for evaluating options in the base Constraint class is deprecated. Initialize properties in the constructor instead.

Some of the work in MR 12291 for #3522497: Passing an $options array to constraint constructors is deprecated, use named arguments instead may be relevant.

There's handling for that value setter in the other MR I did on the original issue, similar to what you have so far. One thing I found was the need to be able to know in constraint factory createInstance() that the value setter thing happened in config manager create(). I'll try porting over some of that work piece by piece here and see how much it helps with the deprecations.

Instead of what I said in #6, I tried bringing over the bulk of MR 12291 to see what deprecations it might fix and what tests are breaking.

One major change from there is change the new ConstraintBase class not to pass options to the Symfony Contraint class parent, like this:

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;
    }

    $groups = $args['groups'] ?? NULL;
    unset($args['groups']);
    $payload = $args['payload'] ?? NULL;
    unset($args['payload']);

    foreach ($args as $key => $arg) {
      if (property_exists($this, $key)) {
        $this->$key = $arg;
      }
    }
    parent::__construct(NULL, $groups, $payload);
  }

Will investigate the test failures as it seems that might not completely work.

Resolved some of the test issues, but some function test failures still remain.

For the MR removed all the deprecation ignore entries except for "%Since symfony/validator 7.4: Support for passing the choices as the first argument to Symfony\\Component\\Validator\\Constraints\\Choice is deprecated.%", even though this issue is technically about one. They're all closely related, so it might be difficult to address them all completely separately.

After that, still left to do is to double check that changes to certain constraints don't break BC, and adding test coverage.

Wanted to make sure I tracked and resolved any errors related to the changes separate from deprecation warnings, so opened another draft PR 13741 to address actual failures and errors with BC. A bunch of tests are red bc I left out the changes to address the deprecation warnings, which I will add back in later.

Temporarily removed the check on the number of required parameters in a constraint constructor to find which constraint causes errors, and it's ExtensionName.

Finally got MR 13741 to all green.

Approach is this:

• All addConstraint() methods are annotated to accept only either NULL, empty array, or an associative array for $options
• ConstraintManager::create() triggers a deprecation if the options passed are not NULL, empty array, or an associative array. The functionality there that adds the 'value' key to a non-array and non-NULL value is deprecated
• A new ConstraintBase class is introduced that handles constructor argument juggling, so it can handle either named arguments being passed or, for BC, an $options array being passed. This saves (for now) needing to add a constructor to every Drupal constraint class and doing the argument juggling there
• A couple specific constructors were also updated for special handling. For example, ComplexDataConstraint has deprecated passing in the options without the 'properties' key

Some future considerations:

• Follow ups need to be created to change the signatures for the addConstraint() methods, as well as adding constructors with property arguments to every constraint class. One outstanding question is what major this should happen in: it might be a lot of work for contrib/custom modules to be updated by D12, but if D12/Symfony 8 has removed the BC support for the $options array entirely, then D13 might be too far away. Unless for D12, the ConstraintBase class is updated to take over the BC support from the Symfony Constraint class

Other notes:

• Linked CR https://www.drupal.org/node/3554746 to this issue as well, though at this point #3555134: Deprecate passing options array to all constraint plugin parameters is a duplicate. The CR needs updating
• Probably need tests that ConstraintBase (and other constructor changes) works as expected for BC
• The MR currently includes changes from #3555535: Since symfony/validator 7.4: Support for passing the choices as the first argument to Symfony\Component\Validator\Constraints\Choice is deprecated. to get existing tests passing, so this will need to be rebased once the other issue is in, since it's likely that will get in first

My thinking for introducing the new base class is to avoid having to add a constructor that has to do parameter juggling for every core constraint class right now , and then having to remove all that constructor parameter juggling later in the major branch. With the base class, most of the individual constructors only need to be touched once later.

For example, if we did the constructors now, this is is what FileSizeLimitConstrait would look something like:

class FileSizeLimitConstraint extends SymfonyConstraint {

#[HasNamedArguments]
public function __construct(
  int|array|null $fileLimit = 0,
  public int $userLimit = 0,
  ?array $groups = NULL,
  mixed $payload = NULL,
) {
  if (is_array($fileLimit)) {
    $options = $fileLimit;
    parent::__construct($options);
    return;
  }

  parent::_construct(NULL, $groups, $payload);
  $this->fileLimit = $fileLimit ?? 0;
}  

And we'd have to do something similar to that (some easier, some more complicated, depending on how many properties a constraint has and whether the properties can have array values) for every constraint class.

But I'm also of two minds of whether going with the base class is a good idea, and what the future of the base class would be is a good question. Do we get rid of it? (Or maybe we use it to bring constraints more in line with other Drupal plugin types by implementing PluginInspectionInterface? Not sure what that gets us though other than access to the plugin ID and definition. Just throwing out ideas.)

#18 looks like it should work for most cases, except weird ones like ComplexDataConstraint, but that is weird regardless of how we want to do it.

Also the edge(?) case if someone does something like ->addConstraint('FileSizeLimit', ['value' => 100]), but maybe we can just document not to do that in the CR.

If we update every constructor, do we want do any BC testing? Do we just pick a random sample? My plan on testing BC with the base class was to create a few test constraint classes that extend the base class and cover the various conditions in ConstraintFactory::createInstance(). As well as perhaps coverage for the few specific classes that have constructor changes.

Just checking before I go update or add 68 constructors...

OK, did individual constructors in MR 13811, and tests are finally green. Still need to do tests for both MRs. I also did property promotion for the *message* properties, because it was possible previously to override the message text via the options array, even if not common. There were examples of this happening in core/core test code, so it should still be possible now.

I updated the IS with details about the two approaches in the MRs, as well as pros/cons that I think each have.

I'm moving this to NR now to try to get consensus on which approach to take before writing the relevant tests for either approach.

Then let's tag it for framework and rm review.

Added #3561135: Deprecate passing options array to UploadedFileConstraint as I think this was missed due to it not being a plugin.

One thought I just had about possible growing use of named parameters generally, and not just for constraint classes:
For classes where there's a reasonable expectation for named parameters to be passed to their constructors, the named parameters kind of implicitly become API, resulting in some tension with the documented BC policy. This is especially exacerbated since passing named parameters that don't exist will cause fatal errors.

I wonder if we can make this a bit easier generally by making these constructors variadic, with a ...$other or similar parameter used widely. The parameter doesn't have to correspond to any class properties, or technically be used at all. It's just there to allow named parameters to be passed with backward for forward compatibility. I grant that it seems a little ugly, but maybe worth considering?

Sounds good. Will rebase and update MR 13811 and have ready for review hopefully by early next week.

Tagging for IS update based on #28.

Core provides a lot of validators (and is adding more with the ongoing config schema work) but I am not sure how widely added they are in contrib or custom code.

Not sure about contrib, but custom validators have been a regular part of projects I've worked on, especially those having custom entity types or field types. Or situations where the value of one field is constrained by other fields. Maybe we can search "extends Constraint" or "extends SymfonyConstraint" to get an idea of number of contrib constraints.

Either way, since we missed 11.3, the runway between 11.4 and 12.0 might be short, so getting this in early is ideal.

Rebased and updated the deprecation message version. Also Updated the IS.

Still TBD
Add BC tests for a selected/representatve set of constraint
Update CR to include deprecations for addConstraint() method parameters and any other relevant changes

I was thinking about the best way to write the BC tests, and then took a break for holidays. I will revisit as soon as I can, though I certainly welcome anyone else writing some constraint plugin BC tests.

Finally got back to this. Added BC tests, but I decided only to test Drupal constraint plugins whose getDefaultOption() method returns a non-null value. This is so that I could test passing values other than non-associative arrays as $options to ConstraintManager::create(). This simulates passing non-associative array options to all the addConstraint() methods.

For other constraint plugins, passing non-associative array values to addConstraint() should have already been resulting in error to date, so I think testing the constructor changes are of lesser importance. The possible BC issue is if there are contrib/custom projects that implement their own constraint manager or constraint factory, but I think it's fairly low risk?

Will review and update CR tomorrow.

Also tagging "Needs followup" to deprecate implementations of getDefaultOption() and getRequiredOptions() in Drupal constraint plugins, since those methods are deprecated in the Symfony Constraint class. Deprecating those are probably lower priority because with base methods removed in Symfony 8, they'll effectively will never be called.

Updated the CR to include notes about the method signature changes to all the addConstraint() methods and ConstraintManager::create().

Also stubbed these follow ups:
#3569092: Update method signatures for addConstraint() and ConstraintManager::create() and remove HasNamedArguments use
#3569093: [12.x] Remove getDefaultOption() and getRequiredOptions() methods in Drupal Constraint plugin classe
#3569096: Remove BC layer $options argument from Constraint plugin constructors

Cleared the respective issue tags and updated the IS to reflect current state.

I think we could hopefully just delete these implementations in D12 given Symfony itself is removing them from the interface

Agreed. Changed the follow up to be a removal instead of deprecation: [##3569093].

I think I addressed the latest MR comments. I also had to make some changes for new constraints upstream.

For other constraint plugins, passing non-associative array values to addConstraint() should have already been resulting in error to date, so I think testing the constructor changes are of lesser importance. The possible BC issue is if there are contrib/custom projects that implement their own constraint manager or constraint factory, but I think it's fairly low risk?

Thinking about this some more, I wonder if we can just drop the $options BC argument now from the constructors of all these constraint plugins (the ones that return NULL from getDefaultOption()). The only BC break I can think of is if there are any contrib or custom subclasses that implemented their own constructor and called the parent constructor, but in those cases, we're already changing the constructor signature and breaking that already.
Edit: maybe not? since I imagine not many are passing the $payload or $groups argument.

If there's any reason to keep the $options argument, I'm all for it, since I'm not overly eager to update a whole bunch of constructors again right now, but they'll have to be removed eventually, so it might make sense to just do it if we can.

Great work! Just one issue with ComplexDataConstraint and property collision

Replied to MR comment. I think this isn't a regression, but we can handle in a follow up.

Updated #3569096: Remove BC layer $options argument from Constraint plugin constructors to include removing the #[HasNamedArgument] use as well as the BC layer in ConstraintManager/ConstraintFactory.

Needs a reroll after #3555532: Since symfony/http-foundation 7.4: Request::get() is deprecated, use properties ->attributes, query or request directly instead. - fine to self RTBC

Stubbed a followup for the properties thing: #3570464: ComplexDataConstraint does not work correctly with typed data having a property named properties

Rebased so back to RTBC.

Note that the only conflict was in .deprecation-ignore.txt.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Shushing the bot. Back to RTBC.

Committed to main - thanks folks!

Backported to 11.x and published CR

Now that this is in, the D12 follow up/Symfony 8 blocker is ready for review: #3569092: Update method signatures for addConstraint() and ConstraintManager::create() and remove HasNamedArguments use

#3569096: Remove BC layer $options argument from Constraint plugin constructors is lower priority since Symfony 8 has kept the $options parameter around. It just does nothing.

Canvas validates many things very thoroughly. In doing so, it seems to trigger a higher number of deprecation notices than most projects: here's an example with ~6000 deprecation notices or a >1 MiB large subject string to check the expected deprecations against … which makes preg_match() fail incorrectly 🤯🧟

See: https://git.drupalcode.org/project/canvas/-/merge_requests/280#note_674164.

Work-around: only check unique deprecation notices: https://git.drupalcode.org/project/canvas/-/merge_requests/280#note_674181

Note that this only happens if your core/phpunit.xml does not have SYMFONY_DEPRECATIONS_HELPER configured to either disabled or not at all (i.e. using the default). An ignore file does NOT help.

Note that simply changing https://www.drupal.org/project/gitlab_templates' default of SYMFONY_DEPRECATIONS_HELPER: disabled and instead setting SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=$CI_PROJECT_DIR/$_WEB_ROOT/core/.deprecation-ignore.txt" causes it to fail the same way on GitLab CI: https://git.drupalcode.org/project/canvas/-/jobs/8388370#L1117

Just to note, I think that the fix here has a couple issues for 11.4.x, which I have reported here: #3599189: Some deprecation messages are missing E_USER_DEPRECATED