Fix »OAuth Client Table Changes« migration snippet in the upgrade guide #1859
Conversation
TypeError
Laravel\Passport\Client::{closure:Laravel\Passport\Client::grantTypes():157}(): Return value must be of type array, null returned
This happens because `isset($value) ? $this->fromJson($value) : array_keys(/* … */)` evaluates to `null`.
> After adding a non-nullable `grant_types` column with no default values `isset($value)` is `true` because `$value` is an empty string.
> As a result `$this->fromJson($value)` is called with an empty string which evaluates to `null` …
```php
> $value = ''; isset($value);
= true
```
This then violates the return type `get: fn (?string $value): array` …
Making the field nullable first fixes the issue:
```php
> $value = null; isset($value);
= false
```
Once the data is written I set nullable to false because the original migration creates a non-nullable field as well …
An alternative to this "workaround" would be updating the `Laravel\Passport\Client::grantTypes()` implementation.
The `config()` call falls back to `null` when the `$client` has on provider. The result is a client with an `owner_id` that has no corresponding `owner_type` set. Adding `\App\Models\User::class` as a fallback seems reasonable.
| $table->after('user_id', function (Blueprint $table) { | ||
| $table->nullableMorphs('owner'); | ||
| }); | ||
| $table->nullableMorphs('owner', after: 'user_id'); |
There was a problem hiding this comment.
Passport supports Laravel 11+ but the suggested syntax is available since 12+ AFAIK. Anything wrong with current syntax?
There was a problem hiding this comment.
The $table->after('user_id', function (Blueprint $table) { … }); wrap did not work for me in combination with $table->nullableMorphs('owner');.
Passport supports Laravel 11+ but the suggested syntax is available since 12+ AFAIK.
I didn't check that. If it needs to work on L11 too and the owner_type + owner_id columns should end up right after the user_id column, we'd probably have to manually create the individual columns within the $table->after('user_id', function (Blueprint $table) { … }); wrap.
There was a problem hiding this comment.
The $table->after('user_id', function (Blueprint $table) { … }); wrap did not work for me in combination with $table->nullableMorphs('owner');
Weired, I'll run a test on the framework to make sure!
There was a problem hiding this comment.
It was working fine, but a bug /breaking change has been introduced by laravel/framework#56613
Please keep it as is, I'll send a PR to the framework to fix this.
| $table->after('provider', function (Blueprint $table) { | ||
| $table->text('redirect_uris'); | ||
| $table->text('grant_types'); | ||
| $table->text('grant_types')->nullable(); |
There was a problem hiding this comment.
Declare default instead, you may apply the same change to redirect_uris column too:
| $table->text('grant_types')->nullable(); | |
| $table->text('grant_types')->default('[]'); |
There was a problem hiding this comment.
Declare default instead
// \Laravel\Passport\Client::grantTypes
Attribute::make(
get: fn (?string $value): array => isset($value) ? $this->fromJson($value) : array_keys(array_filter([
'authorization_code' => ! empty($this->redirect_uris),
'client_credentials' => $this->confidential() && $this->firstParty(),
'implicit' => ! empty($this->redirect_uris),
'password' => $this->password_client,
'personal_access' => $this->personal_access_client && $this->confidential(),
'refresh_token' => true,
'urn:ietf:params:oauth:grant-type:device_code' => true,
])),
);If you default to '[]', isset($value) will evaluate to true for all rows during the migration. Therefore $this->fromJson($value) is being called instead of the array_keys(array_filter([ … ])) code, that actually preserves the data ($this->password_client and $this->personal_access_client) during the migration.
⇒ Your suggested change would drop data instead of preserving it.
you may apply the same change to redirect_uris column too:
redirect_uris does not use an isset check so it works just fine. No need to change anything about redirect_uris.
There was a problem hiding this comment.
You're right, so I guess nullable is fine for both columns?
There was a problem hiding this comment.
// \Laravel\Passport\Client::redirectUris
Attribute::make(
get: fn (?string $value, array $attributes): array => match (true) {
! empty($value) => $this->fromJson($value),
! empty($attributes['redirect']) => explode(',', $attributes['redirect']),
default => [],
},
);I tested it with just one redirect URL as well as multiple URLs joined by commas. It always worked just fine. The redirect column was not nullable, so we can expect a value. I don't think we need to change anything about it. However, adding the nullable and removing it later, just as we do with grant_types does not hurt either.
There was a problem hiding this comment.
We should make them both nullable because adding a not null column without default causes error on some DB drivers!
Co-authored-by: Hafez Divandari <hafezdivandari@gmail.com>
|
Thank you for the super quick review @hafezdivandari |
|
I think this was not 100% ready. I didn't have the time to answer sooner and now coming back here I see this has already been merged. It's not a big deal — no need to revert. However, maybe @hafezdivandari wants to apply his suggested changes in a follow-up PR. I consider this as done on my end. Thank you! :-) |
3 participants
The migration code snippet of the v13 upgrade guide immediately crashed on me.
I had to apply several fixes.
Please see commit descriptions for more details.
Using
\App\Models\User::classeven though old apps might still use\App\User::classdoes not seem to be that big of an issue. The IDE should immediately highlight the missing class. (eefdb76)