Magento-lts: Discussion: @inheritDoc vs @return $this

[colinmollenhour]

Continuation of discussion from here: https://github.com/OpenMage/magento-lts/pull/783#discussion_r308656422

[sreichel]

There is not much documentation about this .... from phpDocumentor ...

• https://docs.phpdoc.org/latest/guides/inheritance.html
• https://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.inlineinheritdoc.pkg.html

---

• {@inheritDoc} ... just "copies" description (and @param? No?) from parents description, w/o return type
• @inheritDoc ... copies complete parents DOCS, including return type. This should be used everywhere it is return parrent::xyz();
• @return $this should be used for method that return $this, but @inheritDoc should work too. if parrent method also returns $this.

[colinmollenhour]

Can we first agree that the behavior of PhpStorm should be the determining factor?

[tmotyl]

Can we first agree that the behavior of PhpStorm should be the determining factor?

:+1: from me, I would just add that it has to be understood by phpstan too.

[kiatng]

We use VS Code + PHP exntensions, one of which

Reads PHPStorm metadata for improved type analysis and suggestions.

That covers it.

[sreichel]

Can we first agree that the behavior of PhpStorm should be the determining factor?

Of course. It was just for info, I dont use phpDoc, but it seems to be same as phpstorm works.

from me, I would just add that it has to be understood by phpstan too.

Yep. IDE improvements are nice to work with, but at the end I would like to have code/docs that passes static tests (phpstan, pslam, or whatever)

[colinmollenhour]

I think the case is fairly strong that where the class method is likely to be overridden and a method is likely to be used in chaining and returns the parent class method return value of the same name which happens to use $this as the return value, we want to use an explicit @return $this on the child method and not @inheritDoc due to the behavior of PhpStorm.

I've proven that this is the only (or at least best to my knowledge) way for PhpStorm to correctly infer types and @tmotyl has confirmed that PhpStan is ok with this syntax so it seems to be the best solution to be compatible with both.

<?php
class A
{
    /**
     * @return $this
     */
    public function doesChain()
    {
        return $this;
    }

    /**
     * @return $this
     */
    public function doesNotChain()
    {
        return $this;
    }
}

class B extends A
{
    /**
     * @return $this
     */
    public function doesChain()
    {
        return parent::doesChain();
    }

    /**
     * @inheritDoc
     */
    public function doesNotChain()
    {
        return parent::doesNotChain();
    }

    public function foo()
    {
        return TRUE;
    }
}

$b = new B;
$b->doesChain()->foo(); // recognized by PhpStorm
$b->doesNotChain()->foo(); // NOT recognized by PhpStorm

A good example would be \Mage_Eav_Model_Entity_Collection_Abstract::addItem which should use @return $this so that if it is overridden the type inference on chained methods still works.

https://github.com/OpenMage/magento-lts/pull/771/files#diff-957cf8fbbb2ba5e503830ce812d79f99R259

[Flyingmana]

Just a comment: I prefer following the standards, not a single implementation.
If PHPStorm does not follow a standard in a certain point we can still do a bug report about this.

[kiatng]

I tried Colin's code in VS Code, it chain for both methods: doesChain() and doesNotChain().

[sreichel]

@colinmollenhour what are your settings in phpstorm? Both methods work (no highlighting on doesNotChain()->foo()), but warning for retrun parent:... ... _Return value is expexted` to be 'B', 'A' returned._

I still use 2019.1 ...

[tmotyl]

As a workaround we could do sth like this in child class (for parent methods which returns ’$this’)

Function foo(){
Parent::foo();
Return $this;
}

Sorry for formatting, im writing from mobile ;)

[sreichel]

imho @inheritdoc is "human" readable and it works perfectly (?) with ide`s ... less code, less possible errors, works with phpdoc/phpdox ....

<?php

class Master
{
    /**
     * Some description ....
     * 
     * @param $arg1
     * @param $arg2
     * @param @argN
     * @return $this
     */
    public function some($arg1, $arg2, $argN) {
        // some logic ...
        return $this;
    }
}

class A extends Master
{
    /**
     * Some copied description ....
     * 
     * @param $arg1
     * @param $arg2
     * @param @argN
     * @return $this
     */
    public function some($arg1, $arg2, $argN) {
        parent::some($arg1, $arg2, $argN);
        return $this;
    }
}

class B extends Master
{
    /**
     * @ineritdoc
     */
    public function some($arg1, $arg2, $argN) {
        parent::some($arg1, $arg2, $argN);
        return $this;
    }
}

[tmotyl]

for me inheritdoc is NOT human readable. I need to switch context to read the argument description.
@sreichel what does you example shows?

[sreichel]

You can see that one way or another. @inheritDoc tells me to look at the overridden method. This might be one step more, but how often is it needed? I think it is important that the IDE works, if necessary also tools like phpDocumentor or something similar works as well.

The example should only show that you can save yourself duplicate documentation, especially if the original description is a bit longer.

Instead of copying the DOCblock to child methods, which is not update safe, you could also use @inheritDoc.

[colinmollenhour]

Oops, my example was indeed flawed (// NOT recognized by PhpStorm) as it actually is recognized by PhpStorm. I can still concoct convoluted code that breaks PhpStorm when using @inheritDoc but it isn't as bad as I thought before. My personal preference would be not to use it but as long as IDE inspection works well I am not going to fuss about it anymore.. :)