Add string validation to isJsonString assertion #345
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.
Assertion::isJsonString
returnstrue
if the value is not a string but is an accepted argument for thejson_decode
function. This is possible because the library works in non-strict mode, so PHP does not raise an error if a non-string argument is passed tojson_decode
.Current behavior
None of these throw an exception:
Expected behavior
All of the above should throw
AssertionFailedException
Psalm
Because the function is annotated with
@psalm-assert =string $value
, Psalm does not raise an issue with this snippet:This will actually fail at runtime with
Fatal error: Uncaught TypeError: test(): Return value must be of type string, int returned
. My PR changes this, soAssertionFailedException
is thrown instead.Backward compatibility
This will be a BC break if someone used the assertion with non-string arguments. On the other hand, description clearly suggests that this assertion should be performed for strings.