Skip to content
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

Null compatibility broken #43705

Closed
jc-skurman opened this issue Aug 15, 2022 · 9 comments
Closed

Null compatibility broken #43705

jc-skurman opened this issue Aug 15, 2022 · 9 comments
Labels

Comments

@jc-skurman
Copy link

  • Laravel Version: 9.24.0
  • PHP Version: 8.1.0
  • Database Driver & Version:
    MySql 8
    Description:
    When we use a cast of type 'object', 'collection' and we have the original value null, we get an error when trying to decode json.
    This change appeared in the PR and is still relevant even for version 9.
    PR
    We get the warning

PHP Deprecated: json_decode(): Passing null to parameter #1 ($json) of type string is deprecated

Steps To Reproduce:

@driesvints
Copy link
Member

@tpetry since you originally created #37961 which sparked #41337: do you know of a way to do a fix so both issues are resolved?

Also want to ping you @JBtje since you're the author of #41337.

@jc-skurman this change has been in the framework now for several months. Did you only update Laravel now recently?

@jc-skurman
Copy link
Author

I just now caught this bug on the old version and after indicating that you no longer support it, I checked it on the latest version

@MasterRO94
Copy link
Contributor

@driesvints

I've checked issues can be reproduced if you have null original value for object or/and collection cast. This can be fixed replacing fromJson call to castAttribute

} elseif ($this->hasCast($key, ['object', 'collection'])) {
           return $this->fromJson($attribute) ===
               $this->fromJson($original);

change to

} elseif ($this->hasCast($key, ['object', 'collection'])) {
           return $this->castAttribute($key, $attribute) ===
               $this->castAttribute($key, $original);             

@tpetry
Copy link
Contributor

tpetry commented Aug 15, 2022

I replaced the dirty check with my custom implementation because in my projects I found a lot more edge cases, e.g. different order of object keys.

But based on the implementation of the PR the null value has to be handled too. null != json, null = null.

@JBtje
Copy link
Contributor

JBtje commented Aug 15, 2022

First and foremost, null (object) is not valid json, so the warning is correct If you seed your empty JSON fields with a valid JSON value (e.g. empty string), the warning is solved.
I have come across this warning a few times, just to realize i did something wrong (i.e. not seeding JSON field properly).

But if you think null should be a valid value for a JSON field, then this could easily be solved:
https://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L1203

        return json_decode($value ?? '', ! $asObject);

solves the warning.

@driesvints
Copy link
Member

@JBtje that's not true actually. null is definitely valid JSON: https://stackoverflow.com/questions/8526995/is-null-valid-json-4-bytes-nothing-else

@driesvints
Copy link
Member

Would appreciate a PR to fix this.

@JBtje
Copy link
Contributor

JBtje commented Aug 15, 2022

Edit: PHP was right...

@driesvints 'null' as a string is valid JSON. null as an object isn't valid JSON.

After reading up some more on the topic; the json-text must be a string.
The json-value however can also be a null (object), true/false (bool), array, object
https://datatracker.ietf.org/doc/rfc8259/
visualisation: https://www.json.org/json-en.html

json_decode( 'null' ) -> null
json_decode( 'false' ) -> false
json_decode( 'true' ) -> true

json_decode( null ) -> invalid json, throws depricated warning
json_decode( false ) -> invalid json, returns int(1) (don't ask why)
json_decode( true ) -> invalid josn, returns null (don't ask why)

Javascript however accepts null (object), true/false (bool) as input. This is allowed under RFC8259 but not mandatory.

Cause of this problem:
https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg
Changed in php/php-src@b10416a#diff-a39ce231303cec12a6d347165931a7f2209ee7dce5989f2e06f8c04143ec7032

Solution to support null (object) in Laravel:
PR: #43706

@driesvints
Copy link
Member

We merged a fix for this. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants