-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
doctrine.rst
Outdated
// you can also receive the $em as an argument | ||
public function editAction(EntityManagerInterface $em) | ||
// you can use the registry 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 new debug:container --types
option will help this out - since that valid type-hints
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 the Product::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 can also receive the $em as an argument | ||
public function editAction(EntityManagerInterface $em) | ||
// you can use the registry 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
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 :)
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
@@ -569,26 +571,18 @@ a controller, this is pretty easy. Add the following method to the | |||
return new Response('Saved new product with id '.$product->getId()); | |||
} | |||
|
|||
// you can also receive the $em as an argument | |||
public function editAction(EntityManagerInterface $em) | |||
// you can use the registry 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 them
@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 get symfony/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 use getManagerForClass()
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!