ComposerRequireChecker icon indicating copy to clipboard operation
ComposerRequireChecker copied to clipboard

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

Open Idrinth opened this issue 7 years ago • 9 comments

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.

Idrinth avatar Apr 09 '18 19:04 Idrinth

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 avatar Apr 09 '18 20:04 Ocramius

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)

Ocramius avatar Apr 09 '18 20:04 Ocramius

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 avatar Apr 09 '18 20:04 Idrinth

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

Idrinth avatar Apr 09 '18 20:04 Idrinth

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

Ocramius avatar Apr 09 '18 21:04 Ocramius

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.

Idrinth avatar Apr 09 '18 21:04 Idrinth

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 ;-)

simara-svatopluk avatar Apr 18 '18 12:04 simara-svatopluk

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

Ocramius avatar Apr 18 '18 12:04 Ocramius

@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 avatar Apr 18 '18 21:04 Idrinth