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

Add support of JSON_THROW_ON_ERROR global constant #168

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@SerafimArts
Copy link

SerafimArts commented Feb 6, 2019

Added support of the JSON_THROW_ON_ERROR constant which was added in PHP 7.3

https://secure.php.net/manual/en/json.constants.php#constant.json-throw-on-error

@stof

This comment has been minimized.

Copy link
Member

stof commented Feb 7, 2019

-1 for that. Adding the constant without adding its behavior looks wrong to me. It forbids using feature detection based on the constant to know whether the constant can be relied on.

This PR is not polyfilling support for JSON_THROW_ON_ERROR

@stof

This comment has been minimized.

Copy link
Member

stof commented Feb 7, 2019

btw, this was already discussed in #161

@SerafimArts

This comment has been minimized.

Copy link
Author

SerafimArts commented Feb 7, 2019

This PR is not polyfilling support for JSON_THROW_ON_ERROR

If follow your logic, then this package (symfony/polyfill) does not implement a single full polyfill at all. And since you argue that full support of the functional is required in order to support some part, it is necessary to deactivate the whole package at all.

I think this is too radical statements.

symfony/polyfill is needed to ensure the implementation of the functionality that can be implemented by means of PHP. It does not guarantee that all the functionality and syntax of the new version of the language will immediately work after connecting the package.

Given this, I propose to implement the functionality that is possible to implement.

@jvasseur

This comment has been minimized.

Copy link

jvasseur commented Feb 7, 2019

@SerafimArts but what use does the constant have when you can't use it to toggle the associated feature? I'm 👎 here because it adds an mostly useless constant while bringing harm by preventing feature detection/silently ignoring bugs.

@SerafimArts

This comment has been minimized.

Copy link
Author

SerafimArts commented Feb 7, 2019

@SerafimArts but what use does the constant have when you can't use it to toggle the associated feature?

Exactly the same as from JsonException exception class, which never happens or from these.

And why does anyone need a polyfill of PHP 7.3, in which there are no constants added in PHP 7.3?

For example, I use these constants and ensure that they work: https://github.com/railt/json/blob/1.4.x/src/Json/Rfc7159/NativeJsonDecoder.php#L85-L87 But due to the fact that the polyfill does not provide their definitions - I have to define them myself and in this case the polyfill that is obliged to do so becomes completely unnecessary.

@SerafimArts

This comment has been minimized.

Copy link
Author

SerafimArts commented Feb 7, 2019

P.S. in my opinion, the very idea that someone will check something on the basis of the existence of this constant, and not on the basis of the version of the language is rather doubtful. Anyone might as well add a class_exists('\\JsonException') check and then be surprised that nothing works.

@jvasseur

This comment has been minimized.

Copy link

jvasseur commented Feb 8, 2019

@SerafimArts but what use does the constant have when you can't use it to toggle the associated feature?

Exactly the same as from JsonException exception class, which never happens or from these.

Well, I would be in favor of removing that one from the polyfill.

And why does anyone need a polyfill of PHP 7.3, in which there are no constants added in PHP 7.3?

The thing is, polyfills should polyfill features, and in this case the feature is both, the class, the constant and the fact that json_decode/json_encode works with them. If we can't polyfill the whole feature I think we souldn't polyfill only a part of it, especially since more important part of the feature is the behavior change in the functions.

For example, I use these constants and ensure that they work: https://github.com/railt/json/blob/1.4.x/src/Json/Rfc7159/NativeJsonDecoder.php#L85-L87 But due to the fact that the polyfill does not provide their definitions - I have to define them myself and in this case the polyfill that is obliged to do so becomes completely unnecessary.

in my opinion, the very idea that someone will check something on the basis of the existence of this constant, and not on the basis of the version of the language is rather doubtful. Anyone might as well add a class_exists('\\JsonException') check and then be surprised that nothing works.

True, but what I fear is that someone will do json_decode(..., JSON_THROW_ON_ERROR) and get no error.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 8, 2019

Closing as the presented reasonings make sense for the const (+ I don't agree that we shouldn't add JsonException.)
Thank you all for contributing.

@SerafimArts

This comment has been minimized.

Copy link
Author

SerafimArts commented Feb 8, 2019

@jvasseur Well, you convinced me that it was a really bad idea to add this constant. Thank you for the constructive and reasoned dialogue. 👍

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