Fix annotations code style issues for entities #408
Conversation
| @@ -95,7 +95,7 @@ class Post | |||
| * mappedBy="post", | |||
| * orphanRemoval=true | |||
| * ) | |||
| * @ORM\OrderBy({"publishedAt" = "DESC"}) | |||
| * @ORM\OrderBy({"publishedAt": "DESC"}) | |||
There was a problem hiding this comment.
Are we sure about this change? In http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/tutorials/ordered-associations.html they use "publishedAt" = "DESC"
There was a problem hiding this comment.
Good spot! I think : is more native since it looks like a JSON object here, so I like : more. But if both works, I'd rather send a PR to Doctrine docs to use : instead of = and see what they'll say :)
There was a problem hiding this comment.
recent versions of doctrine/annotations allow both = and : as delimiter between the array key and value. Old versions were allowing only =, which is why the ORM doc uses it.
There was a problem hiding this comment.
@javiereguiluz, we already use the : in routes: https://github.com/symfony/symfony-demo/blob/master/src/AppBundle/Controller/BlogController.php#L38
There was a problem hiding this comment.
The new syntax could confuse Newbies while they are writing their first Symfony app. The annotations from the framework bundle are still using = and the devs might wonder, why the annotations in this case look complete different. They might not recognize the difference of the two separate projects.
There was a problem hiding this comment.
I'm sorry. I wasn't aware of this Doctrine feature and I thought this was a bug. Let's keep it then.
There was a problem hiding this comment.
@frastel FrameworkBundle does not have annotations. It uses doctrine/annotations.
The difference you see corresponds to 2 different features of the annotation syntax.
Putting a value in a property of the annotation and writing an associative array in a value are already 2 different things.
what could confuse you is that the property name is omitted here (thanks to a feature allowing to do it in some cases). The complete syntax is @ORM\OrderBy(value={"publishedAt": "DESC"})
There was a problem hiding this comment.
@stof You are totally right! I think I didn't finished my thoughts :D. After thinking 3 times about the fact I think : is much cleaner.
I never thought about that the shortcut syntax misses value= actually.
And yeah, I meant the FrameworkExtraBundle, not the FrameworkBundle.
|
Thanks @voronkovich. |
No description provided.