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

adding option to manually register psr0/psr4-like dependency autoloading #67

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Idrinth
Copy link

@Idrinth Idrinth commented Apr 9, 2018

I did stumble upon older versions of libraries(nikic's php-parser in this case), where there is an autoloader of it's own.
This option is meant to help deal with that issue by adding a psr4-like autoloading-definition to packages. It does not change the default behaviour, but might be a workaround for some of #55's issues.

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 9, 2018

This can be avoided by moving the actual reflection logic to BetterReflection, which can already handle autoloading with edge cases and exotic autoloading mechanisms.

As for the patch itself, I rather suggest adding autoloading definitions to your composer.json

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 9, 2018

I understand the reasoning behind this patch, but I disagree with adding more complexity to the tool to handle edge cases.

These should just be exceptions or it should be fixed at lower level by emulating an actual autoloading call (what BetterReflection does)

@Idrinth
Copy link
Author

Idrinth commented Apr 9, 2018

I tried to pick the simplest solution, reworking the whole tool to handle the autoloaders would imo be more work than reasonable to handle the (hopefully rare) cases.
Regarding adding the definitions to the composer.json, I personally dislike adding configuration to files meant to configure something completely different. From my point of view it just makes it harder to understand the software they configure.

@Idrinth
Copy link
Author

Idrinth commented Apr 9, 2018

for me personally I can easily use the fork as repo - should I close this @Ocramius ?

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 9, 2018

@Idrinth I would at least suggest giving this a stab. Yes, it is more work, but in general, fixing the symptom is a problem, while the source of the issue may cause more trouble.

@Idrinth
Copy link
Author

Idrinth commented Apr 9, 2018

Will have a look some time this week.

I propose at least extracting most of the body of the execute method out first to be able to easier test this (likely) complicated piece of code by not having to supply an actual composer file for each test case.

@simara-svatopluk
Copy link

Hi guys, as we are testing this tool, we run into the same issue.
Some dependencies have no autoload or have custom.
And if we are dependent on symfony package, and some other dependency depends on the whole symfony, composer install the whole symfony (not the symfony package only) and the checker tells us that we are missing dependency in composer.json :-( Because the package is not in verdor, but in vendor in symfony

So... are you planning to continue with this PR? It would probably solve all our problems ;-)

@Ocramius
Copy link
Collaborator

@simara-svatopluk give it time: 9 days is peanuts on OSS-scale. If you are in a hurry, I suggest stepping in and helping out.

@Idrinth
Copy link
Author

Idrinth commented Apr 18, 2018

@simara-svatopluk I didn't have as much time as expected to go through reworking the autoload-detection as intended by Ocramius , feel free to just use my fork (branch bandaid) for the time being. Will keep it around for the forseeable future - I usually don't delete my forks.

In any case I intend to do the planned rework, I just don't consider it a quick thing to do, as mentioned before.

Idrinth added a commit to Idrinth/ComposerRequireChecker that referenced this pull request Apr 20, 2018
Idrinth added a commit to Idrinth/ComposerRequireChecker that referenced this pull request Jul 15, 2018
MidnightDesign pushed a commit to MidnightDesign/ComposerRequireChecker that referenced this pull request Sep 17, 2020
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.

3 participants