[FEATURE] Upgrade to doctrine/migrations 3 #118

seifane-wise · 2022-03-02T15:15:14Z

Changes proposed in this pull request:

Upgrade to doctrine/migrations 3
Use doctrine implementations of commands

This PR is linked to the one I made on orm (laravel-doctrine/orm#520).
It basically upgrades to doctrine/migrations 3. Since the package now provides implementation of the commands used, I removed custom implementations and used doctrine ones.

Feedback on this would be greatly appreciated specifically on the wrapping of commands (I feel like it could be done more smart-ly) and on a way of testing this. Since all custom implemention is gone what's left is only the configuration loading part to load config and instantiate an EntityManager to pass down to the command.

Also maybe this needs to be released on a new 3.x branch ?

eigan · 2022-03-03T06:13:33Z

Woah, I love this PR. Might take some time to review though.. 🙂

eigan · 2022-03-03T06:20:06Z

Looks like the tests is gone, I still think we should need some.

seifane-wise · 2022-03-03T14:52:47Z

@eigan I agree with that. I am still thinking of way to test this but it will need some more effort since all doctrine commands are final classes it makes it challenging to mock them to correctly test the commands. I'll see if I find a way to make it sane. (input if any is appreciated :))

I just made this PR first to make sure maintainers where okay with this approach, which apparently you are. I'll continue working on that :)

dpslwk · 2022-03-07T05:55:55Z

not had chance to fully look at test this yet but few quick notes

this also is quite a big change that we might want to release as a new 3.x branch, needs a change log and upgrade guide, if command have change

even if all the test are removed I think the ci workflow should still cover all the combination and be expanded to php 8.1 and laravel 9, ( see matrix and exclude ci as per extension ) this would at least test the install based on all the expected configs
other option is we narrow this rite down to laravel 9, php 8/8.1

seifane-wise · 2022-03-07T17:42:38Z

I tried very hard to find a sane way to test the commands but couldn't find any which is kind of annoying to me because I would still to test the ArrayInput creation since that's the only custom code here and it would be nice to have some for regressions. I agree on the fact that it needs a 3.x branch for this.

For the changelog / upgrade guide I don't think there's much but I will look around see what really changed. The commands should behave just like they did before.

@dpslwk where do you want these files to go ?

cleanup

jochemfuchs · 2022-03-08T13:43:42Z

@seifane-wise
Having tested this branch, I've noticed that it tries to drop sequences in the reset command, regardless of the platform used. Would be nice to add a check of the platform supports sequences ( in my case, I was using Mysql5.7 ).

  if($schema->getDatabasePlatform()->supportsSequences()) {
      $sequences = $schema->listSequences();
      foreach ($sequences as $s) {
          $schema->dropSequence($s);
      }
  }

Otherwise, I was very happy to find your branch! It helped me upgrade to Laravel 9 almost flawlessly.

seifane-wise · 2022-03-08T15:58:53Z

@jochemfuchs Nice catch ! I pushed a fix for that.
Glad to hear it helped you, let me know if other issue arise.

eigan · 2022-03-03T06:18:07Z

src/Console/ExecuteCommand.php

 {
    use ConfirmableTrait;

    /**
     * The name and signature of the console command.
     * @var string
     */
-    protected $signature = 'doctrine:migrations:execute {version : The version to execute }
+    protected $signature = 'doctrine:migrations:execute {versions : The versions to execute }
    {--connection= : For a specific connection.}


Since we no longer control the $signature (everything sent to Doctrine\Migrations), perhaps we should use Illuminate\Console\Command::specifyParameters() instead? (override and dynamically register the args/options)

I didn't try that, I will try this approach.

dpslwk · 2022-03-29T16:53:28Z

src/Console/DiffCommand.php

-        $pos = strpos($name, '.');
-
-        return false === $pos ? $name : substr($name, $pos + 1);
+        return $command->run($this->getDoctrineInput($command), $this->output->getOutput());


Before (L6, LD/orm 1.5, LD/mig 2.3.1)

$ php artisan doctrine:migrations:diff No changes detected in your mapping information.

After (L9, LD/orm 1.9, LD/mig this)

$ php artisan doctrine:migrations:diff Doctrine\Migrations\Generator\Exception\NoChangesDetected No changes detected in your mapping information. at vendor/doctrine/migrations/lib/Doctrine/Migrations/Generator/Exception/NoChangesDetected.php:13 9▕ final class NoChangesDetected extends RuntimeException implements GeneratorException 10▕ { 11▕ public static function new(): self 12▕ { ➜ 13▕ return new self('No changes detected in your mapping information.'); 14▕ } 15▕ } 16▕ +3 vendor frames 4 /Users/matt/Dropbox/HackspaceInstrumentation/github/laravel-doctrine/migrations/src/Console/DiffCommand.php:43 Symfony\Component\Console\Command\Command::run(Object(Symfony\Component\Console\Input\ArrayInput), Object(Symfony\Component\Console\Output\ConsoleOutput)) +13 vendor frames 18 artisan:37 Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))

i think we should catch the error, this will return to old behaviour

Suggested change

return $command->run($this->getDoctrineInput($command), $this->output->getOutput());

try {

return $command->run($this->getDoctrineInput($command), $this->output->getOutput());

} catch (NoChangesDetected $exception) {

$this->error($exception->getMessage());

return 0;

}

will need use Doctrine\Migrations\Generator\Exception\NoChangesDetected;

dpslwk · 2022-03-29T17:13:20Z

its a little annoying that now all the commands are just passing to doctrine the output is giving bad returns

$ php artisan doctrine:migrations:diff

 Generated new migration class to "/Users/matt/Dropbox/Valet/laravel9-doctrine/database/migrations_doctrine/Version20220329165537.php"
 
 To run just this migration for testing purposes, you can use migrations:execute --up 'Database\\Migrations\\Version20220329165537'
 
 To revert the migration you can use migrations:execute --down 'Database\\Migrations\\Version20220329165537'

it would not be migrations:execute but doctrine:migrations:execute
but there is not an easy fix since this is hard coded

Added error handling in diff command

seifane-wise · 2022-03-30T09:28:24Z

its a little annoying that now all the commands are just passing to doctrine the output is giving bad returns

Yeah. Using directly doctrine commands is better to not have to re-invent the wheel but lacks in some areas. Maybe opening a discussion with them on this subject would be good to try to find a better way.

eigan · 2022-05-29T14:09:40Z

So I tried to use the doctrine commands directly but was hitting some issues. The primary being that we currently support multiple configurations in config/migrations.php.

Doctrine\Migrations\Configuration\Configuration is final, we cannot extend this with something which store multiple configurations, and retrieve config for current entityManager.
... So we need to create a Configuration when we know which config to load. Lazy-creating a Configuration is possible by creating our own ConfigurationLoader, but this class is not aware of the context, and will never receive the InputInterface to determine which --connection is sent.
Symfony does not support multiple entity manager configurations (in migrations bundle). So they do not have this issue and can directly use the doctrine commands.

Unless someone figure out something else, then what you have provided here is the best solution at the moment. It would only break if Doctrine changes the signatures and we no longer match. We could create a test-case to make sure we match though:

<?php

namespace LaravelDoctrine\Migrations\Tests\Integration;

use LaravelDoctrine\Migrations\Console\DiffCommand;
use LaravelDoctrine\Migrations\Console\ExecuteCommand;
use LaravelDoctrine\Migrations\Console\GenerateCommand;
use LaravelDoctrine\Migrations\Console\LatestCommand;
use LaravelDoctrine\Migrations\Console\MigrateCommand;
use LaravelDoctrine\Migrations\Console\StatusCommand;
use LaravelDoctrine\Migrations\Console\VersionCommand;

class CommandConfigurationTests extends \PHPUnit\Framework\TestCase
{
    public function testAllCommandsAreConfiguredCorrectly(): void
    {
        $commands = [
            DiffCommand::class => \Doctrine\Migrations\Tools\Console\Command\DiffCommand::class,
            LatestCommand::class => \Doctrine\Migrations\Tools\Console\Command\LatestCommand::class,
            StatusCommand::class => \Doctrine\Migrations\Tools\Console\Command\StatusCommand::class,
            MigrateCommand::class => \Doctrine\Migrations\Tools\Console\Command\MigrateCommand::class,
            ExecuteCommand::class => \Doctrine\Migrations\Tools\Console\Command\ExecuteCommand::class,
            VersionCommand::class => \Doctrine\Migrations\Tools\Console\Command\VersionCommand::class,
            GenerateCommand::class => \Doctrine\Migrations\Tools\Console\Command\GenerateCommand::class
        ];

        foreach ($commands as $ourCommandClass => $doctrineCommandClass) {
            $ourCommand = new $ourCommandClass();
            $doctrineCommand = new $doctrineCommandClass();

            $ourOptions = array_keys($ourCommand->getDefinition()->getOptions());
            $doctrineOptions = array_keys($doctrineCommand->getDefinition()->getOptions());

            $unknownOptions = [];

            foreach ($ourOptions as $ourOption) {
                if ($ourOption === 'connection') {
                    // Our custom connection option
                    continue;
                }

                if (in_array($ourOption, $doctrineOptions, true) === false) {
                    $unknownOptions[] = $ourOption;
                }
            }

            self::assertEmpty($unknownOptions, 'Option(s) not supported by doctrine command '.$doctrineCommandClass.': ' . implode(", ", $unknownOptions));
        }
    }
}

ollieread · 2022-06-20T10:01:06Z

@eigan @dpslwk Is there anything I could do to help out with getting this PR moving? I am about to start 2 new projects that would require this for Laravel 9, and am more than happy to help out if I can.

gierappa · 2022-10-19T08:01:38Z

@seifane-wise Hi I'm getting error: "The metadata storage is not up to date, please run the sync-metadata-storage command to fix this issue." on php artisan doctrine:migrations:diff command. Looks like my migrations table has diffrent column properties. Works fine on previous version. Any ideas?

gierappa · 2022-10-19T08:34:51Z

I updated version_column_length on config/migrations.php to proper value (the same as I had in the db) and it works fine right now. Looks like it was ignored before.

jeremy-smith-maco · 2022-10-23T23:03:45Z

Been a few months now. Would be good to get this going again for people waiting to upgrade to Laravel 9.

eigan · 2022-12-09T19:40:35Z

config/migrations.php

@@ -85,6 +85,6 @@
        | The length for the version column in the migrations table.
        |
        */
-        'version_column_length' => 14
+        'version_column_length' => 1024


@seifane-wise Do you remember why you set 1024 here and not 191? See updated default her: https://github.com/doctrine/migrations/blob/3.5.x/UPGRADE.md

I know 1024 is used in the docs https://github.com/doctrine/migrations/blob/3.5.x/docs/en/reference/configuration.rst but its actually 191 by default in thier code

eigan · 2022-12-09T19:44:42Z

src/Console/MigrateCommand.php

    /**
     * The name and signature of the console command.
     * @var string
     */
    protected $signature = 'doctrine:migrations:migrate
    {version=latest : The version number (YYYYMMDDHHMMSS) or alias (first, prev, next, latest) to migrate to.}
    {--connection= : For a specific connection }
-    {--write-sql= : The path to output the migration SQL file instead of executing it. }
+    {--write-sql=/tmp : The path to output the migration SQL file instead of executing it. }


Did we have to set /tmp here? Doctrine fallbacks to getcwd() when this value is null. Hardcoding paths will break for us using open basedir :)

eigan · 2022-12-13T14:05:26Z

@seifane-wise Sorry for the delay! I hope everyone have just used your fork instead :)

I did a squash and merge and it ended up giving you credit on multiple authors (1661814). Please let me know if you want to manually squash and adjust this (I can then replace the commit), otherwise we just let it stay as is :)

seifane added 19 commits February 21, 2022 22:59

updated for DBAL 3

1260dca

better handling of commands

b81e8d1

fix typo

b990fbc

fix command arguments

2599a5f

don't pass down command argument

f573ca7

adjust arguments

c741574

Filter arguments with command's definition

8a13fdc

removed deprecated versions from CI

13dedda

fixing diff command

94ce2f0

fix typo

3abbf74

Fix diff command + refacto

9494b52

fix typo

3b85802

Correctly instantiate entity manager

4df9c4e

fix checking filter-expression

2994d87

updated default config

ea57e41

make reset command drop sequences and foreigns before dropping table

f7aae22

handle no interaction

7a3ad51

typo

a389bbf

default to /tmp for write-sql

d485577

seifane-wise mentioned this pull request Mar 2, 2022

[FEATURE] Upgrading to DBAL 3 & Switch to symfony/cache laravel-doctrine/orm#520

Closed

seifane added 2 commits March 7, 2022 18:47

edited ci matrix

ff07b81

cleanup

remove php 7.4 from ci matrix

9764df0

fix: check if database platform supports sequence before dropping them

9b643aa

eigan reviewed Mar 9, 2022

View reviewed changes

This was referenced Mar 24, 2022

Add support for Laravel 9 #117

Closed

Update composer.json #116

Closed

dpslwk reviewed Mar 29, 2022

View reviewed changes

Set default config correctly

203d85a

Added error handling in diff command

eigan reviewed Dec 9, 2022

View reviewed changes

eigan changed the base branch from 2.x to 3.x December 13, 2022 13:52

eigan merged commit 1661814 into laravel-doctrine:3.x Dec 13, 2022

-        return $command->run($this->getDoctrineInput($command), $this->output->getOutput());
+                try {
+            return $command->run($this->getDoctrineInput($command), $this->output->getOutput());
+        } catch (NoChangesDetected $exception) {
+            $this->error($exception->getMessage());
+            return 0;
+        }

[FEATURE] Upgrade to doctrine/migrations 3 #118

[FEATURE] Upgrade to doctrine/migrations 3 #118

Uh oh!

Conversation

seifane-wise commented Mar 2, 2022

Changes proposed in this pull request:

Uh oh!

eigan commented Mar 3, 2022

Uh oh!

eigan commented Mar 3, 2022

Uh oh!

seifane-wise commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpslwk commented Mar 7, 2022

Uh oh!

seifane-wise commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jochemfuchs commented Mar 8, 2022

Uh oh!

seifane-wise commented Mar 8, 2022

Uh oh!

eigan Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

seifane-wise Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

dpslwk Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

seifane-wise Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

dpslwk commented Mar 29, 2022

Uh oh!

seifane-wise commented Mar 30, 2022

Uh oh!

eigan commented May 29, 2022

Uh oh!

ollieread commented Jun 20, 2022

Uh oh!

gierappa commented Oct 19, 2022

Uh oh!

gierappa commented Oct 19, 2022

Uh oh!

jeremy-smith-maco commented Oct 23, 2022

Uh oh!

eigan Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

dpslwk Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

eigan Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

eigan commented Dec 13, 2022

Uh oh!

Uh oh!

seifane-wise commented Mar 3, 2022 •

edited

Loading

seifane-wise commented Mar 7, 2022 •

edited

Loading