Generates invalid up() migration

[svycka]

Bug Report

Q	A
BC Break	no
Version	1.8.1

Summary

generates incorrect up migration but down is ok

Current behavior

generated migration just throw exception becous it should first remove constrant and then remove index

<?php declare(strict_types=1);

namespace DatabaseMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
final class Version20181205094238 extends AbstractMigration
{
    public function up(Schema $schema) : void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('DROP INDEX idx_7ee5e3886dc044c5 ON fields');
        $this->addSql('CREATE INDEX IDX_7EE5E388D3CB4A96 ON fields (`group`)');
    }

    public function down(Schema $schema) : void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE fields DROP FOREIGN KEY FK_7EE5E388D3CB4A96');
        $this->addSql('DROP INDEX idx_7ee5e388d3cb4a96 ON fields');
        $this->addSql('CREATE INDEX IDX_7EE5E3886DC044C5 ON fields (`group`)');
        $this->addSql('ALTER TABLE fields ADD CONSTRAINT FK_7EE5E388D3CB4A96 FOREIGN KEY (`group`) REFERENCES field_groups (`id`)');
    }
}

How to reproduce

     * @ORM\ManyToOne(targetEntity="FarmField\Entity\FieldGroup")
     * @ORM\JoinColumn(name="group", referencedColumnName="id")
changed to:
     * @ORM\ManyToOne(targetEntity="FarmField\Entity\FieldGroup")
     * @ORM\JoinColumn(name="`group`", referencedColumnName="id")

Expected behavior

generated valid migration maybe like this:

    public function up(Schema $schema) : void
    {
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE fields DROP FOREIGN KEY FK_7EE5E388FE54D947');
        $this->addSql('DROP INDEX idx_7ee5e3886dc044c5 ON fields');
        $this->addSql('CREATE INDEX IDX_7EE5E388D3CB4A96 ON fields (`group`)');
        $this->addSql('ALTER TABLE fields ADD CONSTRAINT FK_7EE5E388FE54D947 FOREIGN KEY (`group`) REFERENCES field_groups (`id`)');
    }

[Ocramius]

@svycka what ORM/DBAL versions? Can you make sure to try the latest stable releases?

[svycka]

$ composer info | grep doctrine                 
creof/doctrine2-spatial             1.2.0    Doctrine2 multi-platform suppo...
doctrine/annotations                v1.6.0   Docblock Annotations Parser
doctrine/cache                      v1.8.0   Caching library offering an ob...
doctrine/collections                v1.5.0   Collections Abstraction library
doctrine/common                     v2.10.0  PHP Doctrine Common project is...
doctrine/dbal                       v2.9.0   Powerful PHP database abstract...
doctrine/doctrine-module            2.1.7    Zend Framework Module that pro...
doctrine/doctrine-orm-module        2.1.2    Zend Framework Module that pro...
doctrine/event-manager              v1.0.0   Doctrine Event Manager component
doctrine/inflector                  v1.3.0   Common String Manipulations wi...
doctrine/instantiator               1.1.0    A small, lightweight utility t...
doctrine/lexer                      v1.0.1   Base library for a lexer that ...
doctrine/migrations                 v1.8.1   Database Schema migrations usi...
doctrine/orm                        v2.6.3   Object-Relational-Mapper for PHP
doctrine/persistence                v1.1.0   The Doctrine Persistence proje...
doctrine/reflection                 v1.0.0   Doctrine Reflection component
gedmo/doctrine-extensions           v2.4.36  Doctrine2 behavioral extensions

[svycka]

@Ocramius I am not sure if it should generate migration at all, because I did not change the name just escaped column name because group is reserved word.

[Ocramius]

Aha, then yes, the migration is actually correct.

Escaped/unescaped have different semantic also at DB level: while in this scenario everything is equivalent, if your identifier had case sensitivity related characters (caSeSenSITivITY), then it may point to a completely different symbol.

This behaviour is correct, just the order of operations isn't.

[svycka]

@Ocramius I am a bit lost how this can be correct:

    public function up(Schema $schema) : void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('DROP INDEX idx_7ee5e3886dc044c5 ON fields');
        $this->addSql('CREATE INDEX IDX_7EE5E388D3CB4A96 ON fields (`group`)');
    }

// vs

    public function down(Schema $schema) : void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE fields DROP FOREIGN KEY FK_7EE5E388D3CB4A96');
        $this->addSql('DROP INDEX idx_7ee5e388d3cb4a96 ON fields');
        $this->addSql('CREATE INDEX IDX_7EE5E3886DC044C5 ON fields (`group`)');
        $this->addSql('ALTER TABLE fields ADD CONSTRAINT FK_7EE5E388D3CB4A96 FOREIGN KEY (`group`) REFERENCES field_groups (`id`)');
    }

why up migration does not need to drop foreign key but down migration requires it? one ore another is incorrect

[Ocramius]

That's possibly yet another bug. The code that migrates things internally may use the quoted version in one scenario, and the unquoted in another.

[svycka]

then it's migrations bug not dbal?

[Ocramius]

More likely a schema comparator+schema manager bug in DBAL.

If you can somehow isolate it via a unit test, that would make it simpler to fix.

[svycka]

@Ocramius anyway I have manualy updated migration to

    public function up(Schema $schema) : void
    {
        $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('ALTER TABLE fields DROP FOREIGN KEY FK_7EE5E388FE54D947');
        $this->addSql('DROP INDEX idx_7ee5e3886dc044c5 ON fields');
        $this->addSql('CREATE INDEX IDX_7EE5E388D3CB4A96 ON fields (`group`)');
        $this->addSql('ALTER TABLE fields ADD CONSTRAINT FK_7EE5E388FE54D947 FOREIGN KEY (`group`) REFERENCES field_groups (`id`)');
    }

and everything migrated succesfuly.

[svycka]

@ocramius I have created https://github.com/doctrine/dbal/pull/3395 what do you think?