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 serialize IntlTimeZone objects #7939

Closed
drjayvee opened this issue Jan 13, 2022 · 7 comments
Closed

Cannot serialize IntlTimeZone objects #7939

drjayvee opened this issue Jan 13, 2022 · 7 comments

Comments

@drjayvee
Copy link

Description

The following code:

<?php
$tz = IntlTimeZone::createTimeZone('Europe/Amsterdam');
$tz = unserialize(serialize($tz));

var_export($tz->getId());

Resulted in this output:

false

But I expected this output instead:

'Europe/Amsterdam'

It looks like the implementation of __serialize is broken, as the string doesn't contain any object data:

echo serialize(IntlTimeZone::createTimeZone('Europe/Amsterdam'));
// outputs O:12:"IntlTimeZone":0:{}

PHP Version

PHP 7.4.27 (cli) (built: Dec 21 2021 21:31:45) ( NTS )

Operating System

No response

@iluuu1994
Copy link
Member

It doesn't look like IntlTimeZone supports serialization.

https://3v4l.org/jnXqP#v8.1.1

$tz = IntlTimeZone::createTimeZone('Europe/Amsterdam');
var_dump(serialize($tz));
string(24) "O:12:"IntlTimeZone":0:{}"

But we could throw an error or at least emit some warning.

@drjayvee
Copy link
Author

Would it be difficult to implement serialization? Or do you worry about backwards compatibility?

@iluuu1994
Copy link
Member

@drjayvee I know nothing about intl so I'm not sure. Compatibility shouldn't be a concern.

cmb69 added a commit to cmb69/php-src that referenced this issue Jan 15, 2022
As it is now, `IntlTimeZone`, `IntlCalendar` and `IntlDateFormatter`
instances can be serialized, but the representation is meaningless, and
unserialization yields uninitialized/unusable objects.  To prevent
users from noticing this too late, we deny serialization of such
objects in the first place.
@cmb69
Copy link
Member

cmb69 commented Jan 15, 2022

Would it be difficult to implement serialization?

I don't think it would be difficult, but what is the use case? I assume that IntlTimezone is most often used with IntlCalendar and IntlDateFormatter, but neither of these properly supports (un)serialization. Other classes of the intl extension might exhibit the same behavior.

@cmb69 cmb69 linked a pull request Jan 15, 2022 that will close this issue
@drjayvee
Copy link
Author

drjayvee commented Jan 17, 2022

I was under the admittedly naive assumption that every class supports serialization, unless it's especially tricky.

Our particular use case is:

class LocaleConfiguration {
  public IntlTimeZone $timeZone = null;
  public string $locale = 'en-US';
  public int $firstWeekDay = 1;
  // etc
}

And then cache the user's configuration in their session, memory cache, or elsewhere.

The simple yet awkward workaround is

public function __serialize(): array {
  return [
    'timeZone'     => $this->timeZone->getID(),
    'locale'       => $this->locale,
    'firstWeekDay' => $this->firstWeekDay,
    // etc
  ];
}

// reverse for __unserialize()

@cmb69
Copy link
Member

cmb69 commented Jan 17, 2022

Okay, for this case being able to (un)serialize IntlTimeZone objects would make sense.

Note, though, that a lot of internal classes don't support (un)serialization.

cmb69 added a commit to cmb69/php-src that referenced this issue Jan 18, 2022
As it is now, `IntlTimeZone`, `IntlCalendar` and `IntlDateFormatter`
instances can be serialized, but the representation is meaningless, and
unserialization yields uninitialized/unusable objects.  To prevent
users from noticing this too late, we deny serialization of such
objects in the first place.
@cmb69 cmb69 closed this as completed in f06ac9a Feb 17, 2022
@cmb69
Copy link
Member

cmb69 commented Feb 17, 2022

Serialization of these objects will be disallowed as of PHP 8.2.0. PRs to properly support (un)serialization (of individual classes) are welcome!

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

Successfully merging a pull request may close this issue.

3 participants