Replace __sleep/__wakeup with __serialize/__unserialize#2962
Replace __sleep/__wakeup with __serialize/__unserialize#2962GromNaN merged 1 commit intodoctrine:3.0.xfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the codebase by replacing the soft-deprecated __sleep and __wakeup magic methods with their more verbose counterparts __serialize and __unserialize, following PHP's RFC for soft-deprecation.
Key Changes:
- Migration from
__sleep/__wakeupto__serialize/__unserializein serialization logic - Enhanced serialization methods that return associative arrays with explicit key-value pairs instead of property name arrays
- Updated return type annotations to reflect the new structure (
array<string, mixed>instead ofstring[])
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/PersistentCollection/PersistentCollectionTrait.php |
Converted __sleep to __serialize, now returning explicit key-value pairs for collection state |
src/Mapping/ClassMetadata.php |
Replaced __sleep/__wakeup with __serialize/__unserialize, restructuring conditional serialization logic to use associative array assignments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * | ||
| * @return array The names of all the fields that should be serialized. | ||
| */ | ||
| public function __sleep(): array |
There was a problem hiding this comment.
This methods should be marked as @internal or @deprecated in 2.x? They should never be called directly or overridden.
There was a problem hiding this comment.
ClassMetadata is already marked as @final but we should also mark these methods as @deprecated to avoid direct calling them.
There was a problem hiding this comment.
Generally, I'd argue that the magic methods are not meant to be called directly, and thus shouldn't need @internal or @deprecated. Instead, these methods define behaviour of the class which in this case doesn't change -- we're just changing the points where we hook in.
As for a potential compatibility layer in v2, we may want to rely on deprecation notices in certain places to highlight upcoming changes.
| continue; | ||
| } | ||
|
|
||
| if (method_exists($mapping['collectionClass'], '__sleep')) { |
There was a problem hiding this comment.
IMO, this should be in the generator class, as this command is typically not called in development environments. We also need to consider a compatibility layer in v2 to allow people to be notified of the upcoming change in v3 and implement __serialize instead.
There was a problem hiding this comment.
I realize that this trait is added on generated classes that extends user defined classes. The __sleep method is alreadyoverridden by the trait one. So, not functioning. I'll remove this error.
| ]; | ||
|
|
||
| // The rest of the metadata is only serialized if necessary. | ||
| if ($this->changeTrackingPolicy !== self::CHANGETRACKING_DEFERRED_IMPLICIT) { |
There was a problem hiding this comment.
I'm wondering if it's worth keeping the conditionals around. Keeping a map of fields to serialise would also make the unserialisation more secure (see comment below).
| foreach ($data as $field => $value) { | ||
| $this->$field = $value; | ||
| foreach ($data as $property => $value) { | ||
| $this->$property = $value; |
There was a problem hiding this comment.
Not sure how big an issue this still is, but this would technically allow people to override properties that we don't actually serialise or intend to read from serialised data. Paired with the comment above, having a map of all properties we want to serialise would allow us to re-use this map and only use data from fields that we know should be serialised.
There was a problem hiding this comment.
Unserialization is in the hotpath. I think it's an optimization to only serialize the data that is different from the default value.
There was a problem hiding this comment.
As for overriding private properties that are not part of the metadata, that's a stretch. We can also use reflection.
There was a problem hiding this comment.
Sounds good -- since we're not making it any worse I'm fine leaving this as-is.
src/Tools/Console/Command/GeneratePersistentCollectionsCommand.php
Outdated
Show resolved
Hide resolved
Summary
The magic methods
__sleepand__wakeupare being soft-deprecated (RFC). Replaced by__serializeand__unserialize. This new methods are more verbose.Similar to symfony/symfony#61727