-
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
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?
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" __sleep() or __wakeup() so as far as I am concerned both should still be deprecated. I am willing to make a concession to postpone the __wakeup() deprecation to a future version so that Symfony doesn't need to deal with for Symfony 7 as this is a non issue for Symfony 8 as it uses __serialize() and __unserialize().
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 __wakeup(); I don't see why __sleep() should be postponed. But if it needs to be an all or nothing, then what was voted on was to deprecate both of them, not none of them.
There has not been a counter RFC for "undeprecating"
__sleep()or__wakeup()so as far as I am concerned both should still be deprecated. I am willing to make a concession to postpone the__wakeup()deprecation to a future version so that Symfony doesn't need to deal with for Symfony 7 as this is a non issue for Symfony 8 as it uses__serialize()and__unserialize().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
__wakeup(); I don't see why__sleep()should be postponed. But if it needs to be an all or nothing, then what was voted on was to deprecate both of them, not none of them.
Please see https://news-web.php.net/php.internals/128659 for the RMs' position, __sleep() and __wakeup() should both be deprecated before RC1
Please see https://news-web.php.net/php.internals/128659 for the RMs' position,
__sleep()and__wakeup()should both be deprecated before RC1
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
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
Uh oh!
There was an error while loading. Please reload this page.
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.