Phpinspectionsea: [new, configuration] long inheritance chain: to work with ORMs

[kalessil]

Original link: https://bitbucket.org/kalessil/phpinspectionsea/issues/228/long-inheritance-chain-introduce
Author: @marekjalovec

See original link

[Jean85]

Should this config have a default ignore for some kind of classes? I.e., I found it not useful when the long chain is highlighted on test classes, where a long chain is expected (PHPUnit base class > Symfony WebTestCase > my BaseTestCase > my test class)

[marekjalovec]

file types and wildcard matching - *Test.php, *Spec.php and *.phpt files can be probably safely ignored completely.

[mente]

In description of inspection it says:

Analysis is not performed on test classes, which often introduce mocks that would violate this rule, or exceptions, which often contain domain-specific structures.

However it's performed for our phpunit classes. Either description is wrong or what @marekjalovec can be fixed

[kalessil]

Looks like a regression not covered yet by tests, will take care about the regression in a separate ticket.

[mente]

If it's regression - do you need more detailed information to reproduce? I've checked LongInheritanceChainInspector and didn't find any exclusions for tests there

[kalessil]

c1da949 Vladimir Reznichenko on 2/29/2016 at 6:15 PM LongInheritanceChainInspector: do not report deprecated classes (fixed #178) - at that time I dropped tests classes filtering.

[kalessil]

Fixed for test classes (thank you @marekjalovec for additional specs), binary pushed as well.

[kalessil]

Most used cases to cover:

• Doctrine 1/2 ?
• Symfony 2/3 ?
• PhpUnit: \PHPUnit_Framework_TestCase, \PHPUnit\Framework\TestCase
• Yii2: \yii\base\Component, \yii\base\Object

Additional base clasees to consider:

• Propel1: \ModelCriteria, \BaseObject
• Propel2: \Propel\Runtime\ActiveQuery\ModelCriteria
• Symfony1: \sfBaseTask, \sfActions, \sfProjectConfiguration

[Ocramius]

@kalessil exceptions generally are inheritances

[kalessil]

@Ocramius, sounds good. Thank you!

[Azaret]

Hi,
Could this work with some parameter or annotation, rather or in plus of white listing ?

Ie, we're using an internal framework at work and we can easily hit the 3 inheritance with ORM & Form classes.
Usual case is a Form class extending the Entity Class, which extends a db specific wrapper, which extends the normal Table class from the ORM.
So we have 4 classes inheritance which stay a legit design, I'm pretty sure you can have the same things happening in Symfony.

So it could be nice to just have to annotate the ORM Table Class, so any inherited classes would be ignored.

[Ocramius]

we can easily hit the 3 inheritance with ORM & Form classes.

That is already quite the violation though.

Usual case is a Form class extending the Entity Class

This is a huge violation, from an ORM developer perspective: now you have something behaving both as entity and as form, yet no uniform interface is in common.

which extends a db specific wrapper

Argh

which extends the normal Table class from the ORM.

Argh ^ 2

So we have 4 classes inheritance which stay a legit design, I'm pretty sure you can have the same things happening in Symfony.

No, this design is to be considered BAD and it is only acceptable due to legacy constraints and consistency with previous design decisions.

So it could be nice to just have to annotate the ORM Table Class, so any inherited classes would be ignored.

No, this basically means "allow this to be a god class". You are actively building on an anti-pattern here, and you need to be aware of it. Don't try to suppress it at the top level of the inheritance.
The tool is correct in screaming at you, and it should continue to do so every time you add a child class.

[kalessil]

Moving discussion from Twitter: I have a list of classes (PhpUnit and etc) which treated as root classes for inheritance depth calculation.

If we reach the class, the depth calculation stops. It's pretty fair as both framework and based apps developers got warned when overdoing with inheriting.

And input for ZF needed is: which classes (base controllers and etc) I could add to the list?

[froschdesign]

@kalessil

And input for ZF needed is: which classes (base controllers and etc) I could add to the list?

Controller classes are not a problem: MyController extends AbstractActionController extends AbstractController

Usually a form has 3 parent classes: MyForm extends Form extends Fieldset extends Element:

showStoppers.add("\\Zend\\Form\\Form");

[kalessil]

Thank you @froschdesign

[Jean85]

Shouldn't Controller classes be ignored too? I would get a warning if I write an abstract BaseController extends AbstractActionController to store some common methods in my app...

[asgrim]

I think controllers should not be extended like that. Inject that kind of functionality IMO. We're talking best practices here, and reducing inheritance is the goal.

Forms are annoying, but unavoidable as @froschdesign already pointed out.

+1 for form in ZF
-1 for controllers

[froschdesign]

@Jean85

Shouldn't Controller classes be ignored too?

No, because if your class BaseController is an extra layer between the AbstractActionController and a concrete controller then the inspection is correct.

This construct is a problem and not needed in a ZF application:

MyController extends BaseController extends AbstractActionController extends AbstractController

…to store some common methods in my app

Please look at the Controller Plugins and write your own. This should help in most cases.

---

Btw. this is good example why the inspection is needed and correct!

[Jean85]

Thanks for the input @asgrim and @froschdesign. In reality, I'm not a ZF dev, I mainly work with Symfony, and I was porting my issue from the SF Controller structure.

In that case, I have a BaseController to store some shortcuts, like this one for flash notices:

    protected function addFlashBagNotice($message)
    {
        $this->get('session')
            ->getFlashBag()
            ->add('notice', $message);
    }

Do you still think that this kind of stuff is a bad practice that should get a warning? I'm not aware of an homologous of controller plugins in SF controllers.

[asgrim]

Generalised, I'd inject an implementation of something like FlashBagMessengerServiceInterface. It looks like you're receiving directly from a service locator there (shouldn't inject the whole service locator, just what you use). I'm on my phone otherwise I'd give a more comprehensive example ,sorry. ;)

[Jean85]

Understood, don't worry, thanks!

[kalessil]

Thanks for your feedback guys!

[froschdesign]

@Jean85
If your controller extends Symfony\Bundle\FrameworkBundle\Controller\Controller then you can use the addFlash method. No extra layer is needed and no exclusions in "Php Inspections".

[Jean85]

@froschdesign thanks, I missed that!
We had that shortcut since 2.3 and we forgot to switch to the official one when that got out.

[kalessil]

For myself: keep only PhpUnit, Form classes (ZF, Symfony) and review Yii show stoppers.

[kalessil]

Status update: great feedback, here essential changes:

• test cases - to review behaviour
• agreement to not make controllers show-stoppers in future