Changes are detected, and migration created, when there is no change

hgraca commented 3 years ago

Bug Report

Q	A
BC Break	no
Version	3.2.1

Summary

We generate migrations, and run them. This should put the ORM metadata and the actual DB in sync.

However, if we generate migrations again, it will detect changes and generate some stuff again.

Current behavior

It generates migrations when the DB and metadata are in sync.

If we apply the generated migration and generate migrations again, it generates another migration, equal to the previous one.

This seems to be related to the charset and collation.

How to reproduce

I created a repository to reproduce the issue: https://github.com/hgraca/migrations-shiet-show

It uses docker, so you can just checkout the repository and run the following in the CLI:

make docker-create-network
make docker-up
make docker-install-deps
make docker-generate-migration
make docker-run-migrations
make docker-generate-migration

Check the last migration file created, in build/Migration/Version.

We can try running that migration and creating a new one, surely that would put everything in sync...

make docker-run-migrations
make docker-generate-migration

Check the new migration created.

It's exactly the same as the previous one, except for the class name.

Other info

I tried applying the changes (currently commented out) in:

config/doctrine/mappings/components/CM.GlobalTicket.Core.Component.Channel.Domain.Channel.php
- Tweaking this, it can actually get to the point of creating up() and down() methods that are exactly the same.
config/packages/doctrine.php

Unfortunately nothing works.

Expected behavior

After generating a migration and applying it, the DB and mapping should be in sync, therefore generating a new migration should result in no new migrations.

Let me know if I can be of any assistance. I tried debugging this with xdebug but it gets very confusing. I appreciate any help you can give. And tkx for all your work on Doctrine & friends.

goetas commented 3 years ago

What is the SQL that is generated each time? probably there is a misconfiguration for the custom types you are adding to your app

hgraca commented 3 years ago

Tkx for your reply @goetas, and sorry for taking long to reply

So, using the example I have above,

the doctrine configurations this: Doctrine config

<?php

declare(strict_types=1);

use Acme\App\Infrastructure\Persistence\Doctrine\Type\ChannelConfigType;
use Acme\App\Infrastructure\Persistence\Doctrine\Type\ChannelIdType;
use Acme\App\Infrastructure\Persistence\Doctrine\Type\ChannelStateEnumType;
use Acme\App\Infrastructure\Persistence\Doctrine\Type\ChannelTypeEnumType;
use Acme\App\Infrastructure\Persistence\Doctrine\Type\ChannelTypeIdType;
use Acme\App\Infrastructure\Persistence\Doctrine\Type\ClientIdType;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

/** @var string[] $ignoredTables */
$ignoredTables = require __DIR__ . '/doctrine_migrations_ignored_tables.php';

return function (ContainerConfigurator $container) use ($ignoredTables): void {
    $container->extension('doctrine', [
        'dbal' => [
            'url' => '%env(resolve:DATABASE_URL)%',
//            'charset' => 'utf8mb4',
//            'default_table_options' => [
//                'engine' => 'InnoDB',
//                'charset' => 'utf8mb4',
//                'collate' => 'utf8mb4_unicode_ci',
//            ],
            # the DBAL driverOptions option
//            'options' => [
//                'charset' => 'utf8mb4', // characterset ?
//                'collate' => 'utf8mb4_unicode_ci', // collation ?
//            ],
            'mapping_types' => [
                'enum' => 'string',
            ],
            'types' => [
                'channel_id' => ChannelIdType::class,
                'channel_config' => ChannelConfigType::class,
                'channel_state_enum_type' => ChannelStateEnumType::class,
                'channel_type_id' => ChannelTypeIdType::class,
                'channel_type_enum' => ChannelTypeEnumType::class,
                'client_id' => ClientIdType::class,
            ],
            // Only consider tables with names matching the pattern.
            // The pattern is negating (negative lookaround), so it will ignore any table in the list.

            'schema_filter' => $ignoredTables === [] ? null : '~^(?!' . implode('|', $ignoredTables) . ')~',
        ],
        'orm' => [
            'auto_generate_proxy_classes' => true,
            'naming_strategy' => 'doctrine.orm.naming_strategy.underscore_number_aware',
            'auto_mapping' => true,
            'mappings' => [
                'Acme\App\Core\Component' => [
                    'is_bundle' => false,
                    'type' => 'php',
                    'dir' => '%kernel.project_dir%/config/doctrine/mappings/components',
                    'prefix' => 'Acme\App\Core\Component',
                ],
            ],
        ],
    ]);
};

The entities configurations are these: Channel entity

<?php

declare(strict_types=1);

use Acme\App\Core\Component\Channel\Domain\ChannelStateEnum;
use Acme\App\Core\Component\Channel\Domain\ChannelType;
use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder;
use Doctrine\ORM\Mapping\ClassMetadata;

/** @var ClassMetadata $metadata */
$builder = new ClassMetadataBuilder($metadata);

$builder->setTable('channel__channel')
    ->addField(
        'id',
        'channel_id',
        [
            'id' => true,
//            'options' => [
//                'charset' => 'utf8mb4',
//                'collation' => 'utf8mb4_unicode_ci',
//            ],
        ]
    )
    ->addField(
        'clientId',
        'client_id',
        [
            'nullable' => false,
        ]
    )
    ->createManyToOne('channelType', ChannelType::class)
        ->addJoinColumn($columnName = 'channel_type_id', $referencedColumnName = 'id', $nullable = false)
        ->build()
    ->addField(
        'config',
        'channel_config',
        [
            'nullable' => false,
        ]
    )
    ->addField(
        'stateEnum',
        'channel_state_enum_type',
        [
            'length' => 50,
            'nullable' => false,
            'default' => (string) ChannelStateEnum::disabled(),
        ]
    );
$metadata->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_NONE);

ChannelType entity

<?php

declare(strict_types=1);

use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder;
use Doctrine\ORM\Mapping\ClassMetadata;

/** @var ClassMetadata $metadata */
$builder = new ClassMetadataBuilder($metadata);

$builder->setTable('channel__channel_type')
    ->addField(
        'id',
        'channel_type_id',
        ['id' => true]
    )
    ->addField(
        'name',
        'channel_type_enum',
        [
            'nullable' => false,
            'length' => 100,
        ]
    )
    ->addField(
        'enabled',
        'boolean',
        [
            'nullable' => false,
            'default' => 0,
        ]
    );
$metadata->setIdGeneratorType(ClassMetadata::GENERATOR_TYPE_NONE);

generating the first migration results in this:

    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('CREATE TABLE channel__channel (
          id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_id)\',
          channel_type_id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\',
          client_id INT NOT NULL COMMENT \'(DC2Type:client_id)\',
          config JSON NOT NULL COMMENT \'(DC2Type:channel_config)\',
          state_enum VARCHAR(50) DEFAULT \'DISABLED\' NOT NULL,
          INDEX IDX_A1011C93720FB392 (channel_type_id),
          PRIMARY KEY(id)
        ) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB');
        $this->addSql('CREATE TABLE channel__channel_type (
          id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\',
          name VARCHAR(100) NOT NULL COMMENT \'(DC2Type:channel_type_enum)\',
          enabled TINYINT(1) DEFAULT \'0\' NOT NULL,
          PRIMARY KEY(id)
        ) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB');
        $this->addSql('CREATE TABLE refresh_tokens (
          id INT AUTO_INCREMENT NOT NULL,
          refresh_token VARCHAR(128) NOT NULL,
          username VARCHAR(255) NOT NULL,
          valid DATETIME NOT NULL,
          UNIQUE INDEX UNIQ_9BACE7E1C74F2195 (refresh_token),
          PRIMARY KEY(id)
        ) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB');
        $this->addSql('ALTER TABLE
          channel__channel
        ADD
          CONSTRAINT FK_A1011C93720FB392 FOREIGN KEY (channel_type_id) REFERENCES channel__channel_type (id)');
    }

Then applying the migration and generating again, results in this:

    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE
          channel__channel
        CHANGE
          id id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_id)\',
        CHANGE
          channel_type_id channel_type_id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\',
        CHANGE
          state_enum state_enum VARCHAR(50) DEFAULT \'DISABLED\' NOT NULL');
        $this->addSql('ALTER TABLE
          channel__channel_type
        CHANGE
          id id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\'');
    }

And applying the migration again and generating again, results in this:

    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE
          channel__channel
        CHANGE
          id id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_id)\',
        CHANGE
          channel_type_id channel_type_id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\',
        CHANGE
          state_enum state_enum VARCHAR(50) DEFAULT \'DISABLED\' NOT NULL');
        $this->addSql('ALTER TABLE
          channel__channel_type
        CHANGE
          id id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\'');
    }

Any ideas?

stof commented 2 years ago

