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

[9.x] Null value for auto-cast field caused deprication warning in php 8.1 #43706

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

JBtje
Copy link
Contributor

@JBtje JBtje commented Aug 15, 2022

Problem:

However, thr RFC says that null is a valid JSON value:
https://datatracker.ietf.org/doc/rfc8259/

A JSON value MUST be an object, array, number, or string, or one of
the following three literal names:

    false null true

 The literal names MUST be lowercase.  No other literal names are
 allowed.

Thus, auto cast should accept null as a valid JSON and not result in a deprecation warning..

also see issue #43705

@driesvints
Copy link
Member

Please provide a thorough explanation and don't just link to an issue. Otherwise this will get closed.

@GrahamCampbell GrahamCampbell changed the title Null value for auto-cast field caused deprication warning in php 8.1 [9.x] Null value for auto-cast field caused deprication warning in php 8.1 Aug 15, 2022
@taylorotwell taylorotwell merged commit b080b93 into laravel:9.x Aug 15, 2022
@marcovo
Copy link
Contributor

marcovo commented Aug 19, 2022

The reasoning based on the RFC is not entirely solid. It is true that false, true and null are valid JSON literals, but translating to php, this would mean that the following three statements should successfully return the respective values (which they do):

json_decode('true')
json_decode('false')
json_decode('null')

Here, 'null' is the null described by the RFC. This is something else than:

json_decode(null)

i.e., the null literal instead of the string 'null'.

While the submitted change is probably perfectly valid, this would be primarily to conform to php's new function signature. The RFC argument is not valid here.
Also, missing a test here?

@driesvints
Copy link
Member

@marcovo did this break something for you? If so how is that reproducable?

@marcovo
Copy link
Contributor

marcovo commented Aug 19, 2022

It did not break anything for me; merely pointing out that in the opening post, the deprecation warning is a valid point but the RFC argument is not applicable. Ultimately the change seems fine.

@driesvints
Copy link
Member

@marcovo right, gotcha.

@kayw-geek
Copy link
Contributor

@driesvints #43507 So should it be shut down instantly?

@driesvints
Copy link
Member

@kayw-geek you didn't exactly explain where this issue was coming from as well as the people in #43705 did. Afaik you could be calling the method directly. Next time please provide a more thorough explanation and your PR might have been accepted.

@kayw-geek
Copy link
Contributor

@kayw-geek you didn't exactly explain where this issue was coming from as well as the people in #43705 did. Afaik you could be calling the method directly. Next time please provide a more thorough explanation and your PR might have been accepted.

OK, I'll write it in detail later.

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

Successfully merging this pull request may close these issues.

5 participants