Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Overview
In the case of validation errors we return sets of violations and check their counts:
[$tree, $props, $violations] = $this->convertClientToServer($layout, $model);
if ($violations->count() > 0) {
return new EntityConstraintViolationList($entity, iterator_to_array($violations));
}
[$props, $violations] = $this->clientModelToServerProps($tree, $model);
if ($violations->count() > 0) {
return [NULL, NULL, $violations];
}
etc.
This feels a bit too much Go and not enough like PHP - we have exceptions, we can throw those in the case of error.
Proposed resolution
Add an exception wrapper around ConstraintViolationList and throw it when we detect violations, catch it in the correct places, and only handle the success case everywhere else.
User interface changes
None
Issue fork experience_builder-3497203
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
longwaveIn fact Symfony already provides
Symfony\Component\Validator\Exception\ValidationFailedExceptionwhich does exactly what we need.Comment #4
tedbowI didn't know about
Symfony\Component\Validator\Exception\ValidationFailedException. seems like a good ideaComment #5
tedbowJust merged #3495126: Move SDC specific logic out of ClientsideConversionTrait into component source plugins so will need to resolve conflicts
Comment #6
longwaveDidn't even use
ValidationFailedException, we already have a specificConstraintViolationExceptionin this codebase!Conflicts resolved, also added missing return types for #3495126: Move SDC specific logic out of ClientsideConversionTrait into component source plugins given I'm touching these lines anyway.
Comment #7
tedbowLooks good to me. Assign to @wimleers because he or @f.mazeikis needs to approve this MR.
don't have to do this in this issue but do we actually need `
ConstraintViolationException`? couldn't we just useSymfony\Component\Validator\Exception\ValidationFailedException?Comment #9
wim leersFound this via @longwave's #3495126-4: Move SDC specific logic out of ClientsideConversionTrait into component source plugins.
Like @tedbow in #4, I didn't know about Symfony's
ValidationFailedExceptionyet either — thanks @longwave in #2! 😄 But then in #6, @longwave decided to not use it because we already have …… so I second @tedbow's questioning XB's own
final class ConstraintViolationException extends \Exception.Review
See MR. I spotted 2 bugs.
That's why I didn't create the novice follow-up issue that @larowlan requested at https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
Comment #10
longwaveI started looking at converting ConstraintViolationException to ViolationFailedException but then realised that we can't do the followup if we are using the Symfony class directly? I think it's fine to have our own exception here instead of Symfony's, especially now we are catching it in API responses it's probably better to differentiate I think.
Comment #11
tedbowre #10, Yeah I agree the benefit of having something like `
violationWithPropertyPathReplacePrefix` onConstraintViolationExceptionseem more having our own exception classComment #12
longwaveAdded the remapping code here, no need for the followup.
Comment #13
wim leersRTBC'd — with @longwave's latest commits, this now looks like a magnificent improvement!
Only a single request for either a follow-up or a refinement, to bring that gloriously more readable pattern to one more place: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
Up to @longwave to decide whether to do it here or to create a follow-up and merge this right now 😄
Comment #14
longwaveOpening a followup for #13 because ApiPublishAllControllerTest needs converting from a kernel to a functional test in order for the exception subscriber to kick in properly, and that's turning out trickier than I expected.
Comment #16
longwaveComment #17
wim leersThe follow-up mentioned by @longwave in #14: #3497695: Convert ApiControllerBase::createJsonResponseFromViolationSets() to custom exception and remove ApiControllerBase.