which platform are you using ?

greg0ire commented 2 years ago

Might be fixed by #1229 . Please try out 3.4.0

hgraca commented 2 years ago

@stof Sorry for the late reply. What do you mean by platform?

@greg0ire Unfortunately I cant install version 3.4:

$ ./composer.phar update -W "doctrine/doctrine-migrations-bundle:3.4.0" 
PHP temp directory (/tmp) does not exist or is not writable to Composer. Set sys_temp_dir in your php.ini
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires doctrine/doctrine-migrations-bundle ^3.2.1, 3.4.0, found doctrine/doctrine-migrations-bundle[v1.0.0-beta1, ..., 1.3.x-dev, v2.0.0-alpha1, ..., 2.2.x-dev, 3.0.0-alpha.1, ..., 3.3.x-dev] but it does not match the constraint.

$ ./composer.phar why-not "doctrine/doctrine-migrations-bundle" "3.4"
PHP temp directory (/tmp) does not exist or is not writable to Composer. Set sys_temp_dir in your php.ini
There is no installed package depending on "doctrine/doctrine-migrations-bundle" in versions not matching 3.4

My composer.json is currently this:

{
    "type": "project",
    "license": "proprietary",
    "minimum-stability": "stable",
    "prefer-stable": true,
    "require": {
        "php": "^7.2.24",
        "ext-ctype": "*",
        "ext-iconv": "*",
        "ext-intl": "*",
        "ext-json": "*",
        "ext-openssl": "*",
        "composer/package-versions-deprecated": "1.11.99.1",
        "doctrine/doctrine-bundle": "^2.4.2",
        "doctrine/doctrine-migrations-bundle": "^3.2.1",
        "doctrine/migrations": "^3.2.1",
        "doctrine/orm": "^2.9.3",
        "gesdinet/jwt-refresh-token-bundle": "^0.11.1",
        "guzzlehttp/guzzle": "7.3.0",
        "lavary/crunz": "^v2.3.1",
        "league/flysystem": "^2.2.0",
        "league/flysystem-sftp": "^2.1.1",
        "league/uri-components": "2.2.0",
        "lexik/jwt-authentication-bundle": "^v2.12.6",
        "nyholm/psr7": "^1.4.1",
        "psr/http-message": "^1.0.1",
        "ramsey/uuid": "^4.2.0",
        "spatie/url-signer": "1.2.1",
        "symfony/console": "^v5.2.14",
        "symfony/doctrine-messenger": "^v5.2.12",
        "symfony/dotenv": "^v5.2.14",
        "symfony/expression-language": "^v5.2.12",
        "symfony/flex": "^v1.13.4",
        "symfony/framework-bundle": "^v5.2.12",
        "symfony/monolog-bundle": "^v3.7.0",
        "symfony/proxy-manager-bridge": "^v5.2.12",
        "symfony/psr-http-message-bridge": "^v2.1.1",
        "symfony/security-bundle": "^v5.2.12",
        "symfony/twig-bundle": "^v5.2.12",
        "symfony/uid": "^v5.2.11",
        "symfony/yaml": "^v5.2.14",
        "twig/twig": "^2.12|^3.0"
    },
    "require-dev": {
        "dama/doctrine-test-bundle": "^v6.6.0",
        "doctrine/doctrine-fixtures-bundle": "^3.4.0",
        "friendsofphp/php-cs-fixer": "^v2.19.1",
        "malukenho/mcbumpface": "^1.1.5",
        "psalm/plugin-phpunit": "^0.15.2",
        "qossmic/deptrac-shim": "0.14.0",
        "roave/security-advisories": "dev-latest",
        "symfony/browser-kit": "^v5.2.12",
        "symfony/phpunit-bridge": "^v5.3.4",
        "symfony/stopwatch": "^v5.2.12",
        "symfony/var-dumper": "^v5.2.12",
        "symfony/web-profiler-bundle": "^v5.2.13",
        "vimeo/psalm": "^4.9.2"
    },
    "config": {
        "optimize-autoloader": true,
        "preferred-install": {
            "*": "dist"
        },
        "sort-packages": true,
        "allow-plugins": {
            "composer/package-versions-deprecated": true,
            "malukenho/mcbumpface": true,
            "symfony/flex": true
        }
    },
    "autoload": {
        "psr-4": {
            "Acme\\App\\": "src/",
            "Acme\\App\\Presentation\\Api\\GraphQl\\Generated\\": "var/cache/graphql/generated",
            "Acme\\App\\Build\\": "build/",
            "Acme\\PhpExtension\\": "lib/php-extension/src/"
        }
    },
    "autoload-dev": {
        "psr-4": {
            "Acme\\PhpExtension\\Test\\": "lib/php-extension/tests/",
            "Acme\\App\\Test\\": "tests/"
        }
    },
    "replace": {
        "symfony/polyfill-ctype": "*",
        "symfony/polyfill-iconv": "*",
        "symfony/polyfill-php72": "*"
    },
    "scripts": {
        "auto-scripts": {
            "cache:clear": "symfony-cmd",
            "assets:install %PUBLIC_DIR%": "symfony-cmd"
        },
        "post-install-cmd": [
            "@auto-scripts"
        ],
        "post-update-cmd": [
            "@auto-scripts"
        ]
    },
    "conflict": {
        "symfony/symfony": "*"
    },
    "extra": {
        "symfony": {
            "allow-contrib": false,
            "require": "5.2.*"
        }
    }
}

Any suggestions?

greg0ire commented 2 years ago

Yes. Try v3.4.0 of doctrine/migrations, not doctrine/doctrine-migrations-bundle

hgraca commented 2 years ago

Yep, it's confirmed, I'm an idiot! 😬

I will try it as soon as I have some time.

Tkx for the heads up.

hgraca commented 2 years ago

@greg0ire tkx for the suggestion, but it didn't work, the issue is still there. :(

greg0ire commented 2 years ago

:thinking: what version of doctrine/dbal are you using? It's important as well because of https://www.doctrine-project.org/2021/11/26/dbal-3.2.0.html Platform-aware schema comparison.

hgraca commented 2 years ago

We are using doctrine/dbal:2.13.5

Unfortunately we are still on PHP 7.2, and will be so for a few more months :(

But I can actually check in the dummy project I created to reproduce this issue, if updating to the latest versions solves the issue. I will do that asap.

greg0ire commented 2 years ago

Ok, FYI https://www.doctrine-project.org/2022/01/22/sunsetting-dbal-2.html

greg0ire commented 2 years ago

As mentioned in the blog post, we do not support v2 for bugfixes anymore.

hgraca commented 2 years ago

Np, totally understandable, tkx for the heads up.

If updating to the latest version does the trick, I will be very happy with that :)

hgraca commented 2 years ago

Just tried it, and it didnt solve the issue :(

With a clean DB it generates

    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('CREATE TABLE channel__channel (
          id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_id)\',
          channel_type_id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\',
          client_id INT NOT NULL COMMENT \'(DC2Type:client_id)\',
          config JSON NOT NULL COMMENT \'(DC2Type:channel_config)\',
          state_enum VARCHAR(50) DEFAULT \'DISABLED\' NOT NULL COMMENT \'(DC2Type:channel_state_enum)\',
          INDEX IDX_A1011C93720FB392 (channel_type_id),
          PRIMARY KEY(id)
        ) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB');
        $this->addSql('CREATE TABLE channel__channel_type (
          id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\',
          name VARCHAR(100) NOT NULL COMMENT \'(DC2Type:channel_type_enum)\',
          enabled TINYINT(1) DEFAULT 0 NOT NULL,
          PRIMARY KEY(id)
        ) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB');
        $this->addSql('CREATE TABLE refresh_tokens (
          id INT AUTO_INCREMENT NOT NULL,
          refresh_token VARCHAR(128) NOT NULL,
          username VARCHAR(255) NOT NULL,
          valid DATETIME NOT NULL,
          UNIQUE INDEX UNIQ_9BACE7E1C74F2195 (refresh_token),
          PRIMARY KEY(id)
        ) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB');
        $this->addSql('ALTER TABLE
          channel__channel
        ADD
          CONSTRAINT FK_A1011C93720FB392 FOREIGN KEY (channel_type_id) REFERENCES channel__channel_type (id)');
    }

I run that migration and generate migrations again and I get this:

