18

Is it a code smell if the methods in my trait refer to parent::methods or to methods that are assumed to be in the utilising class?

A random (senseless) example

trait foo
{
    public function bar()
    {
        return parent::bar() . 'baz';
    }

    public function format($text)
    {
        $format = true;
        return $this->parse($text, $format);
    }
}

Are those methods to be moved in the class implementation?

Should I have exclusively methods without dependencies, inside a trait?

2 Answers 2

23

It's fine if a trait depends on methods in the class into which it is embedded. But these dependencies should be explicit, by declaring an abstract function. PHP can then check that such a method actually exists.

But traits should not override methods. That is rather confusing behaviour. Instead, give a different name to the methods in the trait. In this particular example, you are relying on a method in the base class of the class into which your trait is embedded. That is a very obscure dependency. Compare the precedence examples in the docs.

Why not use abstract classes instead? Because you can have multiple traits, but only one base class. Therefore, traits are more general, and are useful for a bundle of convenience functions that you want to use in multiple classes. Your format method is a great example of this. Written more clearly, it would be:

trait Formatter
{
    abstract public function parse($text, $format);

    public function format($text)
    {
        $format = true;
        return $this->parse($text, $format);
    }
}
4
  • @amom interesting approach, I've never seen that before, but it looks like it's valid in PHP. I can see how this could be valuable. That said the question was in reference to calling parent methods from a trait. I think that's where the code smell exists in the scenario posted in the question.
    – zquintana
    Commented May 29, 2018 at 15:25
  • @zquintana If I am not wrong amon's approach would ensure that the code will fail before running, in case the parent class didn't already define that abstract method. In a way it is a good (automatic) alternative to using reflection and checking in-code that the method exists (or throw an exception otherwise). It doesn't remove the potential smell of the intention, but at least this latter is explicit and allows to still use traits consistently in such case (especially when some already present structure/organization/hierarchy is already present and not changeable) 🙂 Commented Aug 20, 2018 at 17:22
  • @Kamafeather what your describing is true, but I'd say interfaces are a better fit for that. They would be the best approach and you see other well established projects using that approach. An interface that describes some behavior and a trait that can be used to implement it in a portable fashion. IMO I think that's better then having a trait with an abstract method and using that abstract method as validation. IMO it's better when the calling code says "if interface implemented then" use that interface methods. It ensures at runtime that it's used properly without reflection.
    – zquintana
    Commented Aug 21, 2018 at 18:58
  • Yes your points make sense. I personally see the DeclareInInterface and AbstractInTrait as providing almost the same good (parse-time failing; interface/abstract fingerprint available, contrary to the reflection-way); but you are right underlining that interfaces is where one would usually go checking for methods to implement; I wouldn't go in a trait for that (and could also feel weird). I think the AbstractInTrait is a quick way to achieve the same result, when one doesn't want to think-about/maintain interfaces in the project. Btw I am trying with interfaces atm. Commented Aug 29, 2018 at 16:08
8

Yes, it's a code smell. If a trait is used to make code more portable, then then this sort of thing makes it less portable. You shouldn't do it. Without having a more concrete example it looks like what you should be doing is putting that bar method in an abstract and extending that. That approach avoids the potential runtime bug where the trait is added to a class that doesn't have bar.

3
  • 1
    Every bug is a "runtime bug" in PHP :( Commented Nov 26, 2019 at 23:34
  • 2
    This is true, but my point here is that with an IDE or while running unit tests this bug would not immediately become apparent because it would look like the code is valid from the interpreter. However calling the method could lead to an error. The only way to catch it in that case would be to add a unit test that tests the same method on every implementing class. I would say that's pretty bad design because of that.
    – zquintana
    Commented Nov 27, 2019 at 16:54
  • I agree, thank you for your comment. Commented Nov 28, 2019 at 3:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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