-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[12.x] Class uses recursive #56079
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
base: 12.x
Are you sure you want to change the base?
[12.x] Class uses recursive #56079
Conversation
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Are you going to add trait_uses_recursive()
as well?
* @param $haystackClassName | ||
* @return bool | ||
*/ | ||
public static function inArray($needleClassName, $haystackClassName): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about naming it uses()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally aware about the naming.
This is very much what I had envisioned. I would suggest:
|
|
||
namespace Illuminate\Cache; | ||
|
||
class ClassUsesRecursive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might say, we could go as far as understanding the class as broader (e.g. ClassMetaData
) to potentially include other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, maybe \Illuminate\Support\Reflector
is that class already 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments! 🙏
You're right: I think I will move my code to Reflector
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -11,6 +11,43 @@ | |||
|
|||
class Reflector | |||
{ | |||
private static array $classesUsesRecursive = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think of Laravel/package code uses protected to allow for extension.
private static array $classesUsesRecursive = []; | |
/** | |
* @var array<class-string, list<class-string>> | |
*/ | |
protected static array $classesUsesRecursive = []; |
One thing, you might also cover here, is the following scenario: We have Reflector::classesUsesRecursive(Child::class); // returns [Parent::class, Grandparent::class] (uncached)
Reflector::classesUsesRecursive(Child::class); // returns [Parent::class, Grandparent::class] (now cached)
Reflector::classesUsesRecursive(Parent::class); // returns [Grandparent::class] (uncached) The invocation with |
This PR adds a new class which allow to cache all traits used by a class, its parent classes and trait of their traits.
As suggested in #56069 (comment)