Symfony : is it a bad practice to use a service to return entity?

Question

In my project, I have :

• Company Entity : created with Doctrine
• CompanyController : list actions that can be realized for the company
• CompanyManager Service : used to process some Company Entity data.

For example, Company Entity has a getOpeningDays() function, and CompanyManager Service has a getRealOpeningDays() function that filter out holidays :

<?php
namespace AppBundle\Services\Company;

class CompanyManager
{

    private $em;
    private $companyBase;

    public function __construct(
        \Doctrine\ORM\EntityManager $em, 
        \AppBundle\Services\Work\PublicHolidays $publicHolidays
    ) { 
        $this->em               = $em;
        $this->publicHolidays   = $publicHolidays;
    }

    public function setCompanyId($companyId)
    {
        $this->companyBase = $this->em->getRepository('AppBundle:CompanyBase')->findOneBy(
            array('id' => $companyId)
        );
    }

    public function getRealOpeningDays()
    {
        // ...          
        foreach($publicHolidays as $publicHolidayKey => $publicHolidayData)
        {
            if(in_array($publicHolidayKey, $this->companyBase->getWorkedPublicHolidaysIds()) == false)
            {
                // ...
            }
        }
        return $publicHolidaysNonWorkedDates;
    }
}

In my CompanyController, I have sometimes form that require to use Company Entity.

I am now wondering if it is a bad practice to return this Company Entity from CompanyManager Service, in a similar function :

<?php
namespace AppBundle\Services\Company;

class CompanyManager
{

    //...

    public function getEntity()
    {
        return $this->companyBase;
    }
}

It would allow me to change this :

public function companySocialAction(Request $request)
{
    $session = $request->getSession();
    $companyBase = $this->getDoctrine()->getRepository('AppBundle:CompanyBase')->findOneBy(array('id' => $session->get('companyId')));

    $companyManager = $this->get('CompanyManager');
    $companyManager->setId($companyBase->getId());

    $form = $this->createForm(CompanySocialType::class, $companyBase);
    $form->handleRequest($request);


    // ...
}

In this :

public function companySocialAction(Request $request)
{
    $session = $request->getSession();

    $companyManager = $this->get('CompanyManager');
    $companyManager->setId($session->get('companyId'));

    $form = $this->createForm(CompanySocialType::class, $companyManager->getEntity());
    $form->handleRequest($request);

    // ...
}

Thank you.

Answer 1

This is more of a code review question and thus highly opinion based. But I hope, this is helpful. However, I would split it into to parts:

CompanyManager

Is your CompanyManager a manager in the sense of an EntityManager like the one you're injecting (\Doctrine\ORM\EntityManager)? So all methods like find|findAll|persist|flush etc. are wrapped? Or is it more like a Factory or in terms of Symfony a Provider? If so, you could name it to CompanyProvider and it should only do what the name suggests.

Symfony already uses a provider for the User entity. The scheme can be adopted for other entities as well: Create a User Provider:

class CompanyProvider {
    public function loadCompanyById($id) {
        // […]

        if ($company) {
            return $company;
        }

        throw new CompanyNotFoundException(
            sprintf('Company with id "%s" does not exist.', $id)
        );
    }

    public function refreshCompany(\Company $company)
    {
        // […]
        return $this->loadCompanyById($company->getId());
    }
}

getRealOpeningDays()

I don't think that this method should be part of the class that provides Company entities. It looks more like a utility or service, which could be available for other entities or values as well. Also this function is ambiguous - at least by looking at the simplified example. It is called getRealOpeningDays but it returns something called publicHolidaysNonWorkedDates. If I got you right, you could create a filter like this:

namespace AppBundle\Services;

class HolidayService {

    private $publicHolidays;

    public function __construct(\AppBundle\Services\Work\PublicHolidays $publicHolidays) {
        $this-> publicHolidays = $publicHolidays;
    }

    public function getPublicHolidays() {
        return $this->publicHolidays;
    }

    public function filterByPublicHolidays($datesOpen, $datesOpenOnHolidays = null) {
        // reduce $dateOpen by $this->publicHolidays
        // if $datesOpenOnHolidays is given, don't remove holidays that the company or any other entity is actually open
        return $collection;
    }
}

Minor adjustments

• Don't call a method setCompanyId when it never actually sets the company id, but loads a whole object instead.
• In the first version of companySocialAction you load the entity twice, which is unnecessary. You could simply pass the object around. ^{(I assume that setId means setCompanyId.)}