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

fix(symfony): context not serializable when session #6302

Merged

Conversation

MariusJam
Copy link
Contributor

Q A
Branch? 3.1
Tickets Closes api-platform/api-platform#2611
License MIT

In some case, a "Serialization of 'Closure' is not allowed" Exception is thrown when using Messenger. This is due to the fact that the $context array is passed to the Envelope via the ContextStamp and that $context['request'] is set with a not serializable Request object.

Symfony does not guarantee that a Request is serializable so the best solution would be to completely unset $context['request'] in the ContextStamp. However this would be a breaking change. So, I'm proposing an alternative solution by unsetting $context['request'] only when the Request object has a session as it seems to be the identified buggy case.

@soyuka
Copy link
Member

soyuka commented Apr 9, 2024

Interesting, can't this be fixed in Symfony? Is there a way to set a serializer context when using messenger?

@MariusJam
Copy link
Contributor Author

MariusJam commented Apr 10, 2024

can't this be fixed in Symfony?

I was wondering as well. I asked in Symfony's Slack support channel and the answer was : "Symfony does not guarantee that the Request object is serializable. So passing that in a messenger enveloppe will indeed not work. API-Platform should be careful about that". Hence the PR here.

Is there a way to set a serializer context when using messenger?

It might be possible but that would require to change the Messenger's default configuration. According to Symfony' doc, the serialization/deserialization of messages use the php's serialize() and unserialize() functions by default, not the Serializer component.

I guess changing this default behaviour could be possible but this would be a more major change...

@soyuka
Copy link
Member

soyuka commented Apr 11, 2024

I'm afraid that other parts of the request would be unserializable (we have closures in its attributes sometime). What about removing request alltogether? Another concern is that it could break current implementations, maybe that we should put this on the main branch instead? (expect a release this month)

@MariusJam
Copy link
Contributor Author

Yes that's the risk indeed. That's why I was proposing to already fix the case on 3.1/3.2 branches, but only when there is a session : we know it creates a bug so we should not break anything as it already fails, or I'm missing something 🤔

On the main branch, I can indeed make another PR to completely remove the request.

Would you be okay with that or you prefer just a PR on the main branch?

@soyuka
Copy link
Member

soyuka commented Apr 12, 2024

👍 I fixed the CI on 3.1, I'll re-run your tests after and if all green I merge, expect a release following that

@soyuka soyuka force-pushed the fix/messenger-context-not-serializable branch 2 times, most recently from cfb662c to 9411b2e Compare April 13, 2024 06:41
@soyuka soyuka merged commit 2c25293 into api-platform:3.1 Apr 13, 2024
21 of 26 checks passed
soyuka added a commit that referenced this pull request Apr 13, 2024
* fix(symfony): context not serializable when session

* phpstan

---------

Co-authored-by: soyuka <soyuka@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants