Generated SQL no longer adheres to the order of fields

[jannes-io]

BC Break Report

Q	A
BC Break	yes
Version	3.9.1

(ORM: 3.5.0, DBAL: 4.2.4)

Summary

Traits are now appended to the end of the generated SQL in CREATE TABLE instead of where they actually belong when running migrations:diff

Given the following setup:

trait IdentifiableEntity
{
    #[ORM\Id]
    #[ORM\Column(type: Types::INTEGER)]
    #[ORM\GeneratedValue]
    private int $id;

    public function getId(): int
    {
        return $this->id;
    }
}

class MyEntity
{
    use IdentifiableEntity;

    #[ORM\Column(length: 255)]
    public string $name;
}

Previous behavior

It generated:

$this->addSql('CREATE TABLE my_entity (id INT AUTO_INCREMENT NOT NULL, name VARCHAR(255) NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci`');

Current behavior

Now it generates:

$this->addSql('CREATE TABLE my_entity (name VARCHAR(255) NOT NULL, id INT AUTO_INCREMENT NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci`');

How to reproduce

Set up the class above in an empty Symfony project with orm 3.5 and dbal 4.2 installed, and run bin/console doc:mig:dif.
If you can't reproduce, I can create a small demo app.

[stof]

Is this related to an upgrade of doctrine packages or to an upgrade of PHP ?

I think PHP changed at some point the way property lists (from the class itself and its parent class and used traits) are merged to get the final list reported by Reflection APIs.

[stof]

Actually, for traits, this does not even depend on the PHP version. All PHP versions report the trait property after the class property: https://3v4l.org/MGBE4#veol

[jannes-io]

I have been using this pattern for a long time, from PHP7 to PHP8.4. And the issue I’m reporting didn’t happen until I updated all doctrine packages to their latest versions last week.

I know positioning for update table doesn’t work and it always adds to the end, no problem. And I typically don’t really care about order of columns, that’s what orm/dbal is for. But now my IDs are being added as last column, and that’s just a tad too far… so now I must manually edit every create table whereas before I could just diff and mig.

[greg0ire]

Can you please try narrowing it down to a precise commit? Here is some help: https://dev.to/greg0ire/bisecting-vendors-12kd

Anyway, I don't think this should be considered a breaking change. Preserving the order of fields would be nice to have, but it's never been a requirement AFAIK.

This looks familiar, by the way, maybe you're not the first one to report this.

[jannes-io]

Can you please try narrowing it down to a precise commit?

For someone who's intimately involved in checking commits, doing PRs,... (in other words, a Doctrine maintainer), this might be quite straightforward. Perhaps someone can even just pinpoint the change that caused this from memory. For me, what you're asking is hours of work.

I don't even know if it's in this repo, in dbal, or in orm. So I'd have to check all 3. I just created the issue here because it's the diff command that's ultimately showing the BC break, but the responsible code change can be much deeper.

Preserving the order of fields would be nice to have, but it's never been a requirement AFAIK.

If some functionality has an unintentional side-effect, and then this side-effect is removed, regardless if it was "a requirement", it's still a backwards compatibility break, intentional or not...

There are development processes and flows in teams using Doctrine that have rules regarding certain things, like what the database should look like. In our case, the identifier field(s) should always come first, and our processes show this because logically, traits are at the top of the class, so we never had to explicitly mention that devs should double check all of their migrations for column ordering.

Not being able to trust that the output of the commands in this package align with our internal agreements means extra work for developers, extra work for code reviewers (yet another thing for them to look out for),...

[greg0ire]

For me, what you're asking is hours of work.

I am not asking for anything, since I'm not interested in getting this fixed myself. Just trying to help, by giving you pointers on how to do this. Also, you probably shouldn't be telling me that "hours of work" is too much, since I am frequently giving hours of work to the community.

Perhaps someone can even just pinpoint the change that caused this from memory.

Yes, but sometimes that's not the case, as outlined in my blog post. You can wait a bit, maybe somebody will remember, but if nobody answers, you still have the solution I'm describing.

I don't even know if it's in this repo, in dbal, or in orm. So I'd have to check all 3.

Yes. That's exactly how I would do it myself.

If some functionality has an unintentional side-effect, and then this side-effect is removed, regardless if it was "a requirement", it's still a backwards compatibility break, intentional or not...

It's not that black and white. The order of columns will not break your SELECT queries, so in this case, it's highly debatable. I think we would accept a PR restoring the former order though. You may even contribute a test ensuring there is no "regression", but again, this is debatable.

Anyway, as I said, this sounds familiar, so maybe start with a Github search.

[rixafy]

So far I have narrowed it down at least to version, it's caused by DBAL 4.0.0, I created php script to reproduce the issue (php index.php o:s:u --dump-sql) - https://github.com/rixafy/doctrine-bad-column-order, on DBAL 3.10.1 it works (id is first), on 4.0.0 the title is first.

[greg0ire]

Thanks @rixafy !

git merge-base 3.10.1 4.0.0
44b908cacbf5584df30bca3f3ce8edac34f2ac90

So in case one of you wants to narrow it down further using git bisect, the issue probably lies between 44b908cacbf5584df30bca3f3ce8edac34f2ac90 and 4.0.0.