-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Zend: Deprecate __sleep() #19682
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
Zend: Deprecate __sleep() #19682
Conversation
adadb0d
to
a4e917e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks trivial
Zend/zend_compile.c
Outdated
} | ||
|
||
if (ce->__serialize == NULL && zend_hash_exists(&ce->function_table, ZSTR_KNOWN(ZEND_STR_SLEEP))) { | ||
zend_error(E_DEPRECATED, "The __sleep() serialization hook has been deprecated." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this called a hook? I thought it was just a magic method. Hook may be confusing wrt property hooks.
Although docs say on one particular page:
Beyond the above advice, note that you can also hook into the serialization and unserialization events on an object using the __sleep() and __wakeup() methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess using magic method is less ambiguous. But I did use "hook" because it hooks into the serialization process.
Deprecating __sleep and keeping __wakeup seems kind of wierd and not what was voted on. I think this should be re-considered. But will leave it to RM to decide it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecating __sleep and keeping __wakeup seems kind of wierd and not what was voted on. I think this should be re-considered. But will leave it to RM to decide it.
So I'm a little torn. On the one hand, if the plan is still to deprecate __wakeup
and it is just the implementation that is being split up, then it would be fine and I would just want to make sure that the implementation is also done in 8.5 since those two methods go together.
If the __wakeup
part is not ready yet, then it might be worth delaying the __sleep
part so that they are merged together (same PHP version). We have precedent (from yours truly) for implementing deprecations after the version they targeted (8.4 deprecations for output handlers were done in 8.5).
On the other hand, if __wakeup
is not planned, then this is clearly something that was not voted on - the vote for deprecation was 18-9, so if even one of those 18 would have objected to deprecating __sleep
without also deprecating __wakeup
then the proposal would have failed. I can see an argument that keeping __wakeup
allows deserialization of objects serialized with the old method, while deprecating __sleep
means that there wouldn't be new objects serialized with the old method, but still, I would expect that at least someone would have objected.
So, I guess the question is, are you planning to also deprecate __wakeup
in this release?
if (ce->__serialize == NULL && zend_hash_exists(&ce->function_table, ZSTR_KNOWN(ZEND_STR_SLEEP))) { | ||
zend_error(E_DEPRECATED, "The __sleep() serialization magic method has been deprecated." | ||
" Implement __serialize() instead (or in addition, if support for old PHP versions is necessary)"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test for what happens if the deprecation is turned into an exception?
There has not been a counter RFC for "undeprecating" Considering the analysis from @theodorejb (and the results I have from Damien) shows that the main project being impacted is SF and, from what I understand, the problem is with |
Please see https://news-web.php.net/php.internals/128659 for the RMs' position, |
I would like to merge this and then rebase the other PR on top of it with the new wording. Can I do so? |
cd1facc
to
d9a0b65
Compare
ext/standard/tests/serialize/sleep_deprecation_promoted_exception.phpt
Outdated
Show resolved
Hide resolved
|
||
?> | ||
--EXPECTF-- | ||
Deprecated: The __sleep() serialization magic method has been deprecated. Implement __serialize() instead (or in addition, if support for old PHP versions is necessary) in %s on line %d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there isn't an error about an uncaught exception, so is this actually testing the promotion from deprecation to exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the deprecation is not being promoted, maybe @arnaud-lb or @nielsdos know why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that the class definition is being “hoisted” since it is unconditionally defined. Try wrapping the class into an if (random_int(1, 1))
or defining it via eval()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... early binding...
d76578f
to
c240b29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RM approval with the understanding that #19435 will be updated and merged shortly thereafter, i.e. both __sleep() and __wakeup() will get deprecated and the patches are just split up to simplify things
This reverts commit f18e992.
This reverts commit f18e992.
This reverts commit f18e992.
This reverts commit f18e992.
This reverts commit f18e992.
RFC: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_the_sleep_and_wakeup_magic_methods
Split from #19435 as
__sleep()
has an extremely easy migration path.