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 #0
Problem/Motivation
Follow-up from #2002158-61: Convert form validation of comments to entity validation
Constraint validators are unable to use ContainerFactoryPluginInterface because they are created using a new $class in Symfony's ConstraintValidatorFactory
Proposed resolution
Extend symfony's ConstraintValidatorFactory and include the logic for ContainerFactoryPluginInterface
Remaining tasks
None
User interface changes
none
API changes
None
Beta phase evaluation
Issue category | Task because leads to better code, allowing DI |
---|---|
Issue priority | Normal because nicer DX - note was raised in review of #2403817: Feed entity validation misses form validation logic which is critical |
Disruption | None |
Comment | File | Size | Author |
---|---|---|---|
#27 | 2197029-27.patch | 7.31 KB | ParisLiakos |
#17 | interdiff.txt | 1.44 KB | dawehner |
#17 | 2197029-17.patch | 7.23 KB | dawehner |
Comments
Comment #1
dawehnerWell, we still use the default factory here:
The described class is just for the symfony part of the constrains.
Comment #2
andypostI still think this needs more investigation, constraints could become more complex and better to inject their dependencies
Comment #3
XanoConstraintsManager uses ContainerFactory, and not ConstraintValidationFactory (which belongs to Symfony), so dependency injection works.
If this doesn't answer your question, then I'm afraid I don't know what you mean.
Comment #4
larowlanYes, constraint plugins can implement ContainerFactoryPluginInterface, but validators cannot.
See CommentNameConstraintValidator.
Also see #2403817: Feed entity validation misses form validation logic - the
protected function feedStorage()
over there demonstrates that ContainerFactoryPluginInterface doesn't work for the validators.Comment #5
larowlanAnd an example/failing test
Comment #6
larowlanAnd a fix showing it working
Comment #8
larowlanAnd remove the comment that sent me here.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedlooks great
Comment #10
larowlanbeta eval, issue update
Comment #11
dawehnerPassing along the plugin configuration/ID and definition is pointless afaik. Why do you not just use the ClassResolver?
Comment #12
dawehnerAdapted the code a bit.
Comment #13
larowlanNice
Comment #15
dawehnermeh.
Comment #17
dawehnerLet's fix it.
Comment #18
larowlanlet's do it
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedbetter :)
Comment #22
fagoThis looks good, however I'm unsure how people will know they can do it that way? Should we document it at the manager given #2195083: Add a dedicated @Constraint annotation class is not done yet.
Comment #24
larowlanrandom fails
Comment #26
alexpott2197029-17.patch no longer applies.
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #28
larowlanStraight reroll
Comment #29
alexpottI think being able to unit test validators cleanly reduces fragility and there is no disruption. Committed 5f7ec33 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.