Skip to content

Fix »OAuth Client Table Changes« migration snippet in the upgrade guide #1859

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 9, 2025
Merged

Fix »OAuth Client Table Changes« migration snippet in the upgrade guide #1859

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec884f7
Fix owner columns are not added after the `user_id` as intended
hettiger Oct 8, 2025
fea16cc
Fix `TypeError` when migrating the `oauth_clients` table
hettiger Oct 8, 2025
eefdb76
Fix broken polymorphic owner relationship
hettiger Oct 8, 2025
424330f
Replace hardcoded User class-string
hettiger Oct 8, 2025
6b9b0b8
Leave `grant_types` nullable.
hettiger Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,20 +139,20 @@ If you prefer to use the new structure, you may create a migration to apply the

```php
Schema::table('oauth_clients', function (Blueprint $table) {
$table->after('user_id', function (Blueprint $table) {
$table->nullableMorphs('owner');
});
$table->nullableMorphs('owner', after: 'user_id');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passport supports Laravel 11+ but the suggested syntax is available since 12+ AFAIK. Anything wrong with current syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Contributor

@hafezdivandari hafezdivandari Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare default instead, you may apply the same change to redirect_uris column too:

Suggested change
$table->text('grant_types')->nullable();
$table->text('grant_types')->default('[]');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, so I guess nullable is fine for both columns?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

// \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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make them both nullable because adding a not null column without default causes error on some DB drivers!

});
});

foreach (Passport::client()->cursor() as $client) {
Model::withoutTimestamps(fn () => $client->forceFill([
'owner_id' => $client->user_id,
'owner_type' => $client->user_id ? config('auth.providers.'.$client->provider.'.model') : null,
'owner_type' => $client->user_id
? config('auth.providers.'.($client->provider ?: config('auth.guards.api.provider')).'.model')
: null,
'redirect_uris' => $client->redirect_uris,
'grant_types' => $client->grant_types,
])->save());
Expand Down