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.
