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.
| const char *orig_tz = tz; | ||
|
|
||
| if (strlen(tz) != tz_len) { | ||
| php_error_docref(NULL, E_WARNING, "Timezone must not contain null bytes"); |
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.
| switch (purpose) { | ||
| case ZEND_PROP_PURPOSE_DEBUG: | ||
| case ZEND_PROP_PURPOSE_SERIALIZE: | ||
| case ZEND_PROP_PURPOSE_VAR_EXPORT: | ||
| case ZEND_PROP_PURPOSE_JSON: | ||
| case ZEND_PROP_PURPOSE_ARRAY_CAST: | ||
| break; | ||
| default: | ||
| return zend_std_get_properties_for(object, purpose); | ||
| } | ||
|
|
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.
| dateobj = Z_PHPDATE_P(object); | ||
|
|
||
| array_init(return_value); | ||
| myht = Z_ARRVAL_P(return_value); | ||
| date_object_to_hash(dateobj, myht); |
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! |
derickr
force-pushed
the
GH-8152-add-serialise-and-unserialise-datetime-and-datetimezone
branch
from
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