Improve controller generation in make:crud command

[garak]

Description

Currently, the make:crud maker command generates actions that rely on explicit calls to Doctrine's manager.
I know that is already possible for user to change that code and improve it, but my point is about giving good suggestions (and I think current implementation fails on that).
Example of a generated code:

// in "new" action
$entityManager = $this->getDoctrine()->getManager();
$entityManager->persist($tinyChef);
$entityManager->flush();
// in "edit" action
$this->getDoctrine()->getManager()->flush();
// in "delete" action
$entityManager = $this->getDoctrine()->getManager();
$entityManager->remove($tinyChef);
$entityManager->flush();

This is breaking the law of Demeter, making controllers know too much about Doctrine internals.

My proposal is to add two methods to generated repository class: add and remove. The first method would be responsible to add/edit entity and the second one to delete it. With such methods (of course, by injecting repository in "new"/"edit"/"delete" actions), previous lines can be replaced with:

// in "new" action
$tinyChefRepository->add($tinyChef);
// in "edit" action
$tinyChefRepository->add($tinyChef);
// in "delete" action
$tinyChefRepository->remove($tinyChef);

This is an example of generated code for add method in repository class:

public function add(TinyChef $tinyChef, bool $flush = true): void
{
    $this->getEntityManager()->persist($tinyChef);
    if ($flush) {    // I think it's good to keep the opportunity to avoid flushing
        $this->getEntityManager()->flush();
    }
}

The remove method would be pretty the same (with remove instead of persist).

With such change, controllers wouldn't even know that they're handling Doctrine (except for namespaces), they would just rely on repositories.
I guess that, in a future, this could be also leading to deprecation/removal of getDoctrine() helper in the ControllerTrait.

[xabbuh]

@garak FYI, I have moved the issue to the MakerBundle repository.

[garak]

@garak FYI, I have moved the issue to the MakerBundle repository.

Ooops, sorry about that, I was convinced that this bundle was in the symfony monorepo

[jrushlow]

Howdy @garak. I actually love the idea - I do this currently in my projects anytime I need to leverage the repository itself. @weaverryan and I were talking about getting away from leaning on calling the entity manager directly in cases where we don't need to explicitly for other parts of maker:bundle.

If we were to implement something like this, the methods would be added to the templates in make:entity. Then in the other makers, we would have to:

1 - check if the entity repository exists
2 - check if the repository has the add() / remove()
3 - use those methods if 1 & 2 are true else fall back to the entity manager solution that we use now.

Perhaps implement that logic globally for all makers and provide a global $repository_var for the templates. or something along those lines...

Would love to hear feed back from others on this issue - and any help implementing it into MakerBundle would be very much appreciated!

Thanks @garak for bringing this up.

[bdaler]

If we were to implement something like this, the methods would be added to the templates in make:entity.

@jrushlow in a make:entity or in make:crud command?

I've already been doing this method manually. It also helps with DI when all you have to do is inject the repo vs the entity manager.

[mcwnuq]

@jrushlow Why are these methods generated in make:entity? What are they needed there for?
As @bdaler mentioned, it shouldn't be in make:crud?

[ThomasLandauer]

I suggested to show this as the recommended way in the docs: symfony/symfony-docs#17429