Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClassReflection: make getTraits recursive #557

Merged
merged 7 commits into from Jun 17, 2021
Merged

ClassReflection: make getTraits recursive #557

merged 7 commits into from Jun 17, 2021

Conversation

iamrgroot
Copy link
Contributor

@iamrgroot iamrgroot commented Jun 14, 2021

Changes the ClassReflection::getTraits function to be recursive according to the suggestion from @ondrejmirtes at larastan/larastan#837 (comment)

@iamrgroot iamrgroot changed the title ClassReflection: make getTraits use collectTraits ClassReflection: make getTraits recursive Jun 14, 2021
@ondrejmirtes
Copy link
Member

This change needs a test.

@iamrgroot
Copy link
Contributor Author

@ondrejmirtes should ClassReflection::getTraitNames also use ClassReflection::collectTraits?

In terms of tests needed: a test for the traits returned by ClassReflection::getTraits suffices or do you have more in mind? It's a bit difficult to dive into all of the test quickly.

@ondrejmirtes
Copy link
Member

should ClassReflection::getTraitNames also use ClassReflection::collectTraits

Nope, that's only used in hasTraitUse() and that can stay non-recursive.

About the test for this PR - I'd like to have a new file in data directory next to ClassReflectionTest that adds some example classes with nested trait uses and then have a new testGetTraits() in ClassReflectionTest that checks that getTraits() returns the right thing.

You can obtain ClassReflection instance with this code in the test case:

$reflectionProvider = $this->createBroker();
$reflection = $reflectionProvider->getClass($className);

@iamrgroot
Copy link
Contributor Author

iamrgroot commented Jun 15, 2021

@ondrejmirtes I've discovered that ClassReflection::collectTraits does not detect Traits that have been defined in parent classes. For example in my test case:

trait BazTrait
{
}

class Baz
{
    use BazTrait;
}

class BazChild extends Baz
{
}

($this->createBroker())->getClass(Baz::class)->getTraits() gives ['BazTrait']

($this->createBroker())->getClass(BazChild::class)->getTraits() gives []

Is this indended behavior? I noticed that if I change it to also include parent traits recursively, the following tests fail: AnnotationsPropertiesClassReflectionExtensionTest::testProperties, AnnotationsMethodsClassReflectionExtensionTest::testMethods, ClassReflectionTest::testClassHierarchyDistances. This is probably because of a different order in which the traits are discovered.

What is your take on this? For the Larastan use case, there is a function needed that just returns all traits for a class (and all of the parents).

@ondrejmirtes
Copy link
Member

This is probably a sign we should care about backward compatibility more (when it even breaks PHPStan internals). So I suggest this:

  1. Add getTraits(bool $recursive = false) optional parameter
  2. Test behaviour of both getTraits(true) and getTraits(false).
  3. Change Larastan so that it uses getTraits(true).
  4. In 1.0 myself I'll change this API to something nicer - like getTraits() (recursive) and getImmediateTraits() (current behaviour of getTraits().

@dktapps
Copy link
Contributor

dktapps commented Jun 16, 2021

What about adding a getTraitsRecursive() method? It would be more self-documenting than an opaque boolean parameter.

@ondrejmirtes
Copy link
Member

This is just a temporary measure, I'll do something cleaner in 1.0.

src/Reflection/ClassReflection.php Outdated Show resolved Hide resolved
@ondrejmirtes ondrejmirtes merged commit 07d78b2 into phpstan:master Jun 17, 2021
@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants