Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Example of the Doctrine doc with types as the first-class citizen#7876
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
doctrine.rst Outdated
| // you canalso receivethe$em as an argument | ||
| public function editAction(EntityManagerInterface $em) | ||
| // you canusetheregistry to fetch other em's, if you have mutiple | ||
| public function editAction(ManagerRegistry $registry) |
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.
A question newcomers (and not only them) may ask: why sometimes I need to inject an interface (EntityManagerInterface) and sometimes just a class (ManagerRegistry).
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.
Bah, that's Doctrine's fault.ManagerRegistry IS an interface... I think they've only more recently started to use the Interface suffix.
Btw, I'm hoping the newdebug:container --types option will help this out - since that valid type-hints
yceruto left a comment
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.
Great! 👍 Just minor (optional) dx comments :)
| { | ||
| $product = $this->getDoctrine() | ||
| ->getRepository('AppBundle:Product') | ||
| $product = $em->getRepository('AppBundle:Product') |
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.
[DX] What about to promote theProduct::class usage?
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.
Yes, we should start doing this :). But it's gotta be in a different PR, because it needs to be applied to 3.1 and higher
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.
Opened#7892 to keep track of this :)
| $product = $this->getDoctrine() | ||
| ->getRepository('AppBundle:Product') | ||
| $product = $em->getRepository('AppBundle:Product') | ||
| ->find($productId); |
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.
[DX]$em->find(Product::class, $productId)? at the end it does the same, so less coding :)
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.
Ah, it's cool ... but then you haven't taught the user about the repository. So, I like the slightly longer, but "simpler" approach - tell them that you need the repo 100% of the time :)
doctrine.rst Outdated
| // you canalso receivethe$em as an argument | ||
| public function editAction(EntityManagerInterface $em) | ||
| // you canusetheregistry to fetch other em's, if you have mutiple | ||
| public function editAction(ManagerRegistry $registry) |
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.
$doctrine?
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.
+1
GuilhemN left a comment
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.
awesome 👍
| // ... | ||
| public function createAction() | ||
| public function createAction(EntityManagerInterface $em) |
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.
missing use statement :)
GuilhemN commentedMay 6, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm wondering, should we assert everywhere that folders registered in the standard edition do not need to be registered again in the docs? |
doctrine.rst Outdated
| // you canalso receivethe$em as an argument | ||
| public function editAction(EntityManagerInterface $em) | ||
| // you canusetheregistry to fetch other em's, if you have mutiple |
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.
you can use the registry to fetch other em's, if you have mutiple-->
if you have multiple entity managers, use the registry to fetch themweaverryan commentedMay 10, 2017
@GuilhemN Let's have that discussion on a different PR - like#7883, or another that I'll hopefully open today (this PR just doesn't have any good examples of that as far as I can see). But I'm totally wondering about that. Unless... we can getsymfony/symfony-standard#1070 merged... then it won't matter (and this is one of the reasons I want that feature!) |
| { | ||
| // ... | ||
| $em = $doctrine->getManager(); | ||
| $em2 = $doctrine->getManager('other_connection') |
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.
I am not sure if it is common to get the manager for a particular connection. Don't you rather want to get the entity manager that manages a particular entity (i.e. you will want to usegetManagerForClass() instead)?
This updates the doctrine chapter in the new "types-first" philosophy - i.e. using type-hinting and autowiring to fetch services (rather than service ids). Of course, we must still show both (the type hint and the service id), but probably only the first time in a chapter. Afterwards, we should show the type.
This is the first "normal" chapter to get this update. Thoughts?
Thanks!