-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated order of mapping attribute parameters #10964
Updated order of mapping attribute parameters #10964
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change for people not using named arguments. I don't think there is a way for us to deprecate not using named arguments, which means I don't see a way to do this on 3.0.x AND provide an upgrade path. Hopefully somebody will.
You're right. I haven't thought about this as been using named arguments since the beginning. Just wondering how big is usage without named arguments. Maybe we can start some wider discussion about it ? I know this is a minor thing but I'm surprised it wasn't picked before. Also technically an upgrade would be to swap order of arguments for Index and OneToMany on entities which shouldn't be too much work (although boring one). I could see this going to 3.0 since 3.0 already have breaking changes comparing to 2.x |
By upgrade path, I mean some change you could do while on 2.x that would allow you to be forward-compatible with 3.0.x (meaning you wouldn't need to perform any change after updating to 3.0.0) |
But since I don't think a lot of people omit argument names, I agree that this can go to 3.0.x. Please retarget, and add something about this in UPGRADE.md |
49e69ed
to
5fd4d46
Compare
Done. Let me know if you need Different / better explanation in UPGRADE.md Thanks |
5fd4d46
to
a7b9e8f
Compare
Am I understanding it correctly that this change is introduced to satisfy one IDE or IDE plugin? |
2b77579
to
1c50149
Compare
Hi @SenseException , not just to satisfy IDE but also to keep arguments of mapping classes in order and according to what you have in doctrine docs which I personally think will be more intuitive. |
1c50149
to
7e51a09
Compare
The PhpStorm inspection is bogus. I agree that the inspection complaining is not enough to motivate this change. However, the parameters are currently ordered inconsistently which is something we can fix in 3.0. |
Thanks @soltmar ! |
Currently, when defining entities, PhpStorm complains about order of Doctrine Mapping PHP Attribute parameter order being wrong:
This PR changes parameter order for \Doctrine\ORM\Mapping\Index.php and \Doctrine\ORM\Mapping\OneToMany.php to match what is shown in documentation: