ClassManifest picks up classes that won't exists because of `class_exists` statements

[maxime-rainville]

Affected Version

^4.0

Description

Sometimes you have classes that will only conditionally exists if another class exists. This is helpful when you're implementing a feature that will only be available if another module is installed.

Only problem is that because ClassManifest uses static code analysis to decide what classes exists it won't pick up if there's a statement like this at the start of the file.

if (!class_exists(RegistryPage::class)) {
    return;
}

Steps to Reproduce

Create a file with this code.

<?php

use SilverStripe\Core\ClassInfo;
use SilverStripe\Dev\BuildTask;
use SilverStripe\ORM\DataObject;

class TestBuild extends BuildTask {

    public function run($request)
    {
        $subClasses = ClassInfo::subclassesFor(DataObject::class);

        if (in_array('Test', $subClasses)) {
            echo "Test is returned by subclassesFor\n";
        } else {
            echo "Test is NOT returned by subclassesFor\n";
        }

    }

}

if (!class_exists(NonExistant::class)) {
    return;
}

class Test extends DataObject {

}

Expected outcome: When you run sake dev/tasks/TestBuild, "Test is NOT returned by subclassesFor" should be outputted. Actual outcome: When you run sake dev/tasks/TestBuild, "Test is returned by subclassesFor" is outputted.

[maxime-rainville]

Fixing this properly would probably involved updating ClassManifestVisitor to detect if a class is preceded by a if (!class_exists) statement. Getting this to work consistently could be a bit of a mission.

[maxime-rainville]

This bug is causing some issue with the BehatExtension module. I've got some dumb PR to plaster over it for now. https://github.com/silverstripe/silverstripe-behat-extension/pull/192

[ScopeyNZ]

Does it behave differently if you wrap the whole class in if (!class_exists)?

I've said this somewhere in the past while debating this stuff. The conditions preventing class declarations is some crazy PHP magic, as normally files aren't executed by PHP when parsing class definitions, but it makes some sort of exception for those specific statements.

[maxime-rainville]

Just tried it. We seemed to have the reverse problem when we surround the class definition by if class_exists statement ... ClassManifest doesn't pick up the class even if the class_exists calls return true.

<?php

use SilverStripe\Core\ClassInfo;
use SilverStripe\Dev\BuildTask;
use SilverStripe\ORM\DataObject;

class TestBuild extends BuildTask {

    public function run($request)
    {
        $subClasses = ClassInfo::subclassesFor(DataObject::class);

        if (in_array('Test', $subClasses)) {
            echo "Test is returned by subclassesFor\n";
        } else {
            echo "Test is NOT returned by subclassesFor\n";
        }

    }

}

if (class_exists(DataObject::class)) {
    class Test extends DataObject {

    }

}

[dhensby]

I think this might be because our class finder only looks at the first level of nesting to find a class declaration (for speed) so if you wrap the class declaration in a conditional it won't see the class declaration

[emteknetnz]

if (!class_exists(NonExistant::class)) {
    return;
}

class Test extends DataObject {
}

This kind of strikes me as a pretty unusual way to code. I'm not sure why you wouldn't just declare all the classes. Rather than fixing the ClassManifest I'd be more inclined to say don't code like this.

Do you have a examples, particularly from core, where we do this?

[ScopeyNZ]

Feel free to do a project wide search on cwp/cwp-recipe-kitchen-sink. We do it all over the place to check for something before extending it. Eg:

https://github.com/silverstripe/silverstripe-versioned/search?q=class_exists&unscoped_q=class_exists

[emteknetnz]

I looked at the first example, seems like it illustrates why we need it:

silverstripe/versioned VersionedStage.php

<?php

namespace SilverStripe\Versioned\GraphQL\Types;

use GraphQL\Type\Definition\EnumType;
use SilverStripe\GraphQL\TypeCreator;
use SilverStripe\Versioned\Versioned;

if (!class_exists(TypeCreator::class)) {
    return;
}

class VersionedStage extends TypeCreator

So I guess what happened here is this file kinda-of-should-have-been in the graphql module instead of versioned module. Or maybe have made a hybrid silverstripe/versioned-graphql module. or not :-p

So if we were to remove the class_exists() here, then we would need to update silverstripe/versioned composer.json to make silverstripe/graphql a requirement, which maybe isn't totally unreasonable

If we're not wanting to require more modules in composer.json (which seems cleaner to me), then seem like we need the class_exists() and also fix the ClassManifest

[ScopeyNZ]

Yeah the class_exists stuff is pretty much unavoidable. I disagree with arbitrarily adding graphql as a requirement to versioned, just because versioned has support for it. The idea of a compatibility module (eg. silverstripe-versioned-graphql) is how it should be done ideally imo, but that's a lot of extra modules.

also fix the ClassManifest

I think that's what this issue is implying we need to do.

[robbieaverill]

So I guess what happened here is this file kinda-of-should-have-been in the graphql module instead of versioned module. Or maybe have made a hybrid silverstripe/versioned-graphql module. or not :-p

We've repeatedly had to decide how we handle this issue, and the class_exists() checks have always been the best with all factors weighed.

The reason we do this is because we have modular parts of our code which are designed to work in the presence of various other modules. The "right" way to do it would be to split everything up into compatibility modules, as you suggested @emteknetnz with your emoji, e.g. silverstripe-versioned-graphql.

We've chosen not to do this in the past because (A) it would be a breaking change to do so, and (B) it would create quite a disturbance to our VCS history. There are likely other reasons too, but these are the ones I can think of.

We should decide if this bug is high enough impact to warrant us removing all class_exists() checks and moving all inter-dependent code into new compatibility modules. My suspicion is that it won't be worth doing until SilverStripe 5, where we have an opportunity to break API contracts anyway.

[emteknetnz]

We should decide if this bug is high enough impact to warrant us removing all class_exists()

If it involves creating compatibility modules, then I'd say not even close, it's way too much of a breaking change to warrant it just to remove some class_exists() statements

This particular issue only surfaced because frameworktest was included on asset-admin for a new behat test. Given it's taken this long for this issue to appear then it's probably not that big of an issue

class_exists() is kind of ugly, though it's also understandable (i.e. not confusing) the other non-breaking way of fixing this is composer requirements spam, and people seem even less keen on that which is understandable

[maxime-rainville]

I think using class_exists to conditionally provide additional feature when another module is installed is a totally valid use case.

Ideally, we would update our ClassManifest static parser logic to be smart enough to understand the class_exist statement. This sound difficult because there's all sort of ways you could potentially conditionally create a class.

Alternatively, we could update methods on ClassInfo to filter out classes that don't exists at run time.

[dhensby]

Firstly, here's the code responsible: https://github.com/silverstripe/silverstripe-framework/blob/4.5.3/src/Core/Manifest/ClassManifest.php#L563-L578 or, more specifically: https://github.com/silverstripe/silverstripe-framework/blob/4.5.3/src/Core/Manifest/ClassContentRemover.php - it parses a file looking for class / trait / interface definitions and once it sees one it stops parsing deeper into the tree.

Thoughts:

1. ClassManifest makes no promises to find classes that are or are not available at run time. It parses the PHP files and looks for classes that are defined in code. Just like if we dynamically created a class (as can happen in PHPUnit or other libraries that create mocked objects), these classes won't show in the manifest. edit to elaborate - these classes are defined in code, they are just not defined in runtime because the runtime logic doesn't allow them to be evaluated. In my view those classes do exist and should be included in the manifest, the fact there is some potentially arbitrary conditions on the runtime, make this impossible to deal with safely.
2. What's the problem with showing classes that don't exist because of the conditionals? Is there any core logic that's causing errors because it depends on this?
3. I don't think it's reasonable to expect the class manifest to parse and evaluate conditionals in the class declaration files. I don't like this pattern and I don't think it's legitimate, the reason we do it is to save us having to add extra layers of arbitrary abstraction.

If we really don't want this behaviour, as @maxime-rainville suggests, ClassInfo can filter the classes using class_exists() but I think that is slow and inefficient; if there are use-cases a developer experiences when this happens, they should probably implement that filtering themselves.

tl;dr: I vote this issue is expected behaviour

[maxime-rainville]

If I call a method called subclassesOf, my expectation is that the returned classes will exist, but I don't think it's a major problem. So even if we think it's not the expected behaviour, it might not be worth fixing.

The issue came to light because I was getting an error in behat-extension where we would loop over the results of ClassInfo::subclassesOf(DataObject::class) and create a singleton from that which would throw a ReflectionException. We've bypassed the problem for now.

