thecodingmachine / graphqlite

Use PHP Attributes/Annotations to declare your GraphQL API
https://graphqlite.thecodingmachine.io
MIT License
554 stars 95 forks source link

feat: replace thecodingmachine/class-explorer with kcs/class-finder #664

Closed fogrye closed 4 months ago

fogrye commented 4 months ago

Main issue is to let type mappers find types in vendor packages which class-explorer and maintainer is not updating the project.

Symfony and Laravel bundles have to be updated too. Fixes: #657

oprypkhantc commented 4 months ago

It seems to me that because of this catch block here, one of my use cases (loading non class files) would be solved.

However the other would still be broken - anything that requires autoloading won't function as expected. One example of that - our pre-8.0 enums require special autoloader to call ::initialize() on them to initialize all static variables with enum instances, and there are about 150 of them in our codebase. Excluding them is doable of course, but I'm just not entirely sure not using autoloading is the way to go, as it feels like an incredibly large hack to me.

I've taken a look at the issue that's mentioned in alekitto's package to see what was the root of the problem that made them use include instead of loading the class through autoloading. They mention this issue: https://github.com/composer/composer/issues/6987. In short, the include is there to avoid loading the same file twice IF it's present in a PSR-0/PSR-4 namespaces mapping AND manually loaded via files.

If you just do

class_exists('Aws\functions');
class_exists('Aws\functions');

it breaks. So the worst thing here is it does indeed look like Composer's bug that they don't want to fix :/ Now it makes sense to me why both Symfony and Spiral parse the file to see if it contains a class before trying to autoload it.

To be honest I don't like either solution. We could still try two things - re-reporting this as a bug with a clearer example to composer, or attempt to fix class-finder in a different way - by first checking whether given file is amongst Composer's autoload_files.php file.

That solution would satisfy both needs without compromises if it works :/

oprypkhantc commented 4 months ago

I've submitted an issue on the class-finder side. Maybe we can get the fix done on their side 🤷🏻

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.27%. Comparing base (53f9d49) to head (90d9b95). Report is 70 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #664 +/- ## ============================================ - Coverage 95.72% 95.27% -0.45% - Complexity 1773 1824 +51 ============================================ Files 154 171 +17 Lines 4586 4848 +262 ============================================ + Hits 4390 4619 +229 - Misses 196 229 +33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

oojacoboo commented 4 months ago

@fogrye this looks good - thanks! It's a BC breaking PR. That's fine though, as we're targeting a new major version for the next tag.