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

[10.x] Ensureschema:dump will dump the migrations table only if it exists #51827

Merged
merged 6 commits into from
Jun 19, 2024

Conversation

NickSdot
Copy link
Contributor

Fixes #51813

The migrations table is only available in then default connection.
When running schema:dump --database=Something the command attempts to dump the migration table, too.

This results in:

Symfony\Component\Process\Exception\ProcessFailedException 

The command "mysqldump  --user="${:LARAVEL_LOAD_USER}" --password="${:LARAVEL_LOAD_PASSWORD}" --host="${:LARAVEL_LOAD_HOST}" --port="${:LARAVEL_LOAD_PORT}" --no-tablespaces --skip-add-locks --skip-comments --skip-set-charset --tz-utc "${:LARAVEL_LOAD_DATABASE}" migrations --no-create-info --skip-extended-insert --skip-routines --compact --complete-insert" failed.

Exit Code: 6(Unknown error)

Working directory: /Users/Nick/Projects/blub

Output:
================


Error Output:
================
mysqldump: [Warning] Using a password on the command line interface can be insecure.
mysqldump: Couldn't find table: "migrations"

  at /Users/Nick/.composer/vendor/symfony/process/Process.php:267
    263▕      */
    264▕     public function mustRun(?callable $callback = null, array $env = []): static
    265▕     {
    266▕         if (0 !== $this->run($callback, $env)) {
  ➜ 267▕             throw new ProcessFailedException($this);
    268▕         }
    269▕ 
    270▕         return $this;
    271▕     }

      +18 vendor frames 

  19  artisan:46
      Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))

This PR checks if we are on the default connection and skips dumping migrations if not.

The command has not had any tests.
I don't think the scope of this PR requires to change this?

@taylorotwell
Copy link
Member

One issue with this PR is we can't use facades from Illuminate components. Configuration will have to be injected.

@taylorotwell taylorotwell marked this pull request as draft June 18, 2024 17:32
@NickSdot
Copy link
Contributor Author

NickSdot commented Jun 18, 2024

@taylorotwell I had that thought at first. But then I saw the component already depends on the config facade here. So, I am taking this is undesired? If yes, I'd remove the dependency in one go?

Edit: how would you feel about $connection->getSchemaBuilder()->hasTable('migrations')?

@driesvints driesvints marked this pull request as ready for review June 18, 2024 20:32
@Tofandel
Copy link
Contributor

$connection->getSchemaBuilder()->hasTable('migrations') is the way to go, because other connections definitely can have a migrations table, as is the case with the tenancy/tenancy package

@NickSdot
Copy link
Contributor Author

NickSdot commented Jun 19, 2024

Edit: was answered here #51813 (comment)

$connection->getSchemaBuilder()->hasTable('migrations') is the way to go, because other connections definitely can have a migrations table, as is the case with the tenancy/tenancy package

Interesting! I would have expected that each tenant has a own database config, and therefore each tenant an own default connection as well. To confirm I understand you correctly, tenancy/tenancy uses only one database.php with one default. All tenants are just different connections?

@NickSdot NickSdot changed the title [10.x] Dump migrations table only on default connection [10.x] Ensureschema:dump will dump the migrations table only if it exists Jun 19, 2024
@NickSdot
Copy link
Contributor Author

NickSdot commented Jun 19, 2024

I pushed the changes. Didn't touch the existing Config::get() for now.

cc @Tofandel

@taylorotwell taylorotwell merged commit f7ea4e9 into laravel:10.x Jun 19, 2024
24 checks passed
@NickSdot NickSdot deleted the 51813-fix-schema-dump branch June 19, 2024 16:33
@Tofandel
Copy link
Contributor

Tofandel commented Jun 19, 2024

tenancy/tenancy uses only one database.php with one default. All tenants are just different connections?

There is one default connection which is the main app, and each tenant has it's own database, but only one connection is configured for all the tenants (its the tenant connection), it's dynamically configured at runtime when doing Tenancy::setTenant() or when identifying a tenant via a cli argument or other method, and each of the tenant database maintains it's own migrations table (because the migration files for the tenants are separate from the main app but each tenant usually share the same migrations)

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

Successfully merging this pull request may close these issues.

3 participants