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

Composer does not respect plugin files autoload requirements #10024

Closed
cs278 opened this issue Jul 27, 2021 · 10 comments
Closed

Composer does not respect plugin files autoload requirements #10024

cs278 opened this issue Jul 27, 2021 · 10 comments
Labels
Milestone

Comments

@cs278
Copy link
Contributor

cs278 commented Jul 27, 2021

Composer does not appear to load the files section of the autoload definition for plugins. This appears to hold true for the plugin itself and any dependencies, I expect this might be a design decision but I cannot see it documented anywhere. I've stumbled upon this while investigating a problem with the new releases of symfony/yaml package using PHP 8 functions breaking plugins on PHP 7.4 which led me to symfony/symfony#42280. I've put together a test plugin to demonstrate the problem.

My composer.json:

{
    "repositories": [{
            "type": "git",
            "url": "https://github.com/cs278/composer-include-test-plugin"
        }],
    "require": {
        "cs278/composer-include-test-plugin": "@dev"
    }
}

Output of composer diagnose:

Checking composer.json: WARNING
No license specified, it is recommended to do so. For closed-source software you may use "proprietary" as license.
require.cs278/composer-include-test-plugin : unbound version constraints (@dev) should be avoided
Checking platform settings: OK
Checking git settings: OK
Checking http connectivity to packagist: OK
Checking https connectivity to packagist: OK
Checking github.com rate limit: OK
Checking disk free space: OK
Checking pubkeys: FAIL
Missing pubkey for tags verification
Missing pubkey for dev verification
Run composer self-update --update-keys to set them up
Checking composer version: OK
Composer version: 2.1.5
PHP version: 7.4.21
PHP binary path: /usr/bin/php7.4
OpenSSL version: OpenSSL 1.1.1  11 Sep 2018
cURL version: 7.58.0 libz 1.2.11 ssl OpenSSL/1.1.1
zip: extension present, unzip present, 7-Zip present (7z)

When I run this command:

php -f composer.phar test-plugin -vvv

I get the following output:

Reading ./composer.json (/tmp/chris.smith/tmp.DROuBGKkqm/composer.json)
Loading config file ./composer.json (/tmp/chris.smith/tmp.DROuBGKkqm/composer.json)
Checked CA file /etc/ssl/certs/ca-certificates.crt: valid
Executing command (/tmp/chris.smith/tmp.DROuBGKkqm): git branch -a --no-color --no-abbrev -v
Executing command (/tmp/chris.smith/tmp.DROuBGKkqm): git describe --exact-match --tags
Executing command (CWD): git --version
Executing command (/tmp/chris.smith/tmp.DROuBGKkqm): git log --pretty="%H" -n1 HEAD --no-show-signature
Executing command (/tmp/chris.smith/tmp.DROuBGKkqm): hg branch
Executing command (/tmp/chris.smith/tmp.DROuBGKkqm): fossil branch list
Executing command (/tmp/chris.smith/tmp.DROuBGKkqm): fossil tag list
Executing command (/tmp/chris.smith/tmp.DROuBGKkqm): svn info --xml
Failed to initialize global composer: Composer could not find the config file: /tmp/chris.smith/tmp.DROuBGKkqm/.composer/composer.json

Reading /tmp/chris.smith/tmp.DROuBGKkqm/vendor/composer/installed.json
Loading plugin Cs278\ComposerIncludeTestPlugin\ComposerPlugin (from cs278/composer-include-test-plugin)
Running 2.1.5 (2021-07-23 10:35:47) with PHP 7.4.21 on Linux / 5.4.0-80-generic
PHP/7.4.21
str_contains? NO
bootstrap.php included? NO
PHP Fatal error:  Uncaught Error: Call to undefined function Symfony\Component\Yaml\str_starts_with() in /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php:567
Stack trace:
#0 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php(459): Symfony\Component\Yaml\Inline::evaluateScalar()
#1 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php(85): Symfony\Component\Yaml\Inline::parseMapping()
#2 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Parser.php(384): Symfony\Component\Yaml\Inline::parse()
#3 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Parser.php(97): Symfony\Component\Yaml\Parser->doParse()
#4 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Yaml.php(80): Symfony\Component\Yaml\Parser->parse()
#5 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/cs278/composer-include-test-plugin/src/TestCommand.php(33): Symfony\Component\Yaml\Yaml::parse()
#6 phar:///tmp/chris.smith/tmp.DROuBGKkqm/composer.phar/vendor/symfony/console/Command/Command.php(245): Cs278\ComposerIncludeTestPlugin\TestCommand-> in /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php on line 567

Fatal error: Uncaught Error: Call to undefined function Symfony\Component\Yaml\str_starts_with() in /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php:567
Stack trace:
#0 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php(459): Symfony\Component\Yaml\Inline::evaluateScalar()
#1 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php(85): Symfony\Component\Yaml\Inline::parseMapping()
#2 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Parser.php(384): Symfony\Component\Yaml\Inline::parse()
#3 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Parser.php(97): Symfony\Component\Yaml\Parser->doParse()
#4 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Yaml.php(80): Symfony\Component\Yaml\Parser->parse()
#5 /tmp/chris.smith/tmp.DROuBGKkqm/vendor/cs278/composer-include-test-plugin/src/TestCommand.php(33): Symfony\Component\Yaml\Yaml::parse()
#6 phar:///tmp/chris.smith/tmp.DROuBGKkqm/composer.phar/vendor/symfony/console/Command/Command.php(245): Cs278\ComposerIncludeTestPlugin\TestCommand-> in /tmp/chris.smith/tmp.DROuBGKkqm/vendor/symfony/yaml/Inline.php on line 567

And I expected this to happen:

PHP/7.4.21
str_contains? YES (user-defined)
bootstrap.php included? YES
Yaml::parse? Array
(
    [foo] => bar
)
@helhum
Copy link
Contributor

helhum commented Jul 27, 2021

This happens due to the fact that \Composer\Autoload\AutoloadGenerator::createLoader does create an autoloader, that does not respect include files.

I don't think this can be changed easily, if at all, because this API (which is likely also to be used for custom plugin commands) is meant to be used multiple times within one process and obviously include files can only be included once (at least it isn't documented, that these files must guard for second inclusion).

Pretty sure, that any change in this area will be breaking.

@nicolas-grekas
Copy link
Contributor

Can't using require_once fix the issue?

@helhum
Copy link
Contributor

helhum commented Jul 27, 2021

Can't using require_once fix the issue?

Fix which issue in what case? Considering that Composer plugin packages can be upgraded and are both executed before and after an upgrade, I doubt that a require_once at any point in such process will help to reliably resolve such issues.

@helhum
Copy link
Contributor

helhum commented Jul 27, 2021

I doubt that a require_once at any point in such process will help to reliably resolve such issues.

One practical example:

  • Plugin1 uses PackageClass of Package1 in 5.3.0, which has an include file bootstrap.php defined
  • Package1 adds a global function in bootstrap.php in 5.3.4 and uses this function in PackageClass
  • composer update updates Package1 from 5.3.0 to 5.3.4
  • Plugin1 is initialized before the upgrade, Composer autoloads Package1, which require_once the bootstrap.php
  • Plugin1 is executed after the upgrade, Composer autoloads Package1 again, as things could have been changed, which require_once the bootstrap.php, but this is a noop, as PHP included this file already
  • now using an instance of PackageClass and running into a fatal error as the new code now requires a global function which is not defined.

The plugin system in Composer has improved in Composer 2, so not sure if everything still applies, but the outlines should still be the same. The plugin system already is (because of what it needs to achieve) pretty fragile already. I don't think it will help to add some global state to it.

@cs278
Copy link
Contributor Author

cs278 commented Jul 27, 2021

@helhum as I said in the issue I can see why it was done this way. Likewise I traced the problem to AutoloadGenerator::createLoader, which doesn't document it purposefully ignores the files section which it can receive as input or is it an oversight? 🤷

Even if this cannot be fixed the visibility needs to be improved.

@Seldaek
Copy link
Member

Seldaek commented Jul 28, 2021

Yeah probably should be better documented, but indeed what you figured out above is correct, it's by design and I'm not so keen on changing it. files autoloading was seen as more of a hack initially.. It has since seen more regular usage with some packages providing functions, but it is still quite a problematic autoload method as it is not symbol based like classes. Maybe one day we get proper function autoloading in php 🤞🏻

If someone feels like sending a PR clarifying the plugin docs and/or the docblock on createLoader, those would both be welcome improvements.

@cs278
Copy link
Contributor Author

cs278 commented Jul 29, 2021

Right now plugins cannot safely depend on any packages that use files autoloading, for example no plugin can use nikic/iter for example. There has to be something that can be done to improve on this in Composer, few ideas I've come up with:

  • Autoload the files section when Composer invokes a plugin provided command?
  • Define a new type of polyfill package that Composer will respect the files section for if depended on by a plugin?

Right now ignoring the files section is a simple way for Composer to avoid loading global symbols which break things, but there is nothing stopping global symbols being declared in a file which is autoloaded in another way.

I cannot even figure out a nice workaround for this inside a plugin, unless I'm missing something it doesn't appear possible for a plugin to get it's own PackageInterface object or even the local repository it is installed via.

@hexus
Copy link

hexus commented Aug 18, 2021

I bumped into this today when I required the react/promise library to implement a plugin's compatibility with Composer 1 and Composer 2, but alas, the resolve() function is in a functions.php file autoloaded with files. While I was able to implement a workaround (a dummy Promise-esque object that does the job when running Composer 1), it was certainly a surprise to me.

As much as part of me wishes it "just worked", I understand the reasoning behind not wanting to load arbitrary PHP files this way. While @cs278 makes a good point - there's nothing stopping a class file polluting global scope - at least they can be autoloaded on demand using real PHP autoloaders, and not just require_onced the instant a dependency is installed. This could have a compounding effect if, during Composer's execution, dependencies of dependencies also use files autoloading, for example. There's simply no way to load them on demand.

Another workaround I considered trying was dumping the autoloader and manually loading it during the activation of the plugin ($composer->getAutoloadGenerator()->dump(...)), but this felt all kinds of wrong and would probably have unintended side effects.

@nicolas-grekas
Copy link
Contributor

See #10065

@Seldaek Seldaek modified the milestones: Nice To Have, 2.2 Aug 19, 2021
TomaszGasior added a commit to TomaszGasior/create-symfony-project that referenced this issue Sep 7, 2021
The newest versions of Symfony HTTP client use PHP 8 functions
and require PHP 8 polyfill which does not work in context
of Composer plugin. :(

symfony/symfony#42280
composer/composer#10024
fabpot added a commit to symfony/symfony that referenced this issue Nov 5, 2021
…s (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[Yaml] revert using functions provided by polyfill packages

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #43943
| License       | MIT
| Doc PR        |

This reverts #41431 for the same reason for which we merged #42296 (see #42280 and composer/composer#10024 for more information).

Commits
-------

3b9b700 revert using functions provided by polyfill packages
@Seldaek
Copy link
Member

Seldaek commented Nov 24, 2021

Fixed by #10065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants