GraphQLite attempts to load non-class files, resulting in weird errors

[oprypkhantc]

Hey.

We have a modules/ folder that maps to Modules\ PSR-4 namespace. This namespace was also added to be globbed by graphqlite using ->addTypeNamespace('Modules\\') and -> addControllerNamespace('Modules\\'). That namespace also contains non-class files (such as lang files and others), as well as pre-8.0 enums that require autoloading before they can be used.

If you try to blindly require these files, you'll get errors as those non-class files are meant to be loaded from a specific context, and those enums require a special autoloader to call ::initialize() static method on them.

The problem is that GraphQLite does exactly that - blindly load all files using require_once that appear as classes to it. Obviously, this is not desired, as it forces you to list every class separately in calls to ->addTypeNamespace() and ->addControllerNamespace() to avoid loading what it can't load.

This is happening because of this fix: https://github.com/thecodingmachine/graphqlite/blob/master/src/Utils/Namespaces/NS.php#L55. It was done specifically to avoid issues with autoloading, and to allow autoloading incorrectly namespaced classes.

Instead of doing that, GraphQLite can instead attempt to load a class through autoloading and simply skip it if it fails. This will break loading of classes that have incorrect namespaces.

What do you think @oojacoboo? I'll draft a PR if it's an acceptable BC.

[oojacoboo]

What if addTypeNamespace and addControllerNamespace took an additional argument to explicitly require the files, as opposed to only relying on the autoloader. Also, does it make sense to check the files being skipped to see if they include GraphQLite attributes, and if so, throw an Exception? Quietly skipping files, while it might work in most cases, in others would probably be an unexpected behavior.

[oprypkhantc]

I've taken a look at how other frameworks do it:

Symfony loads controller routes in two ways:

• first is attribute loader, where they tokenize PHP files to see if it contains a class name and parse it - presumably to avoid parsing composer.json or forcing PSR-4 compliant namespaces. This avoids loading non-class files by checking whether a file even contains a class: https://github.com/symfony/routing/blob/7.0/Loader/AttributeFileLoader.php#L52
• second is PSR-4 loader, which simply calls class_exists(). This avoids loading non-class files by calling class_exists():: https://github.com/symfony/routing/blob/7.0/Loader/Psr4DirectoryLoader.php#L82

Spiral also does some weird stuff by tokenizing PHP files and extracting all defined classes, like Symfony does in the first case: https://github.com/spiral/framework/blob/master/src/Tokenizer/src/ClassLocator.php#L55

None of these support a non auto-loaded use case though, but we can do an argument no problem.

As for the skipping files, it's not trivial to check whether code contains GraphQLite attributes as that requires the class to be already loaded to access (new ReflectionClass())->getAttributes(). We could do workarounds with str_contains(), but that would be an unreliable fix to say the least.

All I can think of is the following:

• for a non auto-loaded use case, we can preserve the existing behaviour of require_once(), which will not quietly skip files at all
• for an auto-loaded use case, we can do class_exists() checks to see if a class exists. This will trigger autoloading. If a class doesn't exist (because it's not a class), we'll skip it. If a class does exist then it'll be loaded and if something fails, it will throw an error

[oojacoboo]

Seems reasonable to me

[oprypkhantc]

Ha @oojacoboo. I was working on a pull request and writing tests and it turns out the previous "fix" that doesn't use autoloading doesn't actually work as I expected 😆

What I expected was that it allowed a class to have an incorrect namespace in two scenarios:

1. a class is defined under one of PSR-0 or PSR-4 namespaces from composer.json, but has incorrect namespace
2. a class is defined outside of namespaces from composer.json

That's not the case though 🥲 The way it currently works is GlobClassExplorer returns a map of potential class names and their respective file paths based on composer.json's PSR-0 and PSR-4 namespaces. Something like this:

// Result of GlobClassExplorer::getClassMap()
array:1 [
  "TheCodingMachine\GraphQLite\Fixtures\BadNamespace\BadNamespaceClass" => "/opt/project/tests/Fixtures/BadNamespace/BadNamespaceClass.php"
]

// BadNamespaceClass.php
<?php

namespace OtherNamespace;

class BadNamespaceClass {}

