Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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 CreditAttribution: pwolanin commentedComment #2
pwolanin CreditAttribution: pwolanin commentedtagging
Comment #3
pwolanin CreditAttribution: pwolanin commentedHere's a possible base class
Comment #4
pwolanin CreditAttribution: 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 CreditAttribution: pwolanin commentedAn example conversion - let's see what fails.
Comment #7
pwolanin CreditAttribution: pwolanin commentedHere's an example adding a different abstract method to make things easier still.
Comment #9
pwolanin CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: pwolanin commentedHmm, that broke a lot - errors like
Fatal error: Call to a member function id() on a non-objectComment #15
jamesquinton CreditAttribution: jamesquinton commentedRe-rolling patch
Comment #16
jamesquinton CreditAttribution: jamesquinton commentedSuccessfully rolled back the patch, now testing and debugging.
Comment #17
jamesquinton CreditAttribution: jamesquinton commentedRe-rolled patch and successfully run tests.
Comment #19
jamesquinton CreditAttribution: jamesquinton commentedHmmm - the tests pass locally. Will take another look at this.
Comment #20
pwolanin CreditAttribution: 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 CreditAttribution: jamesquinton commentedComment #22
pwolanin CreditAttribution: 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 CreditAttribution: pwolanin commentedComment #24
fagoI see, that sucks :-/
Comment #25
pwolanin CreditAttribution: 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.