HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpCritical

Trait accessing variables of classes using it

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
traitusingclassesvariablesaccessing

Problem

I just tried PHP Traits for the first time, to keep my code DRY :

container->get('acme_judge');

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

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


-

checkPermission($object_id);

        //do other stuff
    }        
}


What making me a little nervous, is that the trait is accessing the container of the class using it, which means there is actually the requirement that the class using the trait needs to extend the Controller (Symfony\Bundle\FrameworkBundle\Controller\Controller) for the trait to work.

Is this normal when using traits or is it bad practice?

Solution

Though you now have seen the light, and know not to use traits in this way, I'm going ahead and post my review here anyway. I'll be updating it along the way, seeing as this is something I can rant on about for some time.

Your trait suffers from a couple of (severe) issues:

  • It constitutes both a breach of contract, and enforces a bad contract.



  • Traits requiring properties are like using global in a class



  • It's not a valid use-case for a trait. At all.



  • Its namespace is wrong



Now these are just the first things I could think of, each of which are problematic enough to ditch the trait as you rightfully did.

Allow me to explain, though, what these one-liners mean:
Breach of Contract + enforcing a bad one:

Classes define a contract. That's a given. If you pass an instance of class X to a function, you make a deal with that function. You're essentially saying "This object has this specific job, and you can call these methods on it, and/or access these properties". When you extend a class, it's important to respect the contract. You can add things, but never change the promises made by the base class.

If the base class' constructor, for example, expects 1 argument of the type array (enforced through type-hinting), then all of its children should either use the same constructor or, if they choose to override it, define a constructor that takes 1 array argument. They are allowed to loose the type-hint, but may not change it. They're free to add optional arguments, but can't force additional arguments to be passed to the constructor:

abstract class Base
{
    public function __construct(array $param)
    {}
}
class ValidChild extends Base
{}
class AnotherChild extends Base
{
    public function __construct(array $param, $opt = null)
    {}
}
class IffyButTheoreticallyFine extends Base
{
    public function __construct($param, array $opt = null)
    {}
}


These are all valid child classes, however: these are not. They are evil, but sadly commonplace:

class Bastard extends Base
{
    public function __construct()
    {
    }
}
class Ungrateful extends Base
{
    public function __construct(array $param = null)
    {}
}
class EvilTwin extends Base
{
    public function __construct(stdClass $param)
    {
        parent::__construct( (array) $param);
    }
}
class Psychopath extends Base
{
    public function __construct(PDO $db, array $parent = null)
    {
        if (!$parent) {
            $parent = [1, 2, 3];
        }
        parent::__construct($parent);
    }
}


Now, how does this apply to your case? Simple: A trait is a tool to reduce code-duplication. It's a nugget of often-needed functionality that a class (a contract) includes/uses to do its job. For a class to use a trait, the trait has no right to define that class' contract. A trait's relation to its user is, in some way, the same as that of a child's towards its parent: it's the child that inherits the contract, not the parent. Therefore, it must follow that any class, regardless of its job, has to be able to use a trait, without having to change its own contract and dependencies.

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.
A new global in drag

I think it's safe to say that it's universally accepted that a class definition containing global $someVar is a tell-tale sign of bad code. A class is scoped, its methods are scoped and neither of them should rely on the global scope to function. The same applies to a trait: because it can be used in a variety of classes, a trait must be context (and scope) agnostic. If it requires a property to be present, the trait must define it. If that property has to be of a particular type, the only thing you could do is define a property, and enforce an abstract setter for it, which imposes type restrictions. However, this is unreliable, given that multiple traits can be used, it might cause conflicts with the user and the class itself has direct access to the property, too. Lastly: this approach brings us back to square one: a trait like this is no longer a trait.

This implies that a trait cannot have dependencies. If a trait has a dependency, it ceases to be a trait: it's either an abstract class, or an Interface, but definitely not a trait. There is, however a tiny sliver of grey-area here:

Whilst it's not my idea of best practice, one might argue that it's acceptable to write a trait, to be used in tandem with an Interface, for example: a single Interface that ensures data models are traversable, like the IteratorAggregate interface. In that case, a trait like this could perhaps be acceptable.

```
trait IteratorAggregateTrait
{
public function getIterator()
{
if (!$this instanceof IteratorAggregate) {
throw new \LogicException(
s

Code Snippets

abstract class Base
{
    public function __construct(array $param)
    {}
}
class ValidChild extends Base
{}
class AnotherChild extends Base
{
    public function __construct(array $param, $opt = null)
    {}
}
class IffyButTheoreticallyFine extends Base
{
    public function __construct($param, array $opt = null)
    {}
}
class Bastard extends Base
{
    public function __construct()
    {
    }
}
class Ungrateful extends Base
{
    public function __construct(array $param = null)
    {}
}
class EvilTwin extends Base
{
    public function __construct(stdClass $param)
    {
        parent::__construct( (array) $param);
    }
}
class Psychopath extends Base
{
    public function __construct(PDO $db, array $parent = null)
    {
        if (!$parent) {
            $parent = [1, 2, 3];
        }
        parent::__construct($parent);
    }
}
trait IteratorAggregateTrait
{
    public function getIterator()
    {
        if (!$this instanceof IteratorAggregate) {
            throw new \LogicException(
                sprintf(
                    '%s must implement IteratorAggregate to use %s',
                     get_class($this),
                     __TRAIT__
                )
            );
        }
        return new ArrayIterator($this);
    }
}
trait UserDataTrait
{
    public $email = null;
    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            throw new \InvalidArgumentException('bad email');
        }
        $this->email = $email;
        return $this;
    }
}
trait Evil
{
    protected $dependency = null;
    public function someMethod()
    {
        return $this->dependency->getValue();
    }
    abstract public function setDependency(SomeType $dep);
}

Context

StackExchange Code Review Q#74077, answer score: 60

Revisions (0)

No revisions yet.