Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Updated: Comment #N
Follow-up from #2226813: Create a specific Plugin annotation for Drupal Constraint plugins.
Problem/Motivation
The split of Constraints into a plugin and a separate Constraint Validation class is unneeded for us and confusing.
Proposed resolution
Create a base plugin that implements Symfony\Component\Validator\ConstraintValidatorInterface
Override public function validatedBy() in the base plugin class so it returns the same class name.
Remaining tasks
TODO
User interface changes
N/A
API changes
separate classes will be combined into one.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 2226821-15.patch | 22.33 KB | jamesquinton |
| #12 | 2226821-12.patch | 22.34 KB | pwolanin |
| #12 | increment.txt | 12.34 KB | pwolanin |
| #9 | 2226821-9.patch | 10 KB | pwolanin |
| #9 | increment.txt | 5.99 KB | pwolanin |
Comments
Comment #1
pwolanin commentedComment #2
pwolanin commentedtagging
Comment #3
pwolanin commentedHere's a possible base class
Comment #4
pwolanin commentedFix a couple items
Comment #5
fagoThis moves us further away from symfony best practices, but I agree it makes more sense for Drupal. So let's see what we can do.
This seems reasonable. But actually, the passed constraint will be $this then. Can we make it so that this does not confuse devs, e.g. hide it away by forwarding to another method?
Comment #6
pwolanin commentedAn example conversion - let's see what fails.
Comment #7
pwolanin commentedHere's an example adding a different abstract method to make things easier still.
Comment #9
pwolanin commenteda few more conversions
Comment #10
fagoHaving a separate method seems great as we can do the is null check also - we've got the constraints that do the null checking already so there is no point in *any* contrib constraint going with null values also.
#2110345: Simplify validation constraint implementations for fields is finally passing again and should be ready, so let's get it in first and base this on it?
Comment #12
pwolanin commentedLooking through core, there are a few cases that cant be combined, like \Drupal\Core\Validation\Plugin\Validation\Constraint\AllowedValuesConstraintValidator because it's extending a Symfony validator, and we can't make the plugin extend 2 different base classes.
Comment #14
pwolanin commentedHmm, that broke a lot - errors like
Fatal error: Call to a member function id() on a non-objectComment #15
jamesquinton commentedRe-rolling patch
Comment #16
jamesquinton commentedSuccessfully rolled back the patch, now testing and debugging.
Comment #17
jamesquinton commentedRe-rolled patch and successfully run tests.
Comment #19
jamesquinton commentedHmmm - the tests pass locally. Will take another look at this.
Comment #20
pwolanin commentedSo, looking at one of the failures in Drupal\aggregator\Tests\AddFeedTest, I think it's since ComplexDataConstrainValidator has this method:
when we return the class name from validatedBy() that calls is instantiated without passing in any "properties" in the options.
This is kind of a mismatch since we are returning the class name rather than the existing instance. Seems like symfony should take an instance instead of a class name?
Not sure how to unwind this api.
Comment #21
jamesquinton commentedComment #22
pwolanin commentedSo, it seem like we cannot do this is a consistent way - when a constraint has a required option we get a fatal exception when trying to make that class also the constraint validator since symfony constructs an instance without passing in any options.
In addition, we can't combine them in cases where both the constraint and validator would need to extend another class.
So, while the need for 2 classes is annoying, I think having a inconsistent implementation would be worse.
Comment #23
pwolanin commentedComment #24
fagoI see, that sucks :-/
Comment #25
pwolanin commentedsad chx...
Comment #26
fago#2235457: Use link field for shortcut entity has just added a
LinkTypeConstraintin a single class. Thus re-opening this for discussion.However, I'm not sure it's viable to do it as done by LinkTypeConstraint. Reasons:
- validate() receives another instance of the same class as argument, instead of reading from $this, that's weird.
- As soon as you've got a required option, this approach would stop working.
So instead, could we allow having a single constraint class/service with the help of pre-defined validator class acting as adapter? It could just invoke validate() on constraints implementing ValidatingConstraintInterface, which is basically ConstraintValidatorInterface minus the Constraint parameter. Then, you can read your settings from $this and it would continue to work even with required options, as no other instance will be created?
Comment #27
fagoBesides that I figured that symfony documents to support services as validators, but the validator and our code lacks this feature. Looking closer, I figured it's added in by symfony framework:
https://github.com/symfony/symfony/blob/d94d837e9ea75d76eeb0a43e0535a5e7...
So adding something like that in as well seems to be a good idea also.
Comment #38
catchI think this got even stricter in Symfony since this issue was opened. We probably need to do it alongside #3054535: Discuss whether to decouple from Symfony Validator. Moving to a task because it's mostly an API annoyance, very annoying though it is.