final class Version20220127115614 extends AbstractMigration
{
    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE
          channel__channel
        CHANGE
          id id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_id)\',
        CHANGE
          channel_type_id channel_type_id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\'');
        $this->addSql('ALTER TABLE
          channel__channel_type
        CHANGE
          id id CHAR(26) NOT NULL COMMENT \'(DC2Type:channel_type_id)\'');
    }

    public function down(Schema $schema): void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE
          channel__channel
        CHANGE
          id id CHAR(26) CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_unicode_ci` COMMENT \'(DC2Type:channel_id)\',
        CHANGE
          channel_type_id channel_type_id CHAR(26) CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_unicode_ci` COMMENT \'(DC2Type:channel_type_id)\'');
        $this->addSql('ALTER TABLE
          channel__channel_type
        CHANGE
          id id CHAR(26) CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_unicode_ci` COMMENT \'(DC2Type:channel_type_id)\'');
    }
}

greg0ire commented 2 years ago

Ok… if you debug it, I think you will end up here: https://github.com/doctrine/migrations/blob/e17a946a9d3693cc2f3c285e6667522ded237f71/lib/Doctrine/Migrations/Generator/DiffGenerator.php#L93-L105

If you do, and you find that the issue comes from DBAL, try reproducing the issue with the DBAL API (by creating a schema with the DBAL API), and then I would transfer that issue to the DBAL repository.

If you can publish your dummy project, that can help too.

hgraca commented 2 years ago

My dummy project is here: https://github.com/hgraca/migrations-shiet-show The explanation on how to reproduce is in the first post in this thread. Tonight I will push the update with the latest versions, debug and probably transfer the issue to deal then.

Tkx for your support in any case, very appreciated.

greg0ire commented 2 years ago

I don't think you actually have a "Transfer issue" button, but as an admin, I do, so ping me when you think it's time :)

hgraca commented 2 years ago

@greg0ire I commited the update to php 8 and the latest versions of the libraries.

I also debuged and used a break point where you mentioned. I can see this: Screenshot from 2022-01-27 22-54-31 The generated migration up and down methods are exactly the same...

Feel free to transfer this issue to the dbal project, if you feel that's the way to go to. I really cant figure out if the issue is on the dbal side or this side.

greg0ire commented 2 years ago

I will try your dummy project to be sure, and then we'll see

greg0ire commented 2 years ago

This comment is really interesting: https://github.com/doctrine/dbal/blob/3.3.x/src/Platforms/MySQL/Comparator.php#L15-L17

greg0ire commented 2 years ago

I couldn't make the dummy project work: https://github.com/hgraca/migrations-shiet-show/issues/1

hgraca commented 2 years ago

Uff, some owner issues and maybe folders missing.

I will solve it this afternoon and test it from scratch.

Sorry for wasting your time.

greg0ire commented 2 years ago

No worries :slightly_smiling_face:

hgraca commented 2 years ago

Did some changes and pushed. if you clone and run

make docker-create-network # You only need to run this once
make docker-run-test

it should work. It will take a while cozz it will need to create the PHP container.

if you want to enable xdebug, you can do it by uncommenting the first line in docker/xdebug.ini

greg0ire commented 2 years ago

make docker-run-test
make[1]: Entering directory '/tmp/migrations-shiet-show'
[+] Running 0/0
 ⠋ Container 4cffa36eecb9_docker-shietshow-mysql-1  Recreate                                                                                                                                                                               0.0s
Error response from daemon: invalid mount config for type "bind": bind source path does not exist: /tmp/migrations-shiet-show/var/mysql

:stuck_out_tongue:

hgraca commented 2 years ago

TL;DR; If you remove the container (not the image), and run the commands again it should be fine.

I had the same. Its because I removed some mounts from the docker-compose.yml. But the container that exists in the system still had the mount. Some weird business.

greg0ire commented 2 years ago

Ok, I got it to work!

greg0ire commented 2 years ago

Here is a zoom on the diff

^ Doctrine\DBAL\Schema\ColumnDiff^ {#486
  +oldColumnName: "id"
  +column: Doctrine\DBAL\Schema\Column^ {#465
    #_type: Acme\App\Infrastructure\Persistence\Doctrine\Type\ChannelIdType^ {#265}
    #_length: null
    #_precision: 10
    #_scale: 0
    #_unsigned: false
    #_fixed: false
    #_notnull: true
    #_default: null
    #_autoincrement: false
    #_platformOptions: array:1 [
      "version" => false
    ]
    #_columnDefinition: null
    #_comment: null
    #_customSchemaOptions: array:2 [
      "charset" => "utf8mb4"
      "collation" => "utf8mb4_unicode_ci"
    ]
    #_name: "id"
    #_namespace: null
    #_quoted: false
  }
  +changedProperties: array:2 [
    0 => "length"
    1 => "fixed"
  ]
  +fromColumn: Doctrine\DBAL\Schema\Column^ {#342
    #_type: Acme\App\Infrastructure\Persistence\Doctrine\Type\ChannelIdType^ {#265}
    #_length: 26
    #_precision: 10
    #_scale: 0
    #_unsigned: false
    #_fixed: true
    #_notnull: true
    #_default: null
    #_autoincrement: false
    #_platformOptions: array:2 [
      "charset" => "utf8mb4"
      "collation" => "utf8mb4_unicode_ci"
    ]
    #_columnDefinition: null
    #_comment: null
    #_customSchemaOptions: []
    #_name: "id"
    #_namespace: null
    #_quoted: false
  }
}

The things that are detected as changed are not the charset or collation, but fixed and length :thinking:

greg0ire commented 2 years ago

Oh, it looks like platform-aware comparison is not in use here :thinking: Comparator::$platform is null.

greg0ire commented 2 years ago

I think you reported that issue to the correct package, and I think the problem is here: https://github.com/doctrine/migrations/blob/e17a946a9d3693cc2f3c285e6667522ded237f71/lib/Doctrine/Migrations/Metadata/Storage/TableMetadataStorage.php#L187

greg0ire commented 2 years ago

Oh and also we use compareSchemas(), which uses new self(). @morozov , shouldn't this method be deprecated? or should the new self part be somehow avoided when calling that method non-statically?

https://github.com/doctrine/migrations/blob/e17a946a9d3693cc2f3c285e6667522ded237f71/lib/Doctrine/Migrations/Generator/DiffGenerator.php#L104

https://github.com/doctrine/migrations/blob/e17a946a9d3693cc2f3c285e6667522ded237f71/lib/Doctrine/Migrations/Generator/DiffGenerator.php#L115

greg0ire commented 2 years ago

Even when overcoming this with various fixes and hacks, the 2 columns are considered different at the SQL level:

^ " CHAR(26) NOT NULL COMMENT '(DC2Type:channel_id)'"
^ " CHAR(26) CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_unicode_ci` COMMENT '(DC2Type:channel_id)'"

morozov commented 2 years ago

~~It should have been deprecated.~~ By the same token, Schema::getMigrateFromSql() and Schema::getMigrateToSql() were deprecated since they instantiate a comparator.

Hold on:

This method should be called non-statically since it will be declared as non-static in the next major release.

This will make it work properly in the next major (https://github.com/doctrine/dbal/pull/4722) but I'm not sure what to do now.

greg0ire commented 2 years ago

I was trying to figure out which one comes from PHP and which one comes from MySQL by changing the doctrine type name from channel_id to something else, and both SQL definitions changed… I would expect one to come from the database :thinking: I'm super confused right now.

greg0ire commented 2 years ago

If it's possible to detect how it is called (isset($this) or something more complicated?), use a ternary to pick between new self() and $this.

morozov commented 2 years ago

IIRC, it was possible in PHP 4 but not now. Unlike the accepted answer above, in our case, the method is explicitly marked as static. See https://github.com/doctrine/dbal/pull/4707#discussion_r667459365 for context.

greg0ire commented 2 years ago

__call and __callStatic is the only way possible to make both ways work AFAIK.
I wouldn't complicate it that much.

Maybe we have to?

morozov commented 2 years ago

I'm not sure if replacing a method with the magic ones could be counted as backward-compatible. But it looks like worth exploring.

greg0ire commented 2 years ago

It could be considered a BC-break for method_exists() I suppose… alternatively, we could have an optional $comparator parameter and deprecate not passing it. Then, in 4.0, we deprecate passing it. So what do we pick… 💩 or 💩?

morozov commented 2 years ago

An optional parameter will be an immediate BC break: https://3v4l.org/PmUnp

morozov commented 2 years ago

I don't feel good about replacing the method with the magic ones and then introducing it back in 4.0. It's one of those cases when the cure is worse than the disease. Instead, we could be either of:

We introduce a new method in 3.x that will be removed in 4.0. It's the same as the option suggested about but w/o a BC break.
We don't introduce anything and recommend the affected API consumers replicate the correct comparison logic in their code.

Personally, I'd like to release new versions more often than to put band-aids on the poorly designed ones.

greg0ire commented 2 years ago

Just when I was going to suggest we use func_get_args()…

From the consumer standpoint, it would be better than 2. though, I think.

So I'd say either 1. or func_get_args(). Since func_get_args() also involves a cleanup step after upgrading to DBAL 4, we might as well go with 1.

morozov commented 2 years ago

Or actually, the temporary parameter can be introduced via func_get_args() without changing the method signature.

greg0ire commented 2 years ago

Yes, that's what I was proposing… it still involves adding a parameter in this package, then cleaning it up after requiring DBAL 4.

BTW, the ORM is also affected by this: https://github.com/doctrine/orm/blob/223b2650c46d97fcfcabfc577e939cef51915e94/lib/Doctrine/ORM/Tools/SchemaTool.php#L945

This is how it makes the upgrade path look like

Comparator::compareSchemas($fromSchema, $toSchema); // the past
$comparator->compareSchemas($fromSchema, $toSchema, $comparator); // ugly and static analysis won't shut up about it
$comparator->compareSchemas($fromSchema, $toSchema); // in a distant future

morozov commented 2 years ago

This looks fine to me. Those who will want to adopt the temporary solution shouldn't have a problem to update their static analysis configuration. The distant future may happen as soon as the ORM starts supporting DBAL 4.

greg0ire commented 2 years ago

For direct consumers yes, for doctrine/migration it will happen when we drop DBAL 3, but I guess that's OK :slightly_smiling_face:

greg0ire commented 2 years ago

@hgraca can you please update your test project to doctrine/dbal 3.3.1 and doctrine/migrations 3.4.1? I don't think it will fix your issue but it will be less confusing since we are expecting platform-aware comparison to work here.

hgraca commented 2 years ago

@greg0ire done.

greg0ire commented 2 years ago

I've tried connecting to the database with a MySQL client.

Here is the definition of channel__channel

MySQL [shietshow]> SHOW CREATE TABLE channel__channel\G
*************************** 1. row ***************************
       Table: channel__channel
Create Table: CREATE TABLE `channel__channel` (
  `id` char(26) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT '(DC2Type:channel_id)',
  `channel_type_id` char(26) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT '(DC2Type:channel_type_id)',
  `client_id` int(11) NOT NULL COMMENT '(DC2Type:client_id)',
  `config` json NOT NULL COMMENT '(DC2Type:channel_config)',
  `state_enum` varchar(50) COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'DISABLED' COMMENT '(DC2Type:channel_state_enum)',
  PRIMARY KEY (`id`),
  KEY `IDX_A1011C93720FB392` (`channel_type_id`),
  CONSTRAINT `FK_A1011C93720FB392` FOREIGN KEY (`channel_type_id`) REFERENCES `channel__channel_type` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci
1 row in set (0.000 sec)

Now let's try to execute part of the migration:

ALTER TABLE           channel__channel         CHANGE           id id CHAR(26) CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_unicode_ci` COMMENT '(DC2Type:channel_id)';

And now let's check the definition again:

MySQL [shietshow]> SHOW CREATE TABLE channel__channel\G
*************************** 1. row ***************************
       Table: channel__channel
Create Table: CREATE TABLE `channel__channel` (
  `id` char(26) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT '(DC2Type:channel_id)',
  `channel_type_id` char(26) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT '(DC2Type:channel_type_id)',
  `client_id` int(11) NOT NULL COMMENT '(DC2Type:client_id)',
  `config` json NOT NULL COMMENT '(DC2Type:channel_config)',
  `state_enum` varchar(50) COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'DISABLED' COMMENT '(DC2Type:channel_state_enum)',
  PRIMARY KEY (`id`),
  KEY `IDX_A1011C93720FB392` (`channel_type_id`),
  CONSTRAINT `FK_A1011C93720FB392` FOREIGN KEY (`channel_type_id`) REFERENCES `channel__channel_type` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci
1 row in set (0.001 sec)

Let's diff the column definition for id:

- `id` char(26) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT '(DC2Type:channel_id)',
+ `id` char(26) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT '(DC2Type:channel_id)',

As you can see, there is no diff at all, for some reason.

Next steps:

Find out why.
Contribute to the DBAL so that it knows there is no point in doing this?

gilles-g commented 2 years ago

For me, I have downgraded doctrine-bundle to 2.5.5, because of this requirement doctrine/dbal ^2.13.1|^3.1

doctrine-bundle 2.5.6 used doctrine/dbal ^2.13.1|^3.3.2

And after I could downgrade dbal to 3.2.0 instead of 3.3.2... Fix my problem for the moment!

doctrine / migrations