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

Does not respect PHP Files installed outside of vendor #182

Closed
DanielSiepmann opened this issue Apr 6, 2020 · 13 comments
Closed

Does not respect PHP Files installed outside of vendor #182

DanielSiepmann opened this issue Apr 6, 2020 · 13 comments

Comments

@DanielSiepmann
Copy link

DanielSiepmann commented Apr 6, 2020

There are some frameworks that store installed packages somewhere outside the vendor folder. E.g. TYPO3 CMS stored their extensions (installed via composer) somewhere else.
Those are reported as missing with default setup:

ComposerRequireChecker 2.1.0@0c66698d487fcb5c66cf07108e2180c818fb2e72
The following unknown symbols were found:
+---------------------------------------------------------+--------------------+
| unknown symbol                                          | guessed dependency |
+---------------------------------------------------------+--------------------+
| TYPO3\CMS\Core\Context\Context                          |                    |
| TYPO3\CMS\Core\Database\Connection                      |                    |
| TYPO3\CMS\Core\Database\Query\QueryBuilder              |                    |
| TYPO3\CMS\Core\Routing\PageArguments                    |                    |
| TYPO3\CMS\Core\Site\Entity\SiteLanguage                 |                    |
| TYPO3\CMS\Core\Site\SiteFinder                          |                    |
| TYPO3\CMS\Core\Utility\ArrayUtility                     |                    |
| TYPO3\CMS\Dashboard\Widgets\AbstractBarChartWidget      |                    |
| TYPO3\CMS\Dashboard\Widgets\AbstractDoughnutChartWidget |                    |
| TYPO3\CMS\Dashboard\Widgets\AbstractListWidget          |                    |
+---------------------------------------------------------+--------------------+

I wonder why the auto generated files vendor/composer/autoload_*.php are not used to find all files. That should find the files as these are generated with the concrete installation path.

Example of vendor/composer/autoload_psr4.php with some TYPO3 extensions (concrete paths can be different depending on options in composer.json):

<?php

// autoload_psr4.php @generated by Composer

$vendorDir = dirname(dirname(__FILE__));
$baseDir = dirname($vendorDir);

return array(
    'phpDocumentor\\Reflection\\' => array($vendorDir . '/phpdocumentor/reflection-common/src', $vendorDir . '/phpdocumentor/reflection-docblock/src', $vendorDir . '/phpdocumentor/type-resolver/src'),
    // ...
    'TYPO3\\CMS\\Recordlist\\' => array($baseDir . '/.Build/web/typo3/sysext/recordlist/Classes'),
    'TYPO3\\CMS\\Frontend\\' => array($baseDir . '/.Build/web/typo3/sysext/frontend/Classes'),
    'TYPO3\\CMS\\Fluid\\' => array($baseDir . '/.Build/web/typo3/sysext/fluid/Classes'),
    'TYPO3\\CMS\\Extbase\\' => array($baseDir . '/.Build/web/typo3/sysext/extbase/Classes'),
    'TYPO3\\CMS\\Dashboard\\' => array($baseDir . '/.Build/web/typo3/sysext/dashboard/Classes'),
    'TYPO3\\CMS\\Core\\' => array($baseDir . '/.Build/web/typo3/sysext/core/Classes'),
    'TYPO3\\CMS\\Backend\\' => array($baseDir . '/.Build/web/typo3/sysext/backend/Classes'),
   // ...

Is there any chance to chance the detection of files to parse in future? Or is there an official way to support these situations?

I guess the issue should be consistent between all those libraries that use an "own installer": https://github.com/composer/installers/tree/master/src/Composer/Installers.
Same situation might happen for some projects like code sniffer and others that have their own custom installers, that move vendor code to some other folders.

As far as I can see, right now the following line should be adjusted:

$vendorDirs[$vendorName] = $packageDir . '/' . $configVendorDir . '/' . $vendorName;

I'm not experienced yet to give any hints how to retrieve the expected folder by an API.

@DanielSiepmann
Copy link
Author

foreach (array_keys($composerJson['require'] ?? []) as $vendorName) {
    $vendorDirs[$vendorName] = $this->getInstallDir($packageDir, $configVendorDir, $vendorName);
}
private function getInstallDir(string $packageDir, string $configVendorDir, string $vendorName): string
{                                          
    $output = [];       
    exec('composer info ' . $vendorName . ' 2>&1', $output);                                           
    foreach ($output as $line) {
        if (strpos($line, 'path     :') === 0) {
            return str_replace('path     : ', '', $line);
        }
    }

    return $packageDir . '/' . $configVendorDir . '/' . $vendorName;
}

Would work, but of course isn't a solution.

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 6, 2020

This is expected behavior: composer.json, composer.lock and installed.json are the authoritative sources of truth for where files are located on the filesystem, and relying on generated code (including Composer's) is not feasible due to no BC guarantees on the format or syntax of those assets.

I would suggest putting effort in adding autoloading support for custom paths in https://github.com/Roave/BetterReflection/tree/abb9c16692707c607464d028436bce857926395c/src/SourceLocator/Type/Composer/Factory, and then backporting it here (#55)

@SvenRtbg
Copy link
Contributor

SvenRtbg commented Apr 6, 2020

I'm still struggling to understand the issue here. I added the code that respects the vendor-dir setting, and would be rather surprised if Composer invented a way for the package to define where to be installed.

The tool is scanning every directory that is mentioned in the composer.json unter autoload for own code, and is using the vendor directory (which may be changed, see above) together with the list of explicitly required packages for external code. It then parses them and compares if each used symbol in the own code can be resolved by the code base.

If TYPO3 delivers more code via anything beyond git clone && composer install, I would define this as out of scope for this tool. You can configure additional scan directories to customize your experience, though. https://github.com/maglnet/ComposerRequireChecker#scan-additional-files

@DanielSiepmann
Copy link
Author

@SvenRtbg composer allows composer installers which handle the installation and could move things around. The above links to the packages provide an handy overview of supported frameworks that use this feature.
Therefore vendor files might be somewhere else then in configured vendor folder. Most probably due to older conventions from times before composer.

TYPO3 has the concept of extensions, just like Symfony bundles. But for historic reasons the composer packages that define extensions are installed somewhere else. Same is true for other mentioned frameworks and tools.

@Ocramius I understand and sounds reasonable. Using the internal API of composer itself looks very complicated, even if it would provide the necessary info.

@helhum
Copy link

helhum commented Apr 6, 2020

and would be rather surprised if Composer invented a way for the package to define where to be installed.

Composer is extensible via Plugin API A plugin can, besides a bunch of other things, register an installer for certain package types.

In fact, Jordi is maintaining composer/installers, which adds installers and even makes installation path configurable for individual packages. composer/installers has a download count of currently over 26 million installs. So even with not considering TYPO3 specialities, I would't say custom install paths for packages are something totally niche.

tl;dr: there is a way to tell Composer where a package should be installed :)

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 6, 2020

tl;dr: there is a way to tell Composer where a package should be installed :)

