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.

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

Update MigrateCommand.php #1028

Merged
merged 10 commits into from
Jul 6, 2020
Merged

Update MigrateCommand.php #1028

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
929ddb1
Update MigrateCommand.php
ThomasLandauer Jun 25, 2020
f29a905
Update ExecuteCommand.php
ThomasLandauer Jun 28, 2020
69a3358
Update RollupCommand.php
ThomasLandauer Jun 28, 2020
5099259
Update lib/Doctrine/Migrations/Tools/Console/Command/ExecuteCommand.php
ThomasLandauer Jun 29, 2020
e4eb34d
Update lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php
ThomasLandauer Jun 29, 2020
4e3707b
Update lib/Doctrine/Migrations/Tools/Console/Command/RollupCommand.php
ThomasLandauer Jun 29, 2020
6044768
Update ExecuteCommand.php
ThomasLandauer Jul 2, 2020
7ec206e
Update MigrateCommand.php
ThomasLandauer Jul 2, 2020
71da252
Update RollupCommand.php
ThomasLandauer Jul 2, 2020
9517c32
Update MigrateCommandTest.php
ThomasLandauer Jul 3, 2020
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
2 changes: 1 addition & 1 deletion lib/Doctrine/Migrations/Tools/Console/Command/ExecuteCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
$migratorConfigurationFactory = $this->getDependencyFactory()->getConsoleInputMigratorConfigurationFactory();
$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());
ThomasLandauer marked this conversation as resolved.
Show resolved Hide resolved
if (! $migratorConfiguration->isDryRun() && ! $this->canExecute($question, $input)) {
$this->io->error('Migration cancelled!');

Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
$migratorConfigurationFactory = $this->getDependencyFactory()->getConsoleInputMigratorConfigurationFactory();
$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());
Copy link
Member

Choose a reason for hiding this comment

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

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

@greg0ire ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :-( )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

the command getDependencyFactory has been introduced in 3.0

Copy link
Member

@goetas goetas Jun 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@ThomasLandauer yes, DX means Developer Experience

ThomasLandauer marked this conversation as resolved.
Show resolved Hide resolved
if (! $migratorConfiguration->isDryRun() && ! $this->canExecute($question, $input)) {
$this->io->error('Migration cancelled!');

Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/Migrations/Tools/Console/Command/RollupCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected function configure() : void

protected function execute(InputInterface $input, OutputInterface $output) : int
{
$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());
ThomasLandauer marked this conversation as resolved.
Show resolved Hide resolved

if (! $this->canExecute($question, $input)) {
$this->io->error('Migration cancelled!');
Expand Down