Inheritance issues with Implement and Extend

benr77 commented 3 years ago

Bug Report

Q	A
BC Break	no
Library Version	0.2.0
PHP version	8.0

Summary

Do the Extend and Implement classes handle multiple levels of inheritance?

E.g. I have this line, to pick up Symfony form types:

$formTypeClassNames = Rule::allClasses()
        ->that(new ResideInOneOfTheseNamespaces('App\UI\Form'))
        ->andThat(new Implement(FormTypeInterface::class))
        ->should(new HaveNameMatching('*Type'))
        ->because('we want uniform form type naming');

However, it's ignoring all these classes, because they extend AbstractType which in turn implements FormTypeInterface

Expected behavior

I would expect the check to consider interfaces implemented, or classes extended from, all the way up the inheritance tree.

Thanks

AlessandroMinoccheri commented 3 years ago

Hi @benr77 you are right, I created a little test about the behavior and we need to check inside extend class other dependencies.

Broken test to validate the expected behavior:

public function test_it_should_parse_extends_class(): void
    {
        $code = <<< 'EOF'
<?php

namespace Root\Animals;

class Animal implements Foo
{
}

class Cat extends Animal
{

}
EOF;

        /** @var FileParser $fp */
        $fp = FileParserFactory::createFileParser();
        $fp->parse($code);

        $cd = $fp->getClassDescriptions()[1];

        $this->assertEquals(['Foo'], $cd->getInterfaces());
    }
}

AlessandroMinoccheri commented 3 years ago

Hi @benr77, we have discussed this bug / new feature and we think that at the moment we would like to avoid implementing a parser that finds all dependencies inherited by other classes because this can open a lot of scenarios that can be difficult to manage (example vendors), and performance problems. In my opinion, in your case, you can add a rule that checks that every class inside App\UI\Form extends AbstractType. And then another rule that says that AbstractType should implement FormTypeInterface. In this way, you can express your rule in your example.

What do you think?

lchrusciel commented 2 years ago

Hey folks!

thanks for your amazing work! Recently, we've introduced phparkitect in Sylius - https://github.com/Sylius/Sylius/pull/13426. It worked really nice. After few days, I've noticed, that we had one class out of namespace scope, so I fixed it and try to protect us in the future with creation of a new rule - https://github.com/Sylius/Sylius/pull/13495/commits/7772229adc4e5f181ca03748cb66288e8d66f428

What I would like to achieve, is to be sure that everything that implements ApiPlatform\Core\Api\FilterInterface (which is class from vendor) will be placed in Sylius\Bundle\ApiBundle\Filter namespace. The issue is, most of these classes do not implement this interface directly but through class inheritance. I was hoping that with phparkitect I would be able to enforce such rules even if they are based on vendor classes - evne tho, I can imagine that this increase complexity a lot.

AlessandroMinoccheri commented 2 years ago

Hi @lchrusciel we have discussed it many times and at the moment we are checking only the single class without building the full tree of inheritance. I understand your point of view, at the moment we found the solution to check if a class extends a specific interface or extends a specific one.

Checking recursively through inherited classes increases complexity but also the execution time of the tool. If you have a solution or a different point of view we can try to understand how to implement this feature if is necessary.

lchrusciel commented 2 years ago

I totally understand your performance concerns and I get your point. Perhaps, I will find time to check the potential for this change on my project. For now, I'm ok with the current expectation even tho it is not what I aimed to achieve in the first place.

Perhaps this tool is not mature enough for us to use it on Sylius or perhaps we are just use it wrongly. Either way, keep up good work, I bet many people still benefit from it :)

AlessandroMinoccheri commented 2 years ago

Hi @lchrusciel we are working on this PR #239 to solve your problem by getting dependencies recursively.

At the moment I tried to add in local into Sylius this rule:

$rules[] = Rule::allClasses()
        ->that(new ResideInOneOfTheseNamespaces('Sylius\Bundle\ApiBundle\Filter'))
        ->should(new Implement('ApiPlatform\Core\Api\FilterInterface'))
        ->because('We want to uniform code')
    ;

Is this rule what you expect from the tool? At the moment it works fine and no violations are detected with the rule.

lchrusciel commented 2 years ago

I'm grateful for your efforts. As I see PRs it is really tough work. thanks a lot 👍

AlessandroMinoccheri commented 2 years ago

@lchrusciel it's a delicate thing the recursive search, for this reason I am thinking to merge this PR but with a different method to give to users two options:

check as today
check recursively with many possible side effect

What do you think? cc @micheleorselli

lchrusciel commented 2 years ago

From my perspective, we are still using your tool and plan to use it even more (we've scheduled some modularity work). In this context (so assignment of class to given bounded contexts and not leaking to others) it works great. Thanks for that! Perhaps, I was just overshooting with my expectations. I still think that it would be great to ensure that classes in a given folder/namespace fulfil some interface expectations, but maybe it is too much for now

phparkitect / arkitect