Is trait accessing class dependency a bad idea?

Question

I have seen an example on Stackexchange (please note the trait accessing the class property):

trait CheckPermissionTrait
{
    protected function checkPermission($object_id)
    {
        $judge = $this->container->get('acme_judge');

        $user  = $this->container->get('security.context')->getToken()->getUser();

        if( !$judge->isPermitted($user, $object_id) ) {
            throw $this->createAccessDeniedException("Brabbel");
        }
    }
}

And read one of the repliers comments:

Your trait, then is not a valid use-case: all of its users are required, by definition, to add a $this->container property to its dependencies, which will of course have an impact on that class' contract and the contract of its children.

Why is the author claiming that being a bad use case, if that might be what someone needs? Like if someone has few classes that all have the required dependency and the same logic reccures in all of them, should they just keep the code duplicated?

Goethe

@vascowhite This question is off-topic for CodeReview as it is asking for explanation of someone else's code, please don't suggest it. Please refer to meta.codereview.stackexchange.com/questions/5777/…. It is perfectly fine on Stack Overflow. – Polychaete

@Polychaete this is asking for a review of code it should not matter who wrote it, as long as in the end the OP understand it and knows how to make it better. This is off topic for Stack Overflow. – Feuilleton

@Neal You're wrong, it does matter who wrote it. There is in fact a close reason for just that case. Refer to meta.codereview.stackexchange.com/questions/3649/…. Also, it might be opinion-based for Stack Overflow (I don't have the knowledge to tell) but it is not off-topic. – Polychaete

Answer 1

Indeed trait used in this way - a bad idea. If someone decide to use this trait in your code - he must ensure the existence of "container" attribute. "container" should be the correct type (include the methods used) - otherwise it will get an error. In fact this code can not be reused, and this potentially bug. In addition, it violates a rule DIP (dependency inversion principle) of the rules of SOLID.

It is possible to get around this:

interface ExampleContainerInterface{

}
trait CheckPermissionTrait
{
    protected $container;
    public function __construct(ExampleContainerInterface $container)
    {
        $this->container = $container;
    }

    protected function checkPermission($object_id)
    {
        $judge = $this->container->get('acme_judge');
        $user  = $this->container->get('security.context')->getToken()->getUser();
        if( !$judge->isPermitted($user, $object_id) ) {
            throw $this->createAccessDeniedException("Brabbel");
        }
    }
}

class ExampleClassA
{
    use CheckPermissionTrait;
}
class ExampleClassB
{
    use CheckPermissionTrait;
}

or like this (php7):

interface ExampleContainerInterface{

}
trait CheckPermissionTrait
{
    abstract public function getContainer():ExampleContainerInterface;
    protected function checkPermission($object_id)
    {
        $container = $this->getContainer();
        $judge = $container->get('acme_judge');
        $user  = $container->get('security.context')->getToken()->getUser();
        if( !$judge->isPermitted($user, $object_id) ) {
            throw $this->createAccessDeniedException("Brabbel");
        }
    }
}

class ExampleClassA
{
    use CheckPermissionTrait;
    protected $container;
    public function getContainer():ExampleContainerInterface
    {
        return $this->container;
    }
}
class ExampleClassB
{
    use CheckPermissionTrait;

    protected $container;
    public function getContainer():ExampleContainerInterface
    {
        return $this->container;
    }
}

Lemur

Answer 2

I agree with that, I don't think traits should use any methods or attributes of the class, but when I read the laravel framework source code, I see many abuse of this kind of traits, for example, the InteractWithInput trait is deeply coupled with the request class, it's very confused

trait InteractsWithInput
{
    //the getInputSource method is defined in Request class
    public function input($key = null, $default = null)
    {
        return data_get(
            $this->getInputSource()->all() + $this->query->all(), $key, $default
        );
    }
}

class Request
{
    use InteractsWithInput
    protected function getInputSource()
    {
        if ($this->isJson()) {
            return $this->json();
        }

        return in_array($this->getRealMethod(), ['GET', 'HEAD']) ? $this->query : $this->request;
    }
}

Cloud

Answer 3

Or you can use method injection and not violate anything:

trait CheckPermissionTrait
{
    protected function checkPermission($object_id, $container)
    {
        $judge = $container->get('acme_judge');

        $user  = $container->get('security.context')->getToken()->getUser();

        if( !$judge->isPermitted($user, $object_id) ) {
            throw $this->createAccessDeniedException("Brabbel");
        }
    }
}

Obviously, when someone wants to use this trait, he must provide the container as an argument in the method call. Everything is save now.

Mog