[10.x] backport #52188

[calebdw]

Hello!

We still have an application on 10.x and are suffering the same failures referenced in the original issue. This backports #52188 to 10.x

Thanks!

[calebdw]

Thanks!

[Arkhas]

We have a big issue with this PR, since this update, we are getting a InvalidArgumentException in a lot of our response when returning directly a Model from a controller like :

Route::get('/users', function () {
    return User::all();
});

reverting this PR totally fix this issue

The issue is coming from casting an empty column to array : how to reproduce :

$user = new User(['foo' => '']);
$user->mergeCasts(['foo' => 'array']);

 $user->toJson();

// 4 with this PR, 0 without
dd(json_last_error());

so catching this in this function :

         try {
            $json = json_encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR);
        } catch (JsonException $e) {
            throw JsonEncodingException::forModel($this, $e->getMessage());
        }

is causing the exception to be thrown

[calebdw]

That means there's a failed json_encode/decode earlier in the application. Could be due to #52204 which I have not backported, but I can do that real quick

[Arkhas]

That means there's a failed json_encode/decode earlier in the application. Could be due to #52204 which I have not backported, but I can do that real quick

I edited my message, but it seems that is indeed from the same issue, but I don't think the JSON_THROW_ON_ERROR should be directly into the Model class

[Arkhas]

In this PR you completly disable the option to give 0 as $options so it will never be possible to give the "0" flag to the json_encode function, if you want to use JSON_THROW_ON_ERROR then you should use it when you manualy call the toJson() function :

return $model->toJson(JSON_THROW_ON_ERROR);

Anyway, this a breaking change, and this should not be in a minor update in laravel 10

[calebdw]

In this PR you completly disable the option to give 0 as $options so it will never be possible to give the "0" flag to the json_encode function, if you want to use JSON_THROW_ON_ERROR then you should use it when you manualy call the toJson() function :

No, this is correct---the model was throwing an exception anyway if there was a json error. The issue that I fixed was that the exception was being thrown for a previous json error even when the model was successfully converted to json.

[Arkhas]

you can try it doing this in a controller :

$user = new User(['foo' => '']);
$user->mergeCasts(['foo' => 'array']);

return  $user;

it was not crashing before, and crash now

[calebdw]

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've opened a PR which backports the request json_decode issue (#52389), however, the code really needs to be cleaned up to not rely on global state as this error you are experiencing could still happen if other json errors occur earlier in the application

[Arkhas]

I aggre that it should throw an error when dealing with faulty Json, but this is not my point.

• This change is a breaking change and cause any model using an empty array casted column to crash the app
• You are totally breaking the existing function, as passing 0 to the toJson() function is not working anymore

At least it should be notted as a breaking change in the upgrade to Laravel 10 Documentation

@taylorotwell what is your opinion on the matter ?

[calebdw]

You are totally breaking the existing function, as passing 0 to the toJson() function is not working anymore

This is not a breaking change because the Model::toJson method still threw a JsonEncodingException even if 0 was passed to the method

[Arkhas]

well, just without your changes, try that :

$user = new User(['foo' => '']);
$user->mergeCasts(['foo' => 'array']);

return  $user;

Before, the user would be return, and now, you have an exception making the app to crash

[calebdw]

BLUF

An exception is being thrown when returning a Model from a controller that has an array-casted attribute set to an empty string (an empty string is not valid json).

First and foremost, this is data integrity issue---database columns that are cast to json/array should not be EMPTY, they should either be NULL or contain valid json.

However, it would be possible to prevent this parse error by converting falsely json values to a json null ('null') before decoding:

// src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php
    public function fromJson($value, $asObject = false)
    {
-       return Json::decode($value ?? '', ! $asObject);
+       return Json::decode($value ?: 'null', ! $asObject);
    }

Analysis

I've stepped through the framework with a debugger and here's what I've found:

The exception is being thrown by the JsonResponse::setData method (note the json_decode call before encoding the data):

framework/src/Illuminate/Http/JsonResponse.php

Lines 73 to 92 in 763b942

	public function setData($data = []): static
	{
	$this->original = $data;
	// Ensure json_last_error() is cleared...
	json_decode('[]');
	$this->data = match (true) {
	$data instanceof Jsonable => $data->toJson($this->encodingOptions),
	$data instanceof JsonSerializable => json_encode($data->jsonSerialize(), $this->encodingOptions),
	$data instanceof Arrayable => json_encode($data->toArray(), $this->encodingOptions),
	default => json_encode($data, $this->encodingOptions),
	};
	if (! $this->hasValidJson(json_last_error())) {
	throw new InvalidArgumentException(json_last_error_msg());
	}
	return $this->update();
	}

A Model is Jsonable so the first match arm is executed, an exception is not being thrown here so we know that this json_encode call is succeeding:

framework/src/Illuminate/Database/Eloquent/Model.php

Lines 1648 to 1657 in 763b942

	public function toJson($options = 0)
	{
	try {
	$json = json_encode($this->jsonSerialize(), $options | JSON_THROW_ON_ERROR);
	} catch (JsonException $e) {
	throw JsonEncodingException::forModel($this, $e->getMessage());
	}
	return $json;
	}

Model::jsonSerialize() is just an alias for Model::toArray() which casts the attributes before returning the array. If a value is null and it's a primitive cast (which array is) then the cast operation is skipped:

framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

Lines 734 to 740 in 763b942

	protected function castAttribute($key, $value)
	{
	$castType = $this->getCastType($key);
	if (is_null($value) && in_array($castType, static::$primitiveCastTypes)) {
	return $value;
	}

Otherwise the attribute is casted to an array:

framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

Lines 768 to 770 in 763b942

	case 'array':
	case 'json':
	return $this->fromJson($value);

framework/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

Lines 1272 to 1275 in 763b942

	public function fromJson($value, $asObject = false)
	{
	return Json::decode($value ?? '', ! $asObject);
	}

Which leads to Json::decode, and this is where the json_last_error is introduced---because an empty string is not valid json:

framework/src/Illuminate/Database/Eloquent/Casts/Json.php

Lines 32 to 37 in 763b942

	public static function decode(mixed $value, ?bool $associative = true): mixed
	{
	return isset(static::$decoder)
	? (static::$decoder)($value, $associative)
	: json_decode($value, $associative);
	}

[donnysim]

I get the reasoning about that it shouldn't have worked, but you've proved yourself that it is a breaking change even if it shouldn't have worked before. Even I have apps that still run on old MySQL 5 with TEXT fields for json and some have empty data for good or bad reasons, and the argument that your data is wrong does not make it any different that a change broke what was working before without any notice or warning. So this should either go all the way to fix it or not fix it at all in a minor release.

[Arkhas]

Thank you all :)