Fix for bug #8229 (id column from parent class renamed in child class)

[cziegenberg]

This fixes problems with id columns defined in the parent class but renamed in the child class using an attribute override. Before this change always the child column name was used (which was not present in the parent class), now the correct column names are used for the parent table when creating inserts, joins and deletions for it.

[greg0ire]

Please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
[email protected], that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

1. git rebase -i origin/2.7, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
2. A window will show up with many lines, replace pick with fixup on every line but the first one
3. Close your editor, git should do its magic, and you should end up with one commit
4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

[cziegenberg]

Done I think. Not sure about the e-mail - I had to convert my account to an organization last week and switched to a new account, but GitHub Desktop didn't update the name/e-mail after switching.

PS: Thanks for the rebase instruction. I also had to add ~ + [number of commits] to the first command to get it working.

[cziegenberg]

Just recognized another bug related to this one. The foreign key for the id column in the child table is not set, if it's renamed via AttributeOverride. I think this is an ORM bug, not a migrations problem, right? Will look into it...

[cziegenberg]

The foreign key generation is also fixed now (the "inherited" property of the column has been set correctly, but accidentally removed later, so that the foreign key was not created). After this I also had to fix the generation of the WHERE condition for the id column (which depends on the "inherited" property).

[cziegenberg]

@ostrolucky I implemented all suggested changes.

[cziegenberg]

Just realized an error in the annotations after the last change. Will check that. Sorry.

[cziegenberg]

I fixed the wrong annotation, and I also added a test for the possible foreign key problem when deleting entries from the tables of a joined inheritance entity.

[ostrolucky]

Nice work! :) I'm not comfortable to merge this on my own though, let's see if somebody else will pop in. In case nobody will, ping me in a few weeks and we will proceed anyways.

[cziegenberg]

Hmm, there seems to be a problem with Travis CI:

Fatal error: Uncaught InvalidArgumentException: Extension class "PhpBench\Extensions\Dbal\DbalExtension" does not exist in phar:///home/travis/build/doctrine/orm/phpbench.phar/vendor/phpbench/container/lib/Container.php:54

[greg0ire]

Blocked by #8252

[cziegenberg]

Thanks for looking into the build problem...

Latest changes:

• Fixed a problem with the SQL generation from DQL (wrong columns used in the SELECT, the JOIN and the ORDER BY part)

[cziegenberg]

I just fixed another special case with wrong column names when creating a pessimistic write lock.

[ostrolucky]

Alright I see still nobody merged this, so let's do it. Thanks for your effort :)

[beberlei]

I just stumbled over this and i am considering to revert this change, because i am unsure about the consequences of such a radical change in a patch version, and because the Java Persistence API doesn't allow AttributeOverride to be used on STI or JTI. I am unsure about the consequences of this change and it should be broader discussed and not be part of a patch release.

Unrelated, the change to JoinedSubclassPersister::delete is a behavior change, it looks like a good idea, but is also not acceptable in a patch version.

[cziegenberg]

Thanks @beberlei for reviewing this change.

Following the Doctrine documentation, the attribute overrides feature can be used to override all attribute settings, explicitly including the name (see the provided example). The feature existed before, but it didnt't work in all cases. So I cannot see a behavior change here, but a bugfix.

---

I checked the change in JoinedSubclassPersister::delete() again and I will try to explain it:

Before:

1. When foreign keys were supported by DBAL:
   Delete the entry from the root table (and let the delete cascade).
2. When foreign keys were not supported by DBAL:
   Delete all entries separately (the order of deletion didn't matter, because no support for foreign keys).
3. Bug: When foreign keys are supported by DBAL but missing in the DB:
   The delete in 1) succeeded, but wasn't cascaded (some entries remained).

After:

1. The first delete from the root table cascades, when a foreign key exists.
2. If the Nth delete didn't cascade, the (N+1)th delete removes the related entries. If a cascade happended before, the Nth delete doesn't do anything. I changed the order, because I tried to keep the old behavior, but avoid duplicate code as requested in review (and because the order of deletion didn't matter anyway in this case).
3. Bug fixed.

So IMHO it's also just a bugfix and no behavior change, BUT I think, that this can be improved, because I missed another potential bug (which also existed before):
Doctrine ORM always expects foreign keys to be cascading here, but what if the foreign key action is changed to "no action" or "restrict"? Then the delete would fail (and it would have failed in the previous version).

So I'd suggest to fix this as follows:
Do not rely on any expected, unverified foreign key or foreign key action, but delete all entries in the order child to root separately.

[beberlei]

@cziegenberg ah thanks for linking the documentation it actually says this is only working with mapped superclasses:

Used to override a mapping for an entity field or relationship. May be applied to an entity that extends a mapped superclass to override a relationship or field mapping defined by the mapped superclass.

As for the JTI and delete. It was the previous behavior that a foreign key with delete cascade had to be configured. For 10 years I cannot remember a single discussion or bug related to people expecting this to work different. Your approach is a bit safer, but its also inefficient, because it can potentially lead to increase number of DELETE statements.

[cziegenberg]

@beberlei
Yes, additional delete statements that IMHO should perform the same (because in the end they do the same), but in a safer way.

Regarding the JTI and attribute overrides. Okay, the documentation only mentiones the mapped superclass, but I only read this as the typical example. And attribute overrides have also been processed for JTI before, they worked, except for identifiers.

[ostrolucky]

Documentation also provides example where attribute overrides are used without mapped superclass https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/tutorials/override-field-association-mappings-in-subclasses.html

[beberlei]

Just to clarify, this documentation example is for a trait, which is copied into the same entity, so it is not using inheritance either.

@cziegenberg Attribute overrides for regular field mappings in JTI are broken though. You cannot query these fields across the class structure anymore and if the column is not nullable on the parent class, then it breaks during inserting. "It works" for me just means it accidently does what it should in 50% of the use cases, but we forgot to prevent it.

[cziegenberg]

I think what @ostrolucky wanted to say is that a mapped superclass isn't always required, while you said it's "only working with mapped superclasses". This doesn't mean other types of inheritance were planned to be supported, but that the documentation isn't that clear.

I also misunderstood the documentation, and yes, it worked for me (after the fix) and it passed all tests (also for other use cases), so from my point of view everything was fine. You have an overview of all the use cases and features you want to support. If you say it's too risky or causes problems with other use cases that cannot be fixed, then you have to revert the changes (and prevent the wrong usage of this feature). I have to accept this.