Fixes GH-8152: add __serialise and __unserialise methods to DateTime and DateTimeZone - #8422
Conversation
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Other than the return type of the function which needs to be changed from zend_result to bool LGTM, will leave some time for Nicolas to also have a look.
There was a problem hiding this comment.
Is there a test to see if a crafted serialisation will trigger this warning?
There was a problem hiding this comment.
I don't think so, but will check and add one if needed.
There was a problem hiding this comment.
An idea for later, it seems similar code to this is present in other functions, maybe extract this into it's own function?
There was a problem hiding this comment.
It is something you need in nearly every serialise handler, not only in the Date/Time ones. It's a little tricky to extract, as it has both a break (do nothing) and has to return something. I'll make a note and see if this can be done better at some point.
There was a problem hiding this comment.
This can crash if the __constructor was not called:
class D extends DateTime { public function __construct() {} }
serialize(new D());
There was a problem hiding this comment.
I don't think it can, as the __serialize handler for "D" does not call the __serialize handler for "DateTime".
However, the following does crash:
<?php
class D extends DateTime {
public function __construct() {}
public function __serialize() : array {
return parent::__serialize();
}
}
echo serialize(new D());
Will add a guard, good catch!
|
This works nicely with symfony/var-exporter, thanks! |
97db8b3 to
6f2c501
Compare
…icolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [VarExporter] Fix exporting DateTime objects on PHP 8.2 | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Needs php/php-src#8422 Commits ------- a09e2b4 [VarExporter] Fix exporting DateTime objects on PHP 8.2
4 participants
This adds the __serialize() and unserialize() methods to DateTime, DateTimeImmutable, and DateTimeZone for PHP 8.2 and later.
This specific PR does not touch on DateInterval and DatePeriod yet, because these cases are more complicated. I will make a separate PR, which might need to end up breaking BC (albeit for functionality that didn't work as expected anyway).
/cc @nicolas-grekas