[11.x] fix: Model/JsonResource::toJson should not fail with prior json errors

[calebdw]

Hello!

This PR updates the Model/JsonResource::toJson methods so they succeed if there's been prior json errors. It does this by using the JSON_THROW_ON_ERROR flag in conjunction with a try/catch.

The problem

Out of the blue, a Js::from($model) call in a blade template started throwing JsonEncodingExceptions when the model in question hasn't been updated in two years. It turns out that Js::from passes the JSON_THROW_ON_ERROR flag to the Model::toJson method which was in turn just checking json_last_error() after the json_encode call.

However, according to the PHP docs the json_last_error method:

Returns the last error (if any) occurred during the last JSON encoding/decoding, which did not specify JSON_THROW_ON_ERROR.

As the Model::toJson method was now being called with the JSON_THROW_ON_ERROR flag the call to json_encode did not reset the global json state and the method was failing from a json call introduced in a middleware.

Thanks!

[calebdw]

Thanks @taylorotwell!

[lee-van-oetz]

I think this broke things for me in the following way: Some of my models have a getXAttribute mutator defined on them decoding json. Prior to 10.48.18 when the raw data was null, for example, I would still happily get the API response with the respective field empty when simply returning the model instance in the endpoint method body. With 10.48.18 I just get InvalidArgumentException thrown.

[calebdw]

@lee-van-oetz, I don't think this caused your failures:

1. these methods throw JsonEncodingExceptions not InvalidArgumentExceptions
2. these methods were already throwing JsonEncodingExceptions if there was a json error---I just fixed a bug that was causing these methods to throw the exception when the json_encode call was successful but a previous call had failed

[lee-van-oetz]

Apologies, InvalidArgumentException was ultimately getting thrown in \Illuminate\Http\JsonResponse::setData by

    if (! $this->hasValidJson(json_last_error())) {
        throw new InvalidArgumentException(json_last_error_msg());
    }

I've verified the issue I'm having is caused by adding | JSON_THROW_ON_ERROR option in json_encode call in \Illuminate\Database\Eloquent\Model::toJson.

[calebdw]

@lee-van-oetz, then you likely have a rogue, failed json encode/decode call earlier in application. When adding JSON_THROW_ON_ERROR to json_encode it does not reset json_last_error (this is the same issue that I fixed).

Note that the rogue json failure could be the same one as #52204 did not backport that to 10.x

[lotestudio]

Since this update, I must explicitly add "string" as the return type from any controller method that returns a model in my API.

exception: "InvalidArgumentException"
message: "Syntax error"

//throws syntax error
public function show($id)
{
    return Event::with('creator', 'users')
        ->withPlannedClients()->find($id);
} 

//work
public function show($id):string
{
    return Event::with('creator', 'users')
        ->withPlannedClients()->find($id);
} 

//work
public function show($id)
{
    return Event::with('creator', 'users')
        ->withPlannedClients()->find($id)->toJson();
} 

[calebdw]

@lotestudio, make sure you're on the latest 10.x version---there was another fix that prevented null model attributes from triggering the error

[lotestudio]

@calebdw, I'm with v10.48.20 which is latest.

[Flightfreak]

This is a breaking change really. Anyone having json_encode or json_decode actions before the toJson flow without JSON_THROW_ON_ERROR will now experience the same "out of the blue" error as you had experienced. For us, it results in code failing that has run fine since Laravel 4.

The toJson on the Model used to reset the error state, but no longer and now it fails in the Illuminate\Http\JsonResponse@setData .

https://github.com/laravel/framework/blob/11.x/src/Illuminate/Http/JsonResponse.php#L82-L91

[calebdw]

@Flightfreak,

As I mentioned in another PR (#52293 (comment)):

This is because the system relies on global state which is a bad idea and very brittle as you can see.

This PR specifically fixed a bug where a Js::from($model) call in a blade template was throwing exceptions even when the model was successfully being converted to json. In fact, I removed the dependency on global state from the Model and JsonResource. The fact that other areas are now failing as a result of depending on global state is not the fault of this PR nor does it mean that this PR should be reverted.

I did not change the behavior of these methods, they still throw an exception if there's an error encountered when encoding json. I fixed a bug that was causing the exception to be thrown even when the encode was successful.

The solution is to remove the dependency on global state elsewhere in the application, not to revert this PR. If you can post a minimum reproducible example then I can submit another PR to fix the issue.

[Flightfreak]

@calebdw semantics but imho you did change the behaviour of the function and limited the control of its only argument. It impacted a global state before and now it doesn't anymore.

Whether this is breaking or not, considering the rather ambiguous impact of that option (true, this was rather difficult to predict!), this was good material for an upgrade guide.

It's no longer a factor for our project but I guess you could introduce something similar like the empty string fix to lower the impact by introducing this before else:

json_decode('{}', $options);