Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot hydrate a DateTime instance created without the constructor #8152

Closed
nicolas-grekas opened this issue Feb 24, 2022 · 13 comments
Closed

Comments

@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Feb 24, 2022

Description

This report applies to DateTime, DateTimeImmutable, DateTimeZone, DateInterval and DatePeriod.

Before PHP 8.2, there used to be a way to create a DateTime object from an uninitialized prototype:

$date = (new \ReflectionClass('DateTime'))->newInstanceWithoutConstructor();
$date->date = '1970-01-01 00:00:00.000000';
$date->timezone_type = 1;
$date->timezone = '+00:00';
$date->__wakeup();

These 3 properties do not exist on DateTime, but they're still returned by serialize($date), that's how I figured out how to hydrate the previous object. And it works perfectly. But since PHP 8.2, a deprecation is triggered because dynamic properties are not allowed.

This means that since 8.2, there is no way to hydrate a DateTime instance that is created without the constructor.

This is an issue in symfony/var-exporter for example, which is a library that provides a way to serialize objects to native PHP code (instead of regular serialization strings.) I cannot figure out a way to make this lib support PHP 8.2 while not triggering a warning.

Note that the engine allows itself to create those dynamic properties even on PHP 8.2. Here is some code to highlight this:

$d = new DateTime();
var_dump($d->date);

$e = unserialize(serialize($d));
var_dump($e->date);

This outputs:

Warning: Undefined property: DateTime::$date in /in/EUrC5 on line 4
NULL
string(26) "2022-02-24 22:26:25.005040"

A simple fix would be to add #[AllowDynamicProperties] to those classes.
Another fix would be to actually declare the corresponding properties as protected.

WDYT?

@nikic
Copy link
Member

nikic commented Feb 24, 2022

Why not use DateTime::__set_state(['date' => ...])?

Though I do think that we also need to migrate away from __wakeup to __unserialize to avoid the reliance on temporary dynamic properties. And unserialization is clearly missing some checks in this area.

@nicolas-grekas
Copy link
Contributor Author

nicolas-grekas commented Feb 24, 2022

I could work with that I think, but then I figured this doesn't work with child classes of eg DateTime:

class Foo extends DateTime
{
}

echo get_class(Foo::__set_state([
    'date' => '1970-01-01 00:00:00.000000',
    'timezone_type' => 1,
    'timezone' => '+00:00',
]));

Displays DateTime.

@mvorisek
Copy link
Contributor

I cannot figure out a way to make this lib support PHP 8.2 while not triggering a warning.

you can set dummy error handler and remove it after the object is hydrated

@nicolas-grekas
Copy link
Contributor Author

you can set dummy error handler and remove it after the object is hydrated

Well, that's not a proper solution. It will break on PHP 9 if we don't do anything else.

Something that works for DateTime* even when dealing with child classes is to call their constructor via reflection:

class Foo extends DateTime
{
}

$foo = new Foo();

(new ReflectionMethod('DateTime', '__construct'))->invoke($foo, ...)

But for DateInterval and DatePeriod, this doesn't work because their state depends on the way they've been created (using DateTime::diff() or not, etc.)

Another solution would be to implement __serialize/__unserialize in PHP 8.2. We'd still have to try to ensure that pre8.2 payloads are still serializable.

@bwoebi
Copy link
Member

bwoebi commented Feb 25, 2022

Why wouldn't it make sense to declare these properties (without initializing them)? So that writing to them is not a dynamic property access.

I see no concrete disadvantages to that.

@nikic
Copy link
Member

nikic commented Feb 27, 2022

Another solution would be to implement __serialize/__unserialize in PHP 8.2. We'd still have to try to ensure that pre8.2 payloads are still serializable.

That would be my preference. Pre-8.2 payloads should still unserialize fine, because the __wakeup and __unserialize formats are compatible.

@derickr derickr self-assigned this Apr 1, 2022
@derickr
Copy link
Contributor

derickr commented Apr 7, 2022

Some historical perspective:

  • These properties were never meant to be public — as setting these would imply that they would modify the underlying data (they don't).
  • They were added to be displayed for debugging and tests (through var_dump() and friends).
  • Somebody later added serialisation support, and __wakeup() etc.
  • Using __wakeup() to magically make the DateTime properties do something is IMO also a hack.

None of the DateTime* classes should have publicly writable properties, and the right way to hydrate these objects is/was by using __set_state.

I do find it odd that Foo::__set_state() wouldn't return a Foo class though, and I think I consider that a bug. There is however no other built-in class that implements __set_state() in the standard distribution.

@derickr
Copy link
Contributor

derickr commented Apr 14, 2022

FWIW, I've started adding the __serialize/__unserialize methods for the Date classes.

@derickr
Copy link
Contributor

derickr commented Apr 22, 2022

PR 1: #8422

@derickr
Copy link
Contributor

derickr commented Apr 29, 2022

The DateTime/DateTimeImmutable/DateTimeZone part is now merged.

@derickr
Copy link
Contributor

derickr commented Apr 29, 2022

PR 2: #8459 (for DateInterval)

@derickr
Copy link
Contributor

derickr commented Apr 29, 2022

PR 3: #8464 (for DatePeriod)

@derickr
Copy link
Contributor

derickr commented May 5, 2022

All the classes are now covered, and their PRs merged, so closing this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants
@derickr @nikic @nicolas-grekas @mvorisek @cmb69 @bwoebi and others