Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Don't make EnvelopeItemInterface extend Serializable#28247
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
linaori commentedAug 23, 2018
Should this be done in 5.0 for other classes as well? I haven't done a full search across the codebase, but I know that at least the TokenInterface does this. symfony/src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php Line 22 in6a4de22
|
nicolas-grekas commentedAug 23, 2018
@iltar yep, but unfortunately those are not experimental anymore. I'd love to remove the "extends" also. Can we? |
linaori commentedAug 23, 2018
I'm not sure if the serialize/unserialize methods will be calledwithout the interface. What wecould do, is implement the methods in the interface extending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, we tried and already see the limits (like in#28164 for example) so let's remove theSerializable (so we can even provide the opportunity to use the Symfony Serializer instead of(un)serialize 👌)
@nicolas-grekas can you update the CHANGELOG & UPGRADE files?
842da1d tocd9ff9dComparenicolas-grekas commentedAug 24, 2018
CHANGELOG & UPGRADE updated |
cd9ff9d to2beda89Compare
ogizanagi left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas : Purpose was to keep serialization under control and make users think about how to serialize it reliably. Otherwise, PHP serialization is a mess in case of refactoring and for BC layers.
But actually, I'm fine with removing it, you're right.
fabpot commentedAug 27, 2018
Thank you@nicolas-grekas. |
…rializable (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Don't make EnvelopeItemInterface extend Serializable| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes (on experimental API)| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -`Serializable` is a broken interface, see e.g.https://externals.io/message/98834I don't think we should force ppl to implement it anywhere.Actually, it isn't required to be able to serialize an object, and it doesn't enforce making a class serializable (as the `serialize()` method can throw).What was the purpose of the removed logic? ping@sroze@ogizanagiCommits-------2beda89 [Messenger] Don't make EnvelopeItemInterface extend Serializable
…elope items (sroze)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Uses Symfony Serializer by default for envelope items| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets |#28164| License | MIT| Doc PR | øThe original approach was to use `serialize`/`unserialize` for envelope items. It turns out it has limitations (see#28247) and reduces the compatibility with 3rd party systems (see#28164). This pull-request changes the existing mechanism by using Symfony Serializer by default.It keeps the `serialize`/`unserialize` mechanism to allow users of the experimental component to keep using it in 4.2 and allow to have a non-disruptive upgrade to 4.2.Commits-------9b575ab Uses Symfony Serializer by default for envelope items
Uh oh!
There was an error while loading.Please reload this page.
Serializableis a broken interface, see e.g.https://externals.io/message/98834I don't think we should force ppl to implement it anywhere.
Actually, it isn't required to be able to serialize an object, and it doesn't enforce making a class serializable (as the
serialize()method can throw).What was the purpose of the removed logic? ping@sroze@ogizanagi