17

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?

Kamafeather
  • 335
  • 1
  • 2
  • 7

2 Answers2

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);
    }
}
amon
  • 132,749
  • 27
  • 279
  • 375
  • @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 May 29 '18 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) – Kamafeather Aug 20 '18 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 Aug 21 '18 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. – Kamafeather Aug 29 '18 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.

zquintana
  • 254
  • 1
  • 10
  • 1
    Every bug is a "runtime bug" in PHP :( – Yevgeniy Afanasyev Nov 26 '19 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 Nov 27 '19 at 16:54
  • I agree, thank you for your comment. – Yevgeniy Afanasyev Nov 28 '19 at 03:01