54
\$\begingroup\$

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

<?php
namespace Acme\DemoBundle\Model;

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");
        }
    }
}

-

<?php
namespace Acme\DemoBundle\Controller;

use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;
use Acme\DemoBundle\Model\CheckPermissionTrait;

class SomeController extends Controller
{
    use CheckPermissionTrait;

    public function someAction(Request $request, $object_id)
    {
        $this->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?

\$\endgroup\$
  • 2
    \$\begingroup\$ I look forward to seeing answers, as I know very little about traits. You can moot the question by refactoring to checkPermission($object_id, $judge, $user) and not requiring the trait function to know about $this->container -- but I'm curious to learn about the best practice here. \$\endgroup\$ – J.T. Grimes Dec 18 '14 at 22:14
  • \$\begingroup\$ Yes you might want to post that as an answer. Another option is I could make an abstract controller extending the symfony framework controller and then extend that, which is probably better in this case. \$\endgroup\$ – Marcel Burkhard Dec 19 '14 at 6:09
  • \$\begingroup\$ @MarcelBurkhard: Thanks for accepting my rant, but while you were doing that, I was busy updating my answer, adding some (what I feel) more details and possible issues that an abuse/over-use of traits could cause, so it might be worth a look \$\endgroup\$ – Elias Van Ootegem Dec 23 '14 at 11:41
  • 3
    \$\begingroup\$ I see you've pointed this out in your own answer to your question, but I wanted to get this in up here on the original post. In the code provided, the DI Container is being used as a Service Locator which is an anti-pattern because it effectively hides the true dependencies of a class. One should avoid calls like $container->get('alias') in favor of injecting dependencies into the class e.g. via the constructor. \$\endgroup\$ – John Hall Mar 17 '16 at 17:56
51
\$\begingroup\$

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(
                sprintf(
                    '%s must implement IteratorAggregate to use %s',
                     get_class($this),
                     __TRAIT__
                )
            );
        }
        return new ArrayIterator($this);
    }
}

Valid use-case

For reasons I've explained above, it's clear that your example isn't something best handled with traits. It might be useful at this point to serve up a small, 100% valid example of a trait. Suppose you've got some models that represent either a DB record, user input (form data), or something else entirely. All of these models contain user information, including an email address. A model with setters is ideal to ensure that, whenever some data is gathered, that data is then validated (ie filter_var($email, FILTER_VALIDATE_EMAIL)). Because these classes represent different things (db record or form), they're likely to inherit from an abstract class already. Rather than duplicating the email setter, a trait can be used:

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;
    }
}

Wrong namespace

Just a little nit-pick: I don't like the fact that your trait is defined in the Model namespace, while it's pretty clear that it can't be used anywhere else but in the controller.


Update

Time for an update. While I stand (firmly) by what I've stated previously (traits not being in a position to impose a contract), there are certainly people who will argue that they can. Not in the least because PHP's trait construct enables you to declare abstract methods:

trait Evil
{
    protected $dependency = null;
    public function someMethod()
    {
        return $this->dependency->getValue();
    }
    abstract public function setDependency(SomeType $dep);
}

This trait, then, imposes a partial contract on its user: the class that uses the trait must define the setDependency method. So what's the problem, you might say? The problem is simple: traits were introduced to solve supposed problems that might require multiple inheritance, hence a class can use more than one trait, and it's possible for the class to resolve conflicts regarding method names by itself. So let's assume the trait above is being used, and you have a couple more traits lying around, one of which looks like this:

trait Pure
{
    public $dependency = null;
    public function setDependency(Container $container)
    {
        $this->dependency = $container;
        return $this;
    }
}

Now, imagine you have a class that uses both these traits (assume Evil::setDependency is not declared abstract here):

class FromHell
{
    use Pure, Evil {
        Pure::setDependency insteadof Evil;
    }
}

Congrats, the contract imposed by the trait is broken, along with its functionality, simply because the class has chosen to adhere to the contract imposed by Pure. It doesn't take a rocket scientist to realize that traits, whilst they can, in theory, impose a minimal contract, are not as potent at ensuring the contract is adhered to.

Now this was a simple example, but if you want a more plausible scenario: Assume 2 traits, A and B, both define a method logError, A::logError writes to a default log file, B is more generic, and expects you to pass a resource as an argument, or writes to stderr by default.