There must also be a way to have a deterministic API (or declaration) of where that installation is, and what its structure is like.

BTW, highly endorse getting rid of this custom stuff in Typo3: it is needless complexity that only weighs on tooling (any tooling) upstream, and shouldn't be implemented unless it goes well over the 80/20 use-case split.

@DanielSiepmann
Copy link
Author

As mentioned before, this is not specific to TYPO3. There are some more systems making use of different installation paths, beside some libraries as well, like linters.

There is an API, like the composer info <packagename> which provides the actual path.
But the internal composer code looks quite complex to retrieve these information and I didn't invest more time finding out how it is determined. Maybe I can invest some more time to find a clean way to ask composer to provide the installed path for an package, which would solve the issue.

@Ocramius
Copy link
Collaborator

Ocramius commented Apr 6, 2020

I'm aware that drupal also does this sort of stuff. The endorsement remains the same: don't do it.

@SvenRtbg
Copy link
Contributor

SvenRtbg commented Apr 6, 2020

I'm with @Ocramius here: This world would be a better place without custom installation paths.

I've been reading the code of some of the projects linked above, and I come to the conclusion that this all is a mess. No necessarily for users of said installers, but from the perspective of a tool like this one here.

The only solution to allow self service here looks like extending that "scan-files" configuration option. Currently that information is added to the "own code" section. If you add vendor code, it'll be treated as your own code, i.e. it will add symbols that have to be required instead of just adding symbols that help resolve what "own code" uses.

So another, hypothetical "scan-vendor-files" option. But then? Assuming that TYPO3 for example only uses one additional directory for everything, and adding that main directory does not allow to determine which parts were directly required, and which ones are indirect dependencies, it would mean that the tool user has to explicitly add all the correct paths of stuff that is directly required, and omit or filter out everything else that isn't.

Composer itself does not have a proper API for the information required to satisfy what this tool needs. The output of some shell execution is no API that a tool can rely on: Formatting will change, parameters may change... let's try to not go that route.

Has anyone tried to first do composer install --no-plugins, then do the scan? I'd assume that this would force all packages to be put into vendor, and then be available for proper scanning. This will not create a useful installation, but will satisfy the scanner.

@helhum
Copy link

helhum commented Apr 7, 2020

The endorsement remains the same: don't do it
This world would be a better place without custom installation paths.

I fully support these statements. I also understand being reluctant to change things that would need change in a better world.

I cannot tell for other projects than TYPO3, but we are definitely working hard since quite some time to make the custom install paths obsolete. But we're not there yet. Nontheless tools like this one here would be of great help to support us in achieving this goal.

So the pledge here shouldn't be to change the tool to allow for a messier world, but adapt the tool to help making the world a better place.

Anyway. I don't think much (if any at all) work is required to be done on your side. It is perfectly possible to develop an "adapter package" that deals with specialities of individual projects and your suggestion to just do a composer install --no-plugins indeed works nicely and is easy enough to do.

I'd rather close this issue here.

@SvenRtbg
Copy link
Contributor

SvenRtbg commented Apr 7, 2020

Thanks for the feedback. The easiest result to be implemented is to add my suggestion to the documentation, which I'll do right away.

SvenRtbg added a commit to SvenRtbg/ComposerRequireChecker that referenced this issue Apr 7, 2020
@DanielSiepmann
Copy link
Author

Thanks for the discussion and working workaround for CI environments 👍

@SvenRtbg
Copy link
Contributor

SvenRtbg commented Apr 7, 2020

Seems like I missed to click a button, but here we are now with the readme edit.

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

No branches or pull requests

4 participants