Skip to content

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

Merged
taylorotwell merged 6 commits into
laravel:10.xfrom
NickSdot:51813-fix-schema-dump
Jun 19, 2024
Merged

[10.x] Ensureschema:dump will dump the migrations table only if it exists#51827
taylorotwell merged 6 commits into
laravel:10.xfrom
NickSdot:51813-fix-schema-dump

Conversation

@NickSdot

Copy link
Copy Markdown
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
Copy Markdown
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

NickSdot commented Jun 18, 2024

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

NickSdot commented Jun 19, 2024

Copy link
Copy Markdown
Contributor Author

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

NickSdot commented Jun 19, 2024

Copy link
Copy Markdown
Contributor Author

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
@NickSdot
NickSdot deleted the 51813-fix-schema-dump branch June 19, 2024 16:33
@Tofandel

Tofandel commented Jun 19, 2024

Copy link
Copy Markdown
Contributor

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