Skip to content

Implement __serialize() and __unserialize()#8895

Merged
greg0ire merged 1 commit intodoctrine:2.9.xfrom
derrabus:bugfix/serializable
Aug 15, 2021
Merged

Implement __serialize() and __unserialize()#8895
greg0ire merged 1 commit intodoctrine:2.9.xfrom
derrabus:bugfix/serializable

Conversation

@derrabus
Copy link
Copy Markdown
Member

@derrabus derrabus commented Aug 7, 2021

The Serializable interface is deprecated in favor of the magic methods __serialize() and __unserialize(). PHP 8.1 will trigger a deprecation warning if a class implements Serializable but not those two magic methods.

This PR addresses this issue by adding __serialize() and __unserialize() to the SequenceGenerator class.

This does however change the serialized string on PHP 7.4 and above. I don't know if that is a problem.

@derrabus
Implement __serialize() and __unserialize()
ae4bcd6 
Signed-off-by: Alexander M. Turek <me@derrabus.de>
@derrabus derrabus force-pushed the bugfix/serializable branch from 885256e to ae4bcd6 Compare August 7, 2021 23:24
greg0ire
greg0ire approved these changes Aug 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know either if the string change will be an issue.

SenseException
SenseException approved these changes Aug 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this string be somehow shared between versions for any reason in a project? Otherwise I don't see an issue with this.

@beberlei
Copy link
Copy Markdown
Member

beberlei commented Aug 14, 2021 •
edited
Loading

This could be problematic imho in a patch release as we don't have the version number in the cache key of Metadata Cache. What would happen if on PHP 7.4 this was serialized before this change and then unserialized after this change? Will that work?

@beberlei
Copy link
Copy Markdown
Member

beberlei commented Aug 15, 2021

Tested it and it still works to unserialize an old string, so good to go from my POV.

@derrabus
Copy link
Copy Markdown
Member Author

derrabus commented Aug 15, 2021

Unserializing old strings still works as long as we keep the Serializable implementation. But an object serialized with PHP 7.4 won't be unserializable for PHP 7.3 anymore.

@greg0ire greg0ire added this to the 2.9.5 milestone Aug 15, 2021
@greg0ire greg0ire merged commit d636d79 into doctrine:2.9.x Aug 15, 2021
@greg0ire
Copy link
Copy Markdown
Member

greg0ire commented Aug 15, 2021

Thanks @derrabus !

@github-actions github-actions bot mentioned this pull request Aug 23, 2021
Merged
greg0ire added a commit that referenced this pull request Aug 23, 2021
@greg0ire
Merge tag '2.9.5' into 2.9.x-merge-up-into-2.10.x_Ryg07QLG
4f9c104 
2.9.x bugfix release (patch)

- Total issues resolved: **0**
- Total pull requests resolved: **2**
- Total contributors: **2**

 - [8930: Introduce 2.10 to readme](#8930) thanks to @SenseException

 - [8895: Implement &#95;&#95;serialize() and &#95;&#95;unserialize()](#8895) thanks to @derrabus
@derrabus derrabus deleted the bugfix/serializable branch January 17, 2022 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@beberlei beberlei beberlei approved these changes

@greg0ire greg0ire greg0ire approved these changes

@SenseException SenseException SenseException approved these changes

Assignees

No one assigned

Labels

Improvement

Projects

None yet

Milestone

2.9.5

Development

Successfully merging this pull request may close these issues.

4 participants

@derrabus @beberlei @greg0ire @SenseException