deprecate json_encode() of non-serializable instances #15724
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
there's a good reason for classes being marked as non-serializable, so it follows that the same should probably be true for json_encoding them as well. As this flag is only available to internal classes, no userland code should be affected.
| Makes sense but all deprecations are going through RFC so it needs to wait till 8.5. |
| yes. absolutely. this is meant for 8.5 |
anonymous classes are market as ZEND_ACC_NOT_SERIALIZABLE because unserisalizing them for later use is hard to impossible. However, in the case jon json_encode(), as long as the parent class is serializable, there's very little reason to forbidding to json_encode() anonymous classes, indeed, that's probably very useful in case of userland parent classes.
jorgsowa reviewed Sep 5, 2024
| @@ -0,0 +1,24 @@ | |||
| --TEST-- | |||
| json_encode() on instances of classes market with ZEND_ACC_NOT_SERIALIZABLE | |||
There was a problem hiding this comment.
This test should be located in the corresponding tests directory for json extension, ext/json/tests. And despite the outcome of the discussion on the internal list, I would see the benefit of including the tests now. I was surprised that those paths were not tested already.
| @@ -0,0 +1,24 @@ | |||
| --TEST-- | |||
| json_encode() on instances of classes market with ZEND_ACC_NOT_SERIALIZABLE | |||
There was a problem hiding this comment.
Minor typo
Suggested change
| json_encode() on instances of classes market with ZEND_ACC_NOT_SERIALIZABLE | |
| json_encode() on instances of classes marked with ZEND_ACC_NOT_SERIALIZABLE |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
there's a good reason for classes being marked as non-serializable, so it follows that the same should probably be true for json_encoding them as well.
As this flag is only available to internal classes, no userland code should be affected.
This is the implementation of an RFC I'm currently in the process of authoring after there was some mild agreement about this making sense when I asked on internals.
RFC (in draft): https://wiki.php.net/rfc/deprecate-json_encode-nonserializable