Because your project is rather large, you're dealing with multiple levels of inheritance and at some point, some classes end up implementing both traits. Of course, depending on what class it is, either A::logError is given a higher precedence, or B:logError. The latter is more likely to be used in workers/crons/backend code.

It's very likely that your project, seeing as you're relying on traits so much, contains code that looks like this:

$traits = class_uses($object);
if (isset($traits['A']))
{
    $object->logError('The error string');
}

Now if this object has chosen to override A::logError with B::logError, you'll probably waste a lot of time trying to work out why on earth logError doesn't write your error messages to the default log. In short:

Trait contracts are weak suggestions, and can be overridden easily

Back to the definition of abstract methods in a trait: I'll have to test it a bit more, but AFAIK, that's just bad design. Either an abstract class contains your abstract methods (because a class enforces a strong contract), or you implement the method in the class from the get-go. A trait is supposed to reduce code duplication, whereas an abstract method requires you to re-write the method's signature and implementation whenever you use the trait. If that's what you want, why bother using a trait in the first place?

The other issue I have with this is one I'll look into a bit more when I find the time, but classes can be composed out of 1 or more traits, and contain no other definitions of their own. That's fine, but what if we look at our 2 traits above (Pure and Evil). Is Pure::setDependency a valid implementation of the abstract Evil::setDependency method? If so, will that trait method be regarded as an implementation of the abstract method? If not, traits containing abstracts can't be used in classes that contain no definitions of their own, and therefore, such traits are special cases (because their usage is limited).

If Pure::setDependency is a valid implementation of the abstract method in Evil (Container being an alias of SomeType, or its parent class), and traits can be used to implement abstract trait methods, then another issue presents itself: maintenance HELL: Changing either one of these traits could break your code, depending on how, and where the traits are being used. You'll end up having to test all classes using both traits, whenever you change just one trait. When you look at it like that, I'd say some code duplication is the lesser of two evils.

\$\endgroup\$
  • \$\begingroup\$ I'd just like to point out that whilst normally your subclass should (must in my book!) respect the rules laid down in any methods you override, constructors are an exception. This is because a subclass might have completely different dependencies from the superclass even if it has the same functionality. PHP won't complain about constructor signatures not matching and the programmer should know what the requirements are when they instantiate a new class();. The only place where you really do need to enforce constructors is if the class will be built by a factory \$\endgroup\$ – GordonM Feb 28 '18 at 15:40
  • \$\begingroup\$ This is going into my team's design guide. We cornered ourselves into very dependent traits and when we move to a new framework, traits will be highly restricted. This explanation will help me enforce that. Thank you! \$\endgroup\$ – cautionbug Jul 11 at 12:37
10
\$\begingroup\$

After doing some "research" I come to the conclusion, that you shouldn't use Traits the way I did. My main reasons for this conclusion are bad testability and readability. (Please read the first source below)

My code above (in the question) violates some design principles and I will refactor my controller to extend an abstractcontroller which implements the checkPermission function I need in all controllers extending it.

Sources:

Edit: (2015-07-28)

Months have passed and I come back to this answer and I want to mention a major flaw in the code. The controller above relies on the whole service container being injected, which is called the Service locator (anti-)pattern.

Dependencies are retrieved by calling $this->container->get('name_of_some_service') instead of injecting them directly. This violates the law of demeter and results in bad testability and reusability.

Dependency injection is the way to go there and it is also supported by the symfony framework.

The following link will get you started: How to Define Controllers as Services

By using dependency injection you get lower coupling to the framework, dependencies of a given class are in most cases easily visible by looking at the constructor.

\$\endgroup\$
  • \$\begingroup\$ Just for completeness: While it's true that the service locator can affect testability, testing frameworks like codeception do have modules that allow you to replace the container with a mockable object, effectively solving the problems facing while testing \$\endgroup\$ – Elias Van Ootegem Jul 29 '15 at 8:56
0
\$\begingroup\$

It's possible to rebind $this in the scope of a single Closure, so you could access the container of any passed in object

function isJudgePermitted($object, int $object_id) : bool {
  return (function (int $object_id) : bool {
    $judge = $this->container->get('acme_judge');
    $user = $this->container->get('security.context')->getToken()->getUser();
    return $judge->isPermitted($user, $object_id);
  })->call($object, $object_id);
}

// Inside SomeController.

public function someAction(Request $request, $object_id) {
  $judge_permitted = isJudgePermitted($this, $object_id);
  $judge_permitted ?: throw $this->createAccessDeniedException("Brabbel");
  //do other stuff
}   
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.