8000 [Messenger] Fix `TransportNamesStamp` deserialization by tucksaun · Pull Request #49548 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Messenger] Fix TransportNamesStamp deserialization #49548

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

Conversation

tucksaun
Copy link
Contributor
@tucksaun tucksaun commented Feb 27, 2023
Q A
Branch? 6.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets #49574, #31490 (comment)
License MIT
Doc PR n/a

Currently, when ones use TransportNameStamp the following exception can occur if they don't use native PHP serialization:

In Serializer.php line 125:

  [Symfony\Component\Messenger\Exception\MessageDecodingFailedException]
  Could not decode stamp: Cannot create an instance of "Symfony\Component\Messenger\Stamp\TransportNamesStamp" from serialized data because its constructor requires parameter "transports" to be present.

In AbstractNormalizer.php line 384:

  [Symfony\Component\Serializer\Exception\MissingConstructorArgumentsException]
  Cannot create an instance of "Symfony\Component\Messenger\Stamp\TransportNamesStamp" from serialized data because its constructor requires parameter "transports" to be present.

This PR renames TransportNamesStamp constructor argument in order to match the accessor method (getTransportNames) so that deserialization works when using the Serializer.

I know this is technically a (small) BC break but Symfony's BC does not cover named arguments if I remember correctly.

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | symfony#31490 (comment)
| License       | MIT
| Doc PR        | n/a

Currently, when ones use `TransportNameStamp` the following exception occurs:

```
In Serializer.php line 125:

  [Symfony\Component\Messenger\Exception\MessageDecodingFailedException]
  Could not decode stamp: Cannot create an instance of "Symfony\Component\Messenger\Stamp\TransportNamesStamp" from serialized data because its constructor requires parameter "transports" to be present.

In AbstractNormalizer.php line 384:

  [Symfony\Component\Serializer\Exception\MissingConstructorArgumentsException]
  Cannot create an instance of "Symfony\Component\Messenger\Stamp\TransportNamesStamp" from serialized data because its constructor requires parameter "transports" to be present.

```

This PR renames `TransportNamesStamp` constructor argument in order to match the accesor method (`getTranspdortNames`) so that deserialization work.

I know this is technically a BC break but as far as I can tell the feature can not currently work this way and also named arguments are not covered by Symfony's BC if I remember correctly.
@StephanMeijer
Copy link

Thanks for fixing this. You were just a little quicker than me. Also seen this yesterday.

@StephanMeijer
Copy link

A suggestion: would it be logical to typehint the getter with @return string[] so IDE's know that it returns an array of strings? Might be useful.

@OskarStark OskarStark changed the title [Messenger] Fix TransportNamesStamp deserialization [Messenger] Fix TransportNamesStamp deserialization Mar 2, 2023
@tsantos84
Copy link
Contributor

Hi guys, thanks for this PR. Once this PR gets merged, how long it'll take to release a new version?

@nicolas-grekas
Copy link
Member

We do a release every month @tsantos84 so you can expect one in a bit less than 4 weeks.

@nicolas-grekas
Copy link
Member

Good catch, thanks @tucksaun.

@nicolas-grekas nicolas-grekas merged commit fd8b883 into symfony:6.2 Mar 6, 2023
@fabpot fabpot mentioned this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0