[dhensby]

OK - I think we should get some core feedback on this (@silverstripe/core-team).

So, solutions I see - any other ideas and I can add them:

👍: Update the docs to make it clear that the manifest / ClassInfo may return classes that aren't defined at runtime and it's for the developer to check these ❤️: Update ClassInfo (or ClassManifest if appropriate) to use class_exists() calls to filter out classes that are not defined at runtime 🎉: Update ClassManifest to evaluate conditionals that appear to prevent definition of classes at runtime

[dhensby]

My view is that "fixing" the manifest to attempt to evaluate conditions that may or may not prevent the definition of the class at runtime to be too much work, too risky (lots of edge cases to try to deal with) and would likely cause false negatives (which I think is worse than false positives).

We also have been doing what we can to make the ClassManifest as fast as possible, trying to evaluate these conditions (especially class_exists ones) will require multiple passes over the detected classes.

I think it would also be interesting to see how composer does this for their classmaps; if the classmaps composer produces also suffer this problem, I really don't think we should be trying to do better than that.

---

A quick test showed me that classes still appear in the classmap for composer even if they aren't defined at runtime.

On further thought I think if we were to fix this it would have to be fixed in ClassInfo and not in the manifest, the manifest is/was basically our autoloader and so it should be telling us where a class lives if it were defined, we can't / shouldn't evaluate if it's defined before trying to load the file, but ClassInfo can tell us if it's defined at runtime after attempting to include it.

[sminnee]

Yeah I would be inclined to keep ClassManifest as-is, but potentially add a class_exists filter to subclassesOf or maybe some of the places that call it.

I don’t think that fixing this by banning the use of load-time class_exists calls should be given too much consideration.

[dhensby]

OK - based on the votes of those core contributors that have responded I'll reclassify this as a docs issue.

[maxime-rainville]

Not that it should cause us to reconsider the "this is a doc issue" conclusion, but I think this behaviour prevented our CI from catching a missing import in one of those class_exist statement.

Basically, we had this bit of logic where we forgot to add a use statement for TypeCreator. The test would still load the class in memory because of the manifest. Interestingly, we never picked up the bug with behavioral testing or regression testing ... which might imply that our class_exist check doesn't do anything ever.

if (!class_exists(TypeCreator::class)) {
    return;
}

[GuySartorelli]

It's worth noting that class_exists checks will return false positives. e.g with the following:

if (!class_exists(NonExistant::class)) {
    return;
}

class Test extends NonExistant
{
    // ...
}

// This will dump "true"
var_dump(class_exists(Test::class));

However, if you wrap the class like so

if (class_exists(NonExistant::class)) {
    class Test extends NonExistant
    {
        // ...
    }
}

// This will dump "false"
var_dump(class_exists(Test::class));

Therefore it wouldn't be possible to resolve this just in ClassInfo as suggested in https://github.com/silverstripe/silverstripe-framework/issues/9500#issuecomment-621714111 (I tried). It's either do something in class manifest or just document it as is (and document it seems to have won that battle).

[GuySartorelli]

Looks like wrapping it like

if (class_exists(NonExistant::class)) {
    class Test extends NonExistant
    {
        // ...
    }
}

also causes problems where sometimes (if not always?) this just always returns false for class_exists(Test::class)...

[GuySartorelli]

Okay. I've done some more testing on this. It seems there's a problem if chaining class_exists like this, where both Test and Test2 are defined in the same module:

if (!class_exists(NonExistant::class)) {
    return;
}

class Test extends NonExistant
{
    // ...
}

if (!class_exists(Test::class)) {
    return;
}

class Test2 extends DataObject
{
    private static $extensions = [
        Test::class
    ];
}

// This will dump "false"
var_dump(class_exists(Test::class));
// This will dump "true"
var_dump(class_exists(Test2::class));

Then in dev/build there will be an error because it tries to use the Test2 class (because class_exists(Test2::class) evaluates to true), which references the non-existent Test class (because class_exists(Test::class) evaluates to false).

Edit: Further testing shows that this is weirder than I expected. If you change Test2 extends DataObject to Test2 extends Test then class_exists(Test2::class) will correctly evaluate to false... so there's somewhere weird going on where if you inherit a class that doesn't exist class_exists gives the expected result, but if you inherit from a class that does exist, class_exists just assumes your class exists too.