Update MigrateCommand.php by ThomasLandauer · Pull Request #1028 · doctrine/migrations

ThomasLandauer · 2020-06-25T21:24:15Z

Q	A
Type	improvement
BC Break	no
Fixed issues	none

Summary

Adding the name of the database to CLI's yes/no question, analogous to https://github.com/doctrine/DoctrineFixturesBundle/blob/master/Command/LoadDataFixturesDoctrineCommand.php#L103

TODO: Do the same in ExecuteCommand and RollupCommand

Adding the name of the database to CLI's yes/no question, analogous to https://github.com/doctrine/DoctrineFixturesBundle/blob/master/Command/LoadDataFixturesDoctrineCommand.php#L103 TODO: Do the same in `ExecuteCommand` and `RollupCommand`

goetas · 2020-06-26T07:01:30Z

lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php

        $migratorConfiguration        = $migratorConfigurationFactory->getMigratorConfiguration($input);

-        $question = 'WARNING! You are about to execute a database migration that could result in schema changes and data loss. Are you sure you wish to continue?';
+        $question = sprintf('WARNING! You are about to execute a migration in database "%s" that could result in schema changes and data loss. Are you sure you wish to continue?', $this->getDependencyFactory()->getConnection()->getDatabase());


getDatabase is not part of the \Doctrine\DBAL\Driver\Connection interface, thus i'm not sure if we can do this...

@greg0ire ?

🤔 but getConnection returns a \Doctrine\DBAL\Connection instead of that interface, right?

hmm. right. being so, we know the database... (i remember that there was a reason for choosing the concrete implementation, but can't recall why :-( )

In doctrine/migrations 2.2.1 I'm getting:

In MigrateCommand.php line 151:
Attempted to call an undefined method named "getDependencyFactory" of class "Doctrine\Bundle\MigrationsBundle\Command\MigrationsMigrateDoctrineCommand". Did you mean to call "setDependencyFactory"?

But in 3.0.1 get_class($this->getDependencyFactory()->getConnection()) gives me:

Doctrine\DBAL\Connection

the command getDependencyFactory has been introduced in 3.0

@greg0ire I'm not sure if is worth this, it would be nice to change the return type from \Doctrine\DBAL\Driver\Connection to \Doctrine\DBAL\Connection to have a more simple interface

That wouldn't be a BC break, except for extending classes, so… go ahead?

May be I'm worrying too much. Probably the improved dx is worth this change.

@ThomasLandauer can you do the same change on the other commands?

Sure, done :-) (I just copied it over - since they all extend DoctrineCommand)

@goetas: What means "DX"? Developer Experience - as opposed to User Experience (UX)?

@ThomasLandauer yes, DX means Developer Experience

See #1028 (comment)

goetas · 2020-06-29T06:10:32Z

@ThomasLandauer tests are failing

Sqlite database name is empty.... what should we put in that case? the DB path? (is it possible with the current DBAL api?

ThomasLandauer · 2020-06-29T13:03:05Z

Hm, could it be that the failure is an artifact of your testing environment? Cause I am getting the full path to my .sqlite file! (same as in fixtures bundle)...

lib/Doctrine/Migrations/Tools/Console/Command/ExecuteCommand.php

lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php

lib/Doctrine/Migrations/Tools/Console/Command/RollupCommand.php

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

goetas · 2020-07-02T07:06:40Z

Tests are failing with a message as
You are about to execute a migration in database "" that could result in schema changes and data loss
As I was saying in one of my previous comments, for SQLite (with in-memory storage option) the database name seems to be empty)

ThomasLandauer · 2020-07-02T09:14:44Z

OK, I see. So what should we do in that case? Display some constant string, e.g. "SQLite :memory:"? It looks like FixturesBundle isn't doing anything special, so it's probably also just showing "": https://github.com/doctrine/DoctrineFixturesBundle/blob/master/Command/LoadDataFixturesDoctrineCommand.php#L103

goetas · 2020-07-02T12:03:01Z

@ThomasLandauer what about $this->getDependencyFactory()->getConnection()->getDatabase() ?? 'unknown' ?

See #1028 (comment)

ThomasLandauer · 2020-07-02T12:43:17Z

I changed it to ?? '<unnamed>', to emphasize that this is not an error but the database just doesn't have a "name". The tests will probably still fail, since they expect another string. Should I go ahead and adjust them?

goetas · 2020-07-03T07:11:47Z

@ThomasLandauer i'm fine with <unnamed>, can you please update the tests then?

See #1028 (comment)

ThomasLandauer · 2020-07-03T12:19:25Z

Done :-) BTW: I only updated MigrateCommandTest.php; there seem to be no tests for the other two commands.

goetas · 2020-07-04T08:37:54Z

@greg0ire I guess that for this we can do squash merge

greg0ire · 2020-07-06T21:45:17Z

Sure! Done.

@Ocramius

Since DBAL 3.0 (see doctrine/migrations#1028), the message currently reads for SQLite: > Careful, database "" will be purged. Do you want to continue? (yes/no) Neither @Ocramius (see doctrine/dbal#3606 (comment)) nor me (see doctrine/dbal#4982) succeeded in convincing @morozov to continue returning the file path ;-) So it looks like the fix needs to be done here... Suggestion: What about adding the platform too? So the message would be: > Careful, MySQL database "foo" will be purged. Do you want to continue? (yes/no) `doctrine/migrations` is currently broken too: > WARNING! You are about to execute a migration in database "<unnamed>" that could result in schema changes and data loss. Are you sure you wish to continue? (yes/no) [yes]: So when done here, I'm going to submit the same there (i.e. follow up of doctrine/migrations#1028)

Update MigrateCommand.php

929ddb1

Adding the name of the database to CLI's yes/no question, analogous to https://github.com/doctrine/DoctrineFixturesBundle/blob/master/Command/LoadDataFixturesDoctrineCommand.php#L103 TODO: Do the same in `ExecuteCommand` and `RollupCommand`

goetas reviewed Jun 26, 2020

View reviewed changes

ThomasLandauer added 2 commits June 28, 2020 22:56

Update ExecuteCommand.php

f29a905

See #1028 (comment)

Update RollupCommand.php

69a3358

See #1028 (comment)

greg0ire reviewed Jun 29, 2020

View reviewed changes

ThomasLandauer and others added 3 commits June 29, 2020 22:15

Update lib/Doctrine/Migrations/Tools/Console/Command/ExecuteCommand.php

5099259

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

Update lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php

e4eb34d

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

Update lib/Doctrine/Migrations/Tools/Console/Command/RollupCommand.php

4e3707b

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

greg0ire closed this Jul 1, 2020

greg0ire reopened this Jul 1, 2020

ThomasLandauer added 3 commits July 2, 2020 14:40

Update ExecuteCommand.php

6044768

See #1028 (comment)

Update MigrateCommand.php

7ec206e

Update RollupCommand.php

71da252

Update MigrateCommandTest.php

9517c32

See #1028 (comment)

goetas requested a review from greg0ire July 4, 2020 08:37

goetas approved these changes Jul 4, 2020

View reviewed changes

greg0ire merged commit a864981 into doctrine:3.0.x Jul 6, 2020

ThomasLandauer deleted the patch-1 branch July 7, 2020 05:57

alcaeus added this to the 3.0.2 milestone Dec 23, 2020

github-actions bot mentioned this pull request Dec 23, 2020

Merge release 3.0.2 into 3.1.x #1103

Merged

ThomasLandauer mentioned this pull request Nov 11, 2021

Fixing database "name" (=path) for SQLite doctrine/DoctrineFixturesBundle#359

Closed

Uh oh!

Conversation

ThomasLandauer commented Jun 25, 2020

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goetas Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goetas commented Jun 29, 2020

Uh oh!

ThomasLandauer commented Jun 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goetas commented Jul 2, 2020

Uh oh!

ThomasLandauer commented Jul 2, 2020

Uh oh!

goetas commented Jul 2, 2020

Uh oh!

ThomasLandauer commented Jul 2, 2020

Uh oh!

goetas commented Jul 3, 2020

Uh oh!

ThomasLandauer commented Jul 3, 2020

Uh oh!

goetas commented Jul 4, 2020

Uh oh!

greg0ire commented Jul 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

goetas Jun 28, 2020 •

edited

Loading