-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Allows JSON unserializing to be associative or object #28192
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
Conversation
Hi @jajajaime. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
I remember we had some architecture discussions in magento/architecture#254, magento/magento-coding-standard#91 and previously in magento/architecture#161. AFAIK no real decision was done yet. @lenaorobei do we have any updates on that? |
@@ -32,9 +32,9 @@ public function serialize($data) | |||
* @inheritDoc | |||
* @since 100.2.0 | |||
*/ | |||
public function unserialize($string) | |||
public function unserialize($string, $assoc = true) |
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.
Such change is not allowed: https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/
Let's first find out whether we need such functionality at all.
@orlangur please explain why you moved this PR to "on hold" status. |
Unfortunately, we cannot accept this PR since it violates backward compatibility policy. There were some discussions regarding introduction new JSON contract, but there is no resolution yet. Please see details here magento/architecture#254 |
Hi @jajajaime, thank you for your contribution! |
@lenaorobei is this because of "Adding parameters in public methods"? If so, I think it should be on a case by case basis, as creating a new class or interface would create more work for devs to switch to the new one in this case. |
@jajajaime backward compatibility policy is mandatory, no exception.
Please create a proposal in architecture repo first to describe why it needs to be returned as object. |
Description (*)
Allows a developer to pass an optional boolean argument when unserializing a JSON string, so it can be returned as object or associative. Keeps associative as default for backwards compatibility.
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)