Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Messenger] Clearer exception when ObjectNormalizer is missing #31983

Open
wants to merge 1 commit into
base: 4.4
from

Conversation

Projects
None yet
5 participants
@TimoBakx
Copy link
Contributor

commented Jun 10, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? kinda
Deprecations? no
Tests pass? yes
Fixed tickets #29163
License MIT
Doc PR

When using Messenger with the Symfony Serializer component directly (without the pack), dispatching or consuming messages will generate an exception, due to ObjectNormalizer not being available. I added exceptions to point developers that run into this problem to a possible sollution.

@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-serializer-error branch 3 times, most recently from 2e60095 to 7b605df Jun 10, 2019

@@ -119,6 +127,8 @@ private function decodeStamps(array $encodedEnvelope): array
try {
$stamps[] = $this->serializer->deserialize($value, substr($name, \strlen(self::STAMP_HEADER_PREFIX)).'[]', $this->format, $this->context);
} catch (NotNormalizableValueException $e) {

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 11, 2019

Member

But this is denormalization at this stage, right?

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Jun 11, 2019

Author Contributor

True, but it's still a NotNormalizableValueException that's thrown there: https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Serializer/Serializer.php#L194

@@ -81,8 +83,10 @@ public function decode(array $encodedEnvelope): Envelope
try {
$message = $this->serializer->deserialize($encodedEnvelope['body'], $encodedEnvelope['headers']['type'], $this->format, $context);
} catch (NotNormalizableValueException $e) {
throw new MessageDecodingFailedException('No denormalizer found to decode message. Try running "composer require symfony/property-access".', $e->getCode(), $e);

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 11, 2019

Member

I was picturing that we actually just required property-access to be installed. Like, in the if statement above around line 53, we also check for some class in property-access and throw an exception if it's not there. That solution is simpler, your solution is fancier. If we go your direction (not sure which is better), we should at least check to see if property-access exists before throwing an exception saying to install it. I'm also not convinced (but not sure) that we need a new exception class for this - it feels strange to allow someone to "catch" this exception, when I think it will always be a misconfiguration/bug.

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Jun 11, 2019

Author Contributor

I agree with not needing an extra Exception.
But: Throwing a MessageDecodingFailedException when encoding is worse, imho.
We could use a more generic Exception. Which do you think would fit best?

Just checking for the existence of PropertyAccessor itself (like it is done for Serializer) seems like a bad idea, because one could technically write their own normalizer and not have PropertyAccessor installed, making the code work properly.
The only reason this case would happen is if no (de)normalizer installed supports objects. And that's only the case if symfony/serializer is installed and symfony/property-access isn't (as that would actively disable the ObjectNormalizer in the serializer compiler pass). And no other (de)normalizer supports objects. In that case, this would be an out-of-the-box solution to it.

The situation where no suported normalizers exist AND symfony/property-access is installed is never possible.

This comment has been minimized.

Copy link
@chalasr

chalasr Jun 11, 2019

Member

We could use a more generic Exception. Which do you think would fit best?

We used to throw a LogicException for such cases. Messenger has one, should be a good fit.

Just checking for the existence of PropertyAccessor itself (like it is done for Serializer) seems like a bad idea, because one could technically write their own normalizer and not have PropertyAccessor installed, making the code work properly.

Although it's simpler, I tend to agree that closing the door for custom object normalizers feels like an unjustified limitation. But I'm not strongly against it if we think it's good enough.

The situation where no suported normalizers exist AND symfony/property-access is installed is never possible.

If we go that way (open the door for an object normalization mechanism that is not based on property-access), one could have specific per-object-type normalizers and just forgot to create one for a newly added message, so I'd say it's worth checking that property-access is not installed before suggesting to install it.

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Jun 11, 2019

Author Contributor

I'm not sure checking specifically for it would add any value, though. The exception will never be thrown if it exists.

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Jun 12, 2019

Author Contributor

Changed the exception thrown to Symfony\Component\Messenger\Exception\LogicException (the same one is thrown in the constructor when Serializer is missing).

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Jun 12, 2019

Author Contributor

Ah, trying to decode a type that doesn't exist also throws the same exception. But that shouldn't trigger this extra exception message.

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Jun 12, 2019

Author Contributor

I updated the code to check for both an exception message (as this is currently the only way to check the specific missing normalizer) and the presence of PropertyAccessor.

Is this the correct way to go? Should I also implement this in the encoding methods?

Thanks for all your feedback. 👍

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 12, 2019

@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-serializer-error branch 3 times, most recently from f9f4732 to 56458d7 Jun 12, 2019

@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-serializer-error branch from 56458d7 to 2745c23 Jun 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.