mirror of
https://github.com/doctrine/orm.git
synced 2026-04-29 01:13:14 +02:00
PHP8 attributes do not respect uniqueConstraint property from the Table attribute #6935
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @pkly on GitHub (Feb 22, 2022).
Bug Report
Summary
After converting from annotations to attributes I wanted to keep my configuration the same, but it appears that the
uniqueConstraintsproperty is not being read.By directly placing a UniqueConstraint attribute on the entity the behavior is as expected.
Current behavior
Migrations create invalid changes by removing unique indexes from tables. UniqueConstraints are not being recognized.
How to reproduce
With a minimal entity use the following mappings:
Expected behavior
For the array of attributes to work with UniqueConstraint in the table attribute as the uniqueConstraints parameter.
It was not listed in the upgrade docs, so I'm assuming it's a BC break.
@derrabus commented on GitHub (Feb 22, 2022):
Your first example is the correct and documented way to declare a unique constraint with attributes.
Since the code in your second example has never worked, it's by definition not a BC break.
Some background on this: When we introduced attributes, nesting them was not possible yet. I understand that having that constructor parameter causes confusion and I agree that we should do somthing about it.
Since we cannot remove the parameter because we need it for Doctrine Annotations, maybe we should allow both ways (first and second example) for now and remove one of the ways in 3.0?
/cc @beberlei
@pkly commented on GitHub (Feb 22, 2022):
Since an in-place upgrade is not possible I'd still consider it a BC break...
I understand why it doesn't work, but seeing how the 8.1 new in initializers were accepted a while back (https://wiki.php.net/rfc/new_in_initializers) this likely could've been avoided.
As attributes are not yet documented on the doctrine docs for ORM at all I don't think it's not a bc break, since if I didn't find the reader looking at class annotations by myself it'd be rather painful.
Since the argument is there and is needed for annotations I'd highly suggest you still allow it to be used in this use-case. More so because nothing on the attribute even mentions that this element is deprecated or a different usage is suggested.
Maybe if attributes are used and this is detected you should simply add it and fire off a deprecation notice for 3.x?
@beberlei commented on GitHub (Feb 22, 2022):
@derrabus we discussed allowing both ways in #9240 and decided against because of the complexity and confusion of two ways. I'd rather instead throw an exception if Table#uniqueConstraints isset, pointing to the right way. Same for other nested ways that we flattened (Table indexes and JoinTable join columns for example).
@beberlei commented on GitHub (Feb 22, 2022):
@pkly not sure what you mean, attribute mappings are completly documneted here https://www.doctrine-project.org/projects/doctrine-orm/en/2.11/reference/attributes-reference.html#attributes-reference
@pkly commented on GitHub (Feb 22, 2022):
@beberlei sorry, I meant the "Mapping Objects onto a Database" section for 2.11.
Attributes are not listed there, I think I found the attribute reference once via google directly.
As for the solution - throwing sounds like a good idea too, it's just that silently failing on something what is essentially a configuration for a program is not a great idea, and rather confusing.
On top of that the examples in other pages still don't list attributes at all, and since those are not compatible 1:1....
@derrabus commented on GitHub (Feb 22, 2022):
Works for me as well!
@BonBonSlick commented on GitHub (Feb 13, 2024):