Page MenuHomePhabricator

Composer v1.1.0 generated vendor dirs will fail lint by PHP <5.6
Closed, ResolvedPublic

Description

Composer v1.1.0 generates a optimized loader file at composer/autoload_static.php for use by PHP versions >=5.6 that includes syntax which is invalid under PHP <5.6 (expressions in static initialization). I originally filed this as a defect upstream with Composer (https://github.com/composer/composer/issues/5324), but on further inspection it turns out that their code is fine. They conditionally require the autoload_static.php file only when PHP_VERSION_ID indicates it is safe to do so.

The problem for MediaWiki/Wikimedia is that:

  • This file will not pass lint checks by PHP 5.3 or PHP 5.5 on our CI infrastructure.
  • If we exclude the file from mediawiki/vendor.git using a .gitignore entry then mediawiki/vendor.git will become unusable for PHP >= 5.6.

Downgrading Composer to prepare patch

composer self-update 1.0.3 will install the last Composer 1.0.x release. composer self-update --rollback can be used after preparing a mediawiki/vendor.git patch to return to a more modern version of Composer on your local host.

See also summary below by @hashar T135161#3053247

Event Timeline

bd808 created this task.May 12 2016, 5:58 PM
Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 12 2016, 5:58 PM
bd808 added a comment.May 12 2016, 6:05 PM

The work around for this for now is to only use Composer v1.0.0 to generate mediawiki/vendor.git patches.

@bd808 yep we found that problem too, we updated ci to 1.1.0 but it failed wikibase I think as @JanZerebecki found out and reverted it to 1.0.0.

We should ignore that file in phplint from the composer version we use and also the one we use for users that aren't whitelisted yet.

bd808 added a comment.May 12 2016, 7:40 PM

We should ignore that file in phplint from the composer version we use and also the one we use for users that aren't whitelisted yet.

It would be nice to find some more general solution to the lint exclusion problem. Besides our CI tests this problem will effect use of scap sync-dir for a branch's vendor directory and for any extension like Wikidata that also bundles its own Composer managed files.

@bd808 yep. I also thought it was a bug until they pointed out that only php 5.6 will use it and then I realised that we had to ignore it in phplint as @JanZerebecki said. But it seems that is breaking a lot of things doing that. Unless we ask upstream to add

// @codingStandardsIgnoreStart

at the top of the file.

bd808 added a comment.May 12 2016, 7:49 PM

Unless we ask upstream to add

// @codingStandardsIgnoreStart

at the top of the file.

That would be an exclusion for phpcs. It won't do anything to change the behavior of php -l nor as far as I know for the parallel lint runner used in many composer test invocations.

Maybe get upstream to make it a config so that way it doesn't break users workflow like this. Would help us until we migrate to php 5.6 which is a long while to wait.

Maybe we can convince upstream to generate code differently according to the PHP version present or declared in config.platform?

Change 289434 had a related patch set uploaded (by BryanDavis):
Rebuild with composer 1.0.0 for PHP >=5.6 users

https://gerrit.wikimedia.org/r/289434

bd808 added a comment.May 18 2016, 3:52 PM

One course of action would be to work with upstream to introduce a configuration option that we can set in our mediawiki/vendor/composer.json to disable generation of the PHP >=5.6 specific code in autoload_real.php. This is going to take some selling upstream because they are against the concept of committing vendor content to version control in the first place.

Our other option as noted in T135161#2290472 would be to adjust our CI lint tests to ignore this file. That change may require changes to scap as well.

@bd808 yep, but we need to have a valid reason according to https://github.com/composer/composer/issues/5354

Why should config.platform not be used to also control the autoloader code generation?

Paladox added a comment.EditedMay 18 2016, 4:49 PM

@JanZerebecki: We could do that too. But we need to get upstream to agree otherwise they wont since they have to maintain it.

Change 289434 merged by jenkins-bot:
Rebuild with composer 1.0.0 for PHP >=5.6 users

https://gerrit.wikimedia.org/r/289434

Change 289541 had a related patch set uploaded (by Paladox):
Rebuild with composer 1.0.0 for PHP >=5.6 users

https://gerrit.wikimedia.org/r/289541

bd808 added a comment.May 23 2016, 3:55 PM

Here's the current state as far as I understand it:

  • Composer 1.1.0 introduces a conditionally loaded file which is only compatible with PHP >=5.6 (i.e. 5.6.x and 7.x).
  • Lint tasks in our CI chain and scap will break when they encounter this file.
  • Excluding the vendor/composer/autoload_static.php file from our version control will break using the effected vendor directory with PHP >=5.6. (T135635)
  • Upstream is not interested in supporting a configuration option that would prevent this file from being generated.

The current workaround is to ensure that only Composer 1.0.x versions are used to generate vendor directories that are committed to version control. You can force your local Composer install to a working version with composer self-update 1.0.3.

Getting stuck on Composer 1.0.x long term will be a problem. At some point there will be a security or performance enhancement that we need. Forking Composer is not a reasonable solution.

Teaching the lint tasks in our CI and scap tool chains to ignore some set of blacklisted files is a possible solution. The jakub-onderka/php-parallel-lint library used in most (all?) CI jobs can be given an --exclude <dir> option to exclude a directory from lint processing. MediaWiki core already uses this option to exclude the vendor directory entirely from lint. For the mediawiki/vendor.git repository we could easily exclude the composer directory. Similar changes could be made for other local repositories such as wikidata.

Fixing the linter built into Scap will take a bit more work. There is currently no file blacklist mechanism in scap for the lint process which is run as a component of scap sync-dir (lints target dir), scap sync-file (lints target file), and scap sync (lints wmf-config and multiversion). scap sync-dir and scap sync use a shared function (scap.tasks.check_valid_syntax) to perform linting. scap sync-file uses php -l directly.

Changes for the CI jobs should not be done until there is a solution for Scap. Getting the repositories into a state where they can not be readily synced on the beta and production clusters is a real possibility and an operational danger.

Change 291682 had a related patch set uploaded (by BryanDavis):
Remove orphan PHP 5.6 static file autoload file

https://gerrit.wikimedia.org/r/291682

Change 291682 merged by jenkins-bot:
Remove orphan PHP 5.6 static file autoload file

https://gerrit.wikimedia.org/r/291682

Change 289541 abandoned by Reedy:
Rebuild with composer 1.0.0 for PHP >=5.6 users

https://gerrit.wikimedia.org/r/289541

Change 296250 had a related patch set uploaded (by Paladox):
Add composer/autoload_static.php to .gitignore

https://gerrit.wikimedia.org/r/296250

Change 296250 abandoned by Paladox:
Add composer/autoload_static.php to .gitignore

https://gerrit.wikimedia.org/r/296250

bd808 updated the task description. (Show Details)Jul 7 2016, 7:15 PM

I submitted https://github.com/composer/composer/pull/5660 to add a option to disable the static autoloader upstream, and there is some discussion happening there as well.

Reedy added a subscriber: Reedy.Nov 6 2016, 5:01 PM

I just filed https://github.com/JakubOnderka/PHP-Parallel-Lint/issues/83 requesting an exclude-file/making exclude support filenames...

Change 320098 had a related patch set uploaded (by Paladox):
Add a script so we can disable loading the new autoload_static file

https://gerrit.wikimedia.org/r/320098

Change 320098 abandoned by Paladox:
Add a script so we can disable loading the new autoload_static file

Reason:
Yes but some of our linters doint support it expecially php -l.

https://gerrit.wikimedia.org/r/320098

Reedy added a comment.Nov 9 2016, 5:15 PM

I just filed https://github.com/JakubOnderka/PHP-Parallel-Lint/issues/83 requesting an exclude-file/making exclude support filenames...

Yup, exclude actually excludes files, but doesn't say it

reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/vendor$ php ~/PHP-Parallel-Lint/parallel-lint.php composer
PHP 7.0.8 | 10 parallel jobs
................                                             16/16 (100 %)


Checked 16 files in 0.4 seconds
No syntax error found
reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki/vendor$ php ~/PHP-Parallel-Lint/parallel-lint.php --exclude composer/autoload_static.php composer
PHP 7.0.8 | 10 parallel jobs
...............                                              15/15 (100 %)


Checked 15 files in 0.3 seconds
No syntax error found
Reedy updated the task description. (Show Details)Nov 9 2016, 5:41 PM
aude added a subscriber: aude.Feb 22 2017, 1:23 PM

Given it's been almost a year since this problem started, this should be documented in mediawiki/vendor README. I still get surprised by this whenever I make a vendor commit. E.g. https://gerrit.wikimedia.org/r/#/c/313682/

Change 339645 had a related patch set uploaded (by Hashar):
Update composer to 1.1.0

https://gerrit.wikimedia.org/r/339645

hashar added a subscriber: hashar.Feb 24 2017, 4:20 PM

Summary

Upstream failing commit is apparently fd2f51cea8 included since 1.1.0-RC.

src/Composer/Util/Filesystem.php has a method findShortestPathCode() which has a bool parameter $staticCode = false which can be used to use DIR instead of dirname():

src/Composer/Util/Filesystem.php
public function findShortestPathCode($from, $to, $directories = false, $staticCode = false)
{
...
    if ($staticCode) {
        $commonPathCode = "__DIR__ . '".str_repeat('/..', $sourcePathDepth)."'";
    } else {
        $commonPathCode = str_repeat('dirname(', $sourcePathDepth).'__DIR__'.str_repeat(')', $sourcePathDepth);
    }

But on PHP 5.5 you can not concatenate strings in a static property:

<?php
  class MyFault {
     public static $foo = array( 'a' . 'b' );
  }

That got filled by @bd808 as https://github.com/composer/composer/issues/5324 eventually abandoned because apparently the faulty autoload_static.php is only used for PHP 5.6+:

I see that this use of this file is guarded in composer/autoload_real.php with a check for PHP_VERSION_ID >= 50600. The problem then is just one of the MediaWiki lint checks. Sorry for the noise.

Then:

I submitted https://github.com/composer/composer/pull/5660 to add a option to disable the static autoloader upstream, and there is some discussion happening there as well.

Which eventually got rejected on the basis that although the generated vendor/composer/autoload_static.php is invalid in PHP5.5 the real autoloader does not include it:

vendor/composer/autoload_real.php
$useStaticLoader = PHP_VERSION_ID >= 50600 && !defined('HHVM_VERSION');
if ($useStaticLoader) {
    require_once __DIR__ . '/autoload_static.php';

Else it fall back to autoload_classmap.php

To follow upstream point of not linting every single files in /vendor/ we would want @Reedy proposal to PHP-Parallel-Lint so we can exclude files when linting.

hashar updated the task description. (Show Details)Feb 24 2017, 4:24 PM
Reedy added a comment.Feb 24 2017, 4:35 PM

To follow upstream point of not linting every single files in /vendor/ we would want @Reedy proposal to PHP-Parallel-Lint so we can exclude files when linting.

If we can work out where to tack on the exclude for autoload-static.php in the integration config... This should be simple to do

@Reedy that should be easy to do, here's the mw core one i did https://gerrit.wikimedia.org/r/#/c/339666/

@Reedy that should be easy to do, here's the mw core one i did https://gerrit.wikimedia.org/r/#/c/339666/

Eh? I see no sign of any specific exclusion

@Reedy --exclude vendor

it's not in that patch, mediawiki core defines phplint and phpcs. In composer we do --exclude vendor for phplint.

So I don't quite know how to fix it :/ mediawiki/vendor triggers a php55lint job which runs php -l against any file changed in HEAD.

I thought about using jakub-onderka/php-parallel-lint which lets one easily add --exclude instruction, we can then php l everything and forget about just running against HEAD.

So in theory it is just about adding in mediawiki/vendor composer.json:

"require-dev": {
        "jakub-onderka/php-parallel-lint": "0.9.2"
},
"scripts": {
        "test": "parallel-lint --exclude jakub-onderka/php-parallel-lint --exclude composer/autoload_static.php ."
}

Then update the composer.lock but not the autoloaders nor commit that dev dependency.

In CI we would trigger composer install (not update) and then composer test which should do the right thing. Thoughts?

Change 340238 had a related patch set uploaded (by Hashar; owner: Hashar):
(WIP) build: jakub-onderka/php-parallel-lint

https://gerrit.wikimedia.org/r/340238

Change 340241 had a related patch set uploaded (by Hashar; owner: Hashar):
Experimental composer tests for mediawiki/vendor

https://gerrit.wikimedia.org/r/340241

Legoktm closed this task as Resolved.
Legoktm claimed this task.

I'm marking this as resolved since https://gerrit.wikimedia.org/r/#/c/339599/ now passes CI tests and should also pass scap.

@Legoktm I'm wondering do extensions still pass? Since won't phplint fail because it will be looking in vendor? I actually mean the phplint-php55 test

Change 340241 merged by jenkins-bot:
Experimental composer tests for mediawiki/vendor

https://gerrit.wikimedia.org/r/340241

@Legoktm I'm wondering do extensions still pass? Since won't phplint fail because it will be looking in vendor? I actually mean the phplint-php55 test

php55lint only acts against file changed in HEAD, and afaik none of the extensions commit their vendor. If it happens to be the case, we can drop the job and migrate them to use composer test --> PHP-Parallel-Lint

Essentially: it is not an issue.

Change 340238 merged by jenkins-bot:
[mediawiki/vendor] build: jakub-onderka/php-parallel-lint

https://gerrit.wikimedia.org/r/340238

Change 384002 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] mediawiki/vendor switch to composer test

https://gerrit.wikimedia.org/r/384002

Change 384002 merged by jenkins-bot:
[integration/config@master] mediawiki/vendor switch to composer test

https://gerrit.wikimedia.org/r/384002