Throw exceptions instead of returning tuples for validation errors

In fact Symfony already provides Symfony\Component\Validator\Exception\ValidationFailedException which does exactly what we need.

I didn't know about Symfony\Component\Validator\Exception\ValidationFailedException. seems like a good idea

Just merged 📌 Move SDC specific logic out of ClientsideConversionTrait into component source plugins Active so will need to resolve conflicts

Didn't even use ValidationFailedException, we already have a specific ConstraintViolationException in this codebase!

Conflicts resolved, also added missing return types for 📌 Move SDC specific logic out of ClientsideConversionTrait into component source plugins Active given I'm touching these lines anyway.

Looks good to me. Assign to @wimleers because he or @f.mazeikis needs to approve this MR.

Didn't even use ValidationFailedException, we already have a specific ConstraintViolationException in this codebase!

don't have to do this in this issue but do we actually need `ConstraintViolationException`? couldn't we just useSymfony\Component\Validator\Exception\ValidationFailedException?

wim leers → credited larowlan → .

Found 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 ValidationFailedException yet 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...

I 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.

re #10, Yeah I agree the benefit of having something like `violationWithPropertyPathReplacePrefix` on ConstraintViolationException seem more having our own exception class

Added the remapping code here, no need for the followup.

RTBC'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 😄

Opening 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.

The follow-up mentioned by @longwave in #14: 📌 Convert ApiControllerBase::createJsonResponseFromViolationSets() to custom exception Active .

Automatically closed - issue fixed for 2 weeks with no activity.