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

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

Merged
merged 7 commits into from
Mar 20, 2024

Conversation

fogrye
Copy link
Contributor

@fogrye fogrye commented Mar 14, 2024

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

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: thecodingmachine#657
@oprypkhantc
Copy link
Contributor

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: composer/composer#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
Copy link
Contributor

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

@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

As each condition applied to finder stays there each place it is applied should be done on cloned instance to avoid accumulation. Main instance of finder is kept with all the conditions provided during configuring. NS is created only from NSFctory where it's cloned so no need to add same clone inside NS.
@oojacoboo
Copy link
Collaborator

@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.

@oojacoboo oojacoboo merged commit 9fc7f9e into thecodingmachine:master Mar 20, 2024
8 checks passed
faizanakram99 added a commit to faizanakram99/graphqlite that referenced this pull request May 30, 2024
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.

Add support of types from vendor packages
4 participants