In the first case, where a class has a namespace that doesn't match PSR-0 or PSR-4, the file would indeed be loaded. The problem is that although a class was loaded, we don't know it's full class name :) In the example above, the one GlobClassExplorer guessed for us is TheCodingMachine\GraphQLite\Fixtures\BadNamespace\BadNamespaceClass, however an actual class name is OtherNamespace\BadNamespaceClass.

Because there's still no class with class name TheCodingMachine\GraphQLite\Fixtures\BadNamespace\BadNamespaceClass loaded, a check below require_once fails and the file is still skipped: https://github.com/thecodingmachine/graphqlite/blob/master/src/Utils/Namespaces/NS.php#L63

In the second case, GlobClassExplorer doesn't even return our BadNamespaceClass as an option, because it's outside of mapped PSR-0 or PSR-4 directories.

---

All this to say even the current behaviour essentially silently skips all files with incorrect namespaces, and it was specifically made not to get an error on an invalid namespace in a file.

[oprypkhantc]

Oh, and for some reason this was only a fix for ->addTypeNamespace(). ->addControllerNamespace() always uses autoloading 🤷🏻

[oojacoboo]

Please also see this ticket, see if there is some overlap: https://github.com/thecodingmachine/graphqlite/issues/657

[oprypkhantc]

Not much of an overlap.

But I've searched through packagist and it seems there's one up-to-date package that does correctly parse all namespaces, including those from vendor. It does so by accessing the Composer's autoloader functions, instead of parsing composer.json: https://github.com/alekitto/class-finder/tree/master. That would fix the #657, but doesn't relate to this

[fogrye]

@oprypkhantc I think it will relate unfortunately, lib you suggested is great, thx for that, but it does exactly the same what you mention (require_once everything it can), but I have an easier solution for you: I'm leaving possibility to provide FinderInterface and you can preconfigure your loader with FinderInterface->notInNamespace() to blacklist folders which should be ignored. I hope this will solve your issue too.

[oprypkhantc]

@fogrye Not sure what you mean. The lib I suggested only globs classes, the "load" part is on the graphqlite side. There's no need for blacklists if we just allow using proper autoloading as it should've been in the first place.

[fogrye]

@oprypkhantc as you can see here files will be autoloaded so, unfortunately, if I understand your issue correctly, the only way left is to blacklist.

[oprypkhantc]

@oprypkhantc as you can see here files will be autoloaded so, unfortunately, if I understand your issue correctly, the only way left is to blacklist.

Sorry, didn't notice that. That's unfortunate, but we can still use that package's idea to support vendor loading - instead of parsing the composer.json, we could pull namespace mapping from Composer's AutoLoader class.

I don't believe excludes/blacklist is a good idea as it feels like a step backwards :(

[fogrye]

@oprypkhantc I copied the test you have created and it passes with mine changes #664, I will try to figure out how to test BadNamespaceClass without moving and and new test case.

[fogrye]

No, you placed them in best way possible, you can check this commit, I hope this will solve your issue too. Thanks a lot.

[fogrye]

Assuming that we will resolve this after migration to class-finder I would like to add commentary regarding how to specify that some namespaces should ignore autoloading: I think additional bool as parameter addTypeNamespace will complicate configuration with, for example, symfony bundle, don't you think that separate method for type namespaces with disabled autoloader is more elegant solution?

[oojacoboo]

@oprypkhantc was this resolved by https://github.com/alekitto/class-finder/commit/4187ae1a09cfd63994961d0551823440d4f21319? If so, a PR will need to be submitted to update the version, after it's tagged.

[oprypkhantc]

@oojacoboo Unfortunately not, as it still uses include instead of autoloading by default. I was waiting for @fogrye's PR to be merged so I can re-do mine :)

[oojacoboo]

@oprypkhantc you can't rely on Composer's autoload file?

We actually ran into a similar issue with some .php files that were templates inside a plugin lib. We resolved that by changing the file extension, which resolved a weird include issue with class-finder. However, that's not been an issue with the Composer autoload, and by using that autoload file directly, it should have worked fine.

[oprypkhantc]

What do you mean by relying on Composer's autoload file?

The problem with loading arbitrary .php files is now fixed, thanks to alekitto/class-finder using a try-catch and @fogrye implementing the necessary changes. But the problem of not using autoloading to load classes is still an issue, as some code may specifically rely on custom autoloaders, like it is in our case.