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

Rely on ClassInfo to retrieve all classes #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rskuipers
Copy link

@rskuipers rskuipers commented Apr 3, 2023

We were frequently getting strange issues where getOwner() could not be properly resolved.
After some debugging I came to the following conclusion.

get_declared_classes() only returns a list of classes that were loaded by either being referenced by another class, or because that class was being analyzed.
So any extensions that were attached to a class that wasn't yet analyzed or otherwise referenced in the code and used $this->getOwner() would fail because the code would conclude the extension wasn't applied on any class.

By using ClassInfo::allClasses() we utilize the ClassManifest of SilverStripe which contains all classes defined.

This resolves quite a few headaches for us (we've been trying to track this down for a bit).

@mleutenegger
Copy link
Member

It seems like there was a problem with the workflow. In the meantime workflow-definitions have been updated but Github seems to not allow me to re-run the checks. I know this sounds silly, but could you push a commit (e.g. a newline) so it is forced to rerun them?

Previous implementation would miss a lot of classes because they're lazy loaded. Using ClassInfo makes sure we go through all available classes.
@rskuipers rskuipers force-pushed the bugfix/fix-missing-classes branch from 8c2e199 to a4ccd20 Compare May 22, 2023 19:21
@rskuipers
Copy link
Author

@mleutenegger no worries! done.

@mleutenegger
Copy link
Member

mleutenegger commented May 31, 2023

Interesting, it seems like the tests are failing because of the reverse of the problem you tried to fix 🤪 .

While ClassInfo::allClasses() returns all classes in the project, class_uses(...) does not know them and throws a warning which then kills the test runner. As (for obvious reasons) we can't really skip classes that do not exist, I wonder if there is a working solution using a mix of \ReflectionClass::getTraits() and the ClassInfo::subclassesFor(...) method.

Basically, instead of doing it like this

$classes = ClassInfo::allClasses();
$result = array();
foreach ($classes as $class) {
$hasTrait = false;
foreach ($classes as $subclass) {
$hasTrait = false;
foreach (class_uses($class) as $trait) {
$hasTrait = $hasTrait || $trait == ClassHelper::Extensible;
}
if ($hasTrait) {
$result[$subclass] = $subclass;
}
}
}
$result = array_values($result);
return $result;

We do something like this:

$classes = ClassInfo::allClasses();
$result = array();
foreach ($classes as $class) {
    $reflection = new \ReflectionClass($class);
    $traits = $reflection->getTraits();
    if ( /* "Extensible" in $traits */ ) {
        foreach (ClassInfo::subclassesFor($class, true) as $subclass) {
            $result[$subclass] = $subclass;
        }
    }
}
$result = array_values($result); 
return $result; 

If you have the time to try it, that would be great

@mleutenegger
Copy link
Member

mleutenegger commented May 31, 2023

additionally, I am not quite sure if this double-loop over the same class-list even leads to the correct result, as it would loop over the same class list twice, instead of actually filtering out subclasses of those using the Extensible trait.

foreach ($classes as $class) {
$hasTrait = false;
foreach ($classes as $subclass) {
$hasTrait = false;

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

Successfully merging this pull request may close these issues.

2 participants