- 🇬🇧United Kingdom jonathanshaw Stroud, UK
It's not clear what the problem is that we are trying to solve here.
Lower-level constraints are still being validated recursively even if high-level constraints report violations, so changing the sequence of validation will not lead to changes in what violations are reported for high-level validation. Nothing valid becomes invalid, or vice versa, if we change the order of execution.
I can see 2 problems that are still relevant:
The DX problem: Entity/field Validators have to be coded defensively, they cannot assume their fields/properties are themselves valid and in some sense this is not DRY. (see #61)
The UX problem: Higher-level entity/field constraint violations are reported to users in addition to and prior to the lower-level field/property violation that is the root cause of the higher-level constraint failure; this could be seen as confusing - it is more helpful to focus the user's attention on the lower-level violation.
Here's a proposal for addressing this without an elaborate system of explicit dependencies between constraints:
1. Use try/catch around the call to each constraint validator, and add a violation "Could not verify '{$constraint_message'" if the validator blows up and throws an exception or error. This would mean that validators did not need to be coded defensively; they could generally assume lower-level data was valid.
2. Only add this new kind of violation, if the child node validators do not add any violations. This would lead to the user typically only being exposed to a single violation message that correctly identified the root cause. The only drawback to this is that a poorly coded validator, which breaks not because lower-level data is invalid but because of some other unrelated bug, will not have its failure surfaced to the user until after any other lower-level violations are fixed. I think that's acceptable - bugs cause suboptimal UX by nature, we can't prevent that.
3. Store/retrieve/display the violations in the opposite sequence to now. If we think it's often better UX for the lower-level violations to be reported first and acted on first, then we can just handle that either at the level of the violation internal storage, retrieval, or even display. We don't need to change the sequence of execution to address this.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The problem we're trying to solve here is two-fold:
- all of Drupal core, for both content and config, lumps all validation constraints together in one bucket
- all of Drupal core, for both content and config, pays no attention to the validation sequence: it always uses the same top-down (breadth-first, see
\Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validateNode()
) recursive validation strategy, which results in the entity-level validation constraints not being able to assume valid field values and field-level validation constraints not being able to assume valid property values
I suspect
- https://symfony.com/doc/current/validation/groups.html
- https://symfony.com/doc/current/validation/sequence_provider.html
- https://symfony.com/doc/current/reference/constraints/Sequentially.html is the solution.
are part of the solution.
\Drupal\Tests\jsonapi\Functional\ResourceTestBase
has thorough test coverage, but it does NOT assert that the validation error messages in case of a nonsensical value (for example, the stringhello world
being specified for a boolean "status" field or an integer "weight" field).
That test (and the REST equivalents) are specifically testing access control and basic shape of the representation, not detailed validation.I have recently started encountering this problem again, but now in the world of Config, because I've been working on config validation. There, it's likely that you'll see multiple errors for the same config property path, for example:
[ 0 => "The '' plugin does not exist.", 1 => "This value should not be null.", ]
It goes without saying that the latter (generated by the
NotNull
constraint) should result in none of the other validators running, because there literally is no point in them running. The consequence: noisy, confusing validation errors. - 🇭🇺Hungary mxr576 Hungary
Wow, wonderful thread! :) I learned a lot from reading this and I am also still unsure how many problems we have and what are those... :D
I also encountered the problem in the past that in validators you have to use defensive coding or you have to "fail silently" in Validator A because Validator B will fail too and that is enough.
I have checked the EmailValidator from Symfony 7.1 and they also use defensive coding there.
It goes without saying that the latter (generated by the NotNull constraint) should result in none of the other validators running, because there literally is no point in them running.
Maybe we should just forget stacking independent validator units and instead concrete and complex requirements should be covered by a specific validator... which goes against compos ability and (again) goes against what we can see in Symfony... BUT as it was highlighted and proved with historical context in #77, we already have a Drupalism in typed data validation due to the implementation does not support validation groups (which are essentially a solution for considering a set of independent validators as one validator unit).
- 🇭🇺Hungary mxr576 Hungary
Maybe we should just forget stacking independent validator units and instead concrete and complex requirements should be covered by a specific validator... which goes against composability
Well there is https://symfony.com/doc/7.1/reference/constraints/Compound.html for that in Symfony, but that would still expose all validation error independently and maybe what I would expected a "composite" would have its own dedicated validation error that would be displayed when any of the validators failed.
Again... preferences.