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

Upgrade Guide assumes Migration Customization #1264

Closed
caugner opened this issue May 11, 2020 · 9 comments · Fixed by #1265
Closed

Upgrade Guide assumes Migration Customization #1264

caugner opened this issue May 11, 2020 · 9 comments · Fixed by #1265

Comments

@caugner
Copy link

caugner commented May 11, 2020

  • Passport Version: 8.5.0 => 9.1.0
  • Laravel Version: 7.10.3
  • PHP Version: 7.3.17
  • Database Driver & Version: pdo_pgsql

Description:

The Upgrade Guide explains in the Support For Multiple Guards section that a provider column must be added to the oauth_clients database table.

However, the migration provided only works if Migration Customization is used and php artisan vendor:publish --tag=passport-migrations was executed prior to the update.

Otherwise, the migration will fail on fresh installations, because the column is already introduced by the (updated) default migration.

Steps To Reproduce:

  1. composer require laravel/passport:^8
  2. php artisan migrate
  3. composer require laravel/passport:^9
  4. php artisan make:migration PassportUpgradeTo9Dot0
  5. (Copy suggested migration.)
  6. php artisan migrate (works)
  7. php artisan migrate:fresh (fails)
@driesvints
Copy link
Member

Thanks. We've tried clarifying that you should manually add this column when you haven't published the migrations.

@caugner
Copy link
Author

caugner commented May 11, 2020

If you have not previously published the Passport migations, you should manually create a new migration to add this column to your databsae.

@driesvints Thanks for the update, but unfortunately I don't think it's quite right (apart from the typos).

There are two cases:

  1. The passport migrations have not been published yet.
  2. The passport migrations have been published.

And there are two paths:

  1. artisan migrate (to migrate deployed databases).
  2. artisan migrate:fresh (to initialise databases, e.g. as part of testing).

Case 1 will always cause a problem, because the deployed database will be inconsistent with the CreateOauthClientsTable migration. Adding a migration that adds the column will solve the migrate path, but break the migrate:fresh path at the same time.

Only case 2 has a safe solution. Adding the migration that adds the column will solve the migrate path without breaking the migrate:fresh path.

@caugner
Copy link
Author

caugner commented May 11, 2020

PS: The only safe solution for both cases and both paths would be a conditional migration that only adds the column if it doesn't exist.

@driesvints
Copy link
Member

@caugner have you read the actual upgrade guide? We changed it again after the pr.

@caugner
Copy link
Author

caugner commented May 11, 2020

@driesvints Indeed I had only looked at the referenced PR.

If you have not previously published the Passport migrations, you should manually add the provider column to your database.

To me it seems like this suggested manual migration defeats the purpose of the migrations, but I guess there is no better solution in sight (like adding a conditional migration)?

@driesvints
Copy link
Member

@caugner the OP of the PR should indeed have created a new migration. This is a bit unfortunate. We'll try to watch over this more carefully in the future.

@AlbinoDrought
Copy link

If anyone else needs it, here is the migration I am using to go from v8 to v9: (postgres)

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

class UpgradeLaravelPassportToNine extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        // https://github.com/laravel/passport/issues/1264#issuecomment-626745261
        // unpublished migration changed in laravel/passport 9, only add this column if it's missing:
        $hasProviderColumn = \Illuminate\Support\Facades\DB::table('information_schema.columns')
            ->select('column_name')
            ->where('table_name', '=', 'oauth_clients')
            ->where('column_name', '=', 'provider')
            ->exists();

        if (!$hasProviderColumn) {
          Schema::table('oauth_clients', function (Blueprint $table) {
              // https://github.com/laravel/passport/blob/master/UPGRADE.md#support-for-multiple-guards
              $table->string('provider')->after('secret')->nullable();
          });
        }
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::table('oauth_clients', function (Blueprint $table) {
            $table->dropColumn('provider');
        });
    }
}

@bolechen
Copy link

@ref #1308

@oranges13
Copy link

oranges13 commented Dec 16, 2020

The solution from @AlbinoDrought worked great, but our tests are using sqlite which does not have an information_schema table. For greater compatibility, you can use the Schema::hasColumn method directly:

if(!Schema::hasColumn('oauth_clients','provider')) {
    Schema::table('oauth_clients', function (Blueprint $table) {
        // https://github.com/laravel/passport/blob/master/UPGRADE.md#support-for-multiple-guards
        $table->string('provider')->after('secret')->nullable();
    });
}

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

Successfully merging a pull request may close this issue.

5 participants