New make:migration command #97

weaverryan · 2017-12-14T01:43:06Z

This replaces #10.

There are 2 cases:

(A) When there ARE schema changes:

(B) When there are NOT schema changes:

This includes a BC break on MakerInterface, but it will only affect people who have created custom makers (and they'll get a clear error).

javiereguiluz · 2017-12-14T14:05:22Z

src/Command/MakerCommand.php

@@ -79,7 +82,7 @@ protected function interact(InputInterface $input, OutputInterface $output)
                continue;
            }

-            $value = $this->io->ask($argument->getDescription(), $argument->getDefault(), [Validator::class, 'notBlank']);
+            $value = $this->io->ask($argument->getDescription(), $argument->getDefault(), array(Validator::class, 'notBlank'));


Please, revert this change. We don't use the legacy array() notation in this bundle.

And it's available since PHP 5.4. Anyone using this bundle will probably have at least 5.5 or 5.6

javiereguiluz · 2017-12-14T14:06:36Z

src/ExtraGenerationMakerInterface.php

+    /**
+     * Called after normal code generation.
+     *
+     * @param ConsoleStyle $io


The PHPdoc is not needed because we use type-hints.

javiereguiluz · 2017-12-14T14:08:47Z

src/Maker/MakeMigration.php

+    {
+        $command
+            ->setDescription('Creates a new migration based on database changes.')
+            ->addOption('db', null, InputOption::VALUE_REQUIRED, 'The database connection to use for this command.')


I'd remove "...to use for this command" in all descriptions because it's redundant. Also, I'd like better descriptions. For example, this doesn't help me -> db = The database connection. I can't provide a database connection to a console command. Is this the name of the database connection? The name of the Doctrine database connection? etc.

javiereguiluz · 2017-12-14T14:09:26Z

src/Maker/MakeMigration.php

+    {
+        $command
+            ->setDescription('Creates a new migration based on database changes.')
+            ->addOption('db', null, InputOption::VALUE_REQUIRED, 'The database connection to use for this command.')


Should all these options define default values for the common case where there's only 1 database, 1 em, etc.?

I think you can already get the default connection from Doctrine, as well as for the entity manager, especially if you have only one connection

javiereguiluz · 2017-12-14T14:09:54Z

src/Maker/MakeMigration.php

+
+    public function getFiles(array $params): array
+    {
+        return array();


More array() notation to replace by [] Thanks!

javiereguiluz · 2017-12-14T14:11:13Z

src/Maker/MakeMigration.php

+    {
+        $io->text(array(
+            'Next: Edit Version(lastVersion) to customize the migration.',
+            'Find the documentation at <fg=yellow>https://symfony.com/doc/current/bundles/DoctrineMigrationsBundle/index.html</>',


Could we standardize these doc links in all commands? Find the documentation at... looks too long to me. What about See ... ?

Pierstoval · 2017-12-15T08:31:57Z

src/Maker/MakeMigration.php

+
+    public function interact(InputInterface $input, ConsoleStyle $io, Command $command)
+    {
+    }


Why implement interact when it's empty?

Pierstoval · 2017-12-15T08:32:37Z

tests/Command/FunctionalTest.php

+            new MakeMigration(),
+            array(
+            ),
+        );


Again please use short-syntax for arrays 👍

weaverryan · 2017-12-30T04:28:21Z

This is ready to be reviewed :) - screenshots back in the original description. This also uses the new test system, which means there are real functional tests that actually run the commands and assert that the 2 outputs above occur.

fabpot · 2017-12-30T08:53:22Z

src/Maker/MakeMigration.php

+                'No database changes were detected.'
+            ]);
+            $io->text([
+                'The database schema was compared with your mapping information and they are already in sync: there is no generation to migrate.',


I don't understand the "there is no generation to migrate." part :) I suppose it should be "there is no migration to generate."

Whoooops :)

weaverryan · 2017-12-30T19:37:45Z

Comment addressed (and a few other things). Tests are passing, ready for a final review :).

And sorry for the noise in the diff: this command is a bit different, so we needed to expand the maker system a bit and also the tests, which affected some unrelated things.

javiereguiluz · 2017-12-31T09:05:44Z

src/Maker/MakeMigration.php

+
+    public function writeSuccessMessage(array $params, ConsoleStyle $io)
+    {
+        if (false !== strpos($this->migrationOutput, 'No changes detected')) {


I don't like this logic because it looks fragile ... but there's no other possible way to detect if a migration was generated or not. If MigrationsBundle supported JSON as one of its output formats ... but it doesn't.

You're exactly right. So I'm depending on our tests to catch this :/

javiereguiluz · 2017-12-31T09:09:39Z

src/Maker/MakeMigration.php

+    {
+        if (false !== strpos($this->migrationOutput, 'No changes detected')) {
+            $io->warning([
+                'No database changes were detected.',


Just wondering if we could reword the messages in case no migration is needed:

Before:

[WARNING] No database changes were detected. The database schema was compared with your mapping information and they are already in sync: no migration was generated.

After:

[WARNING] No migration was generated. The database schema and the application mapping information are already in sync.

This PR was squashed before being merged into the 1.0-dev branch (closes #97). Discussion ---------- New make:migration command This replaces #10. There are 2 cases: (A) When there ARE schema changes: <img width="1053" alt="screen shot 2017-12-29 at 11 07 47 pm" src="https://user-images.githubusercontent.com/121003/34451290-9b0c9bb0-ecef-11e7-9a1d-41ba78c84638.png"> (B) When there are NOT schema changes: <img width="1269" alt="screen shot 2017-12-29 at 11 07 14 pm" src="https://user-images.githubusercontent.com/121003/34451291-a617113e-ecef-11e7-979f-c30f2cc3e91e.png"> This includes a BC break on `MakerInterface`, but it will only affect people who have created custom makers (and they'll get a clear error). Commits ------- f2e25e9 tweaking message 95af9fa Adding make:migration, which includes reworking MakerInterface

weaverryan mentioned this pull request Dec 14, 2017

[DoctrineMigration] add the maker command for doctrine migration #10

Closed

javiereguiluz reviewed Dec 14, 2017

View reviewed changes

Pierstoval reviewed Dec 15, 2017

View reviewed changes

tests/Command/FunctionalTest.php Outdated

new MakeMigration(),

array(

),

);

Copy link

Pierstoval Dec 15, 2017

Choose a reason for hiding this comment

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

Again please use short-syntax for arrays 👍

weaverryan force-pushed the feature/add-doctrine-migration-command branch 3 times, most recently from 6034b95 to 2e9ef44 Compare December 30, 2017 04:24

fabpot requested changes Dec 30, 2017

View reviewed changes

weaverryan force-pushed the feature/add-doctrine-migration-command branch 7 times, most recently from b18bf6b to b55fc62 Compare December 30, 2017 19:21

Adding make:migration, which includes reworking MakerInterface

95af9fa

weaverryan force-pushed the feature/add-doctrine-migration-command branch from b55fc62 to 95af9fa Compare December 30, 2017 19:30

javiereguiluz approved these changes Dec 31, 2017

View reviewed changes

tweaking message

f2e25e9

weaverryan merged commit f2e25e9 into symfony:master Dec 31, 2017

weaverryan deleted the feature/add-doctrine-migration-command branch December 31, 2017 18:15

weaverryan mentioned this pull request Dec 31, 2017

Add maker command for form type extensions #85

Closed

dependabot-preview bot mentioned this pull request Mar 19, 2018

Bump symfony/maker-bundle from 1.1.1 to 1.3.0 alcohol/pastebin-php#36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New make:migration command #97

New make:migration command #97

weaverryan commented Dec 14, 2017 •

edited

javiereguiluz Dec 14, 2017

Pierstoval Dec 15, 2017

javiereguiluz Dec 14, 2017

javiereguiluz Dec 14, 2017

javiereguiluz Dec 14, 2017

Pierstoval Dec 15, 2017

javiereguiluz Dec 14, 2017

javiereguiluz Dec 14, 2017

Pierstoval Dec 15, 2017

Pierstoval Dec 15, 2017

weaverryan commented Dec 30, 2017

fabpot Dec 30, 2017

weaverryan Dec 30, 2017

weaverryan commented Dec 30, 2017

javiereguiluz Dec 31, 2017

weaverryan Dec 31, 2017

javiereguiluz Dec 31, 2017

New make:migration command #97

New make:migration command #97

Conversation

weaverryan commented Dec 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Dec 14, 2017 •

edited