Page MenuHomePhabricator

Avoid explicit commit references in composer.json in code meant for public consumption
Closed, ResolvedPublic

Description

The Composer schema spec has this to say about explicit commit references (ie. stuff like "dev-master#2eb0c0978d290a1c45346a1955188929cb4e5db7"):

This feature has severe technical limitations, as the composer.json metadata will still be read from the branch name you specify before the hash. You should therefore only use this as a temporary solution during development to remediate transient issues, until you can switch to tagged releases. The Composer team does not actively support this feature and will not accept bug reports related to it.

Let this sink in: when using an explicit commit reference, the composer.json data (including the autoloading specification, dependencies etc) will be from a different version of the library than the actual files. If the explicitly referenced commit and the current head of master have any difference in functionality-affecting composer.json metadata, the library will be broken and unusable.

Found out about this because MediaWiki 1.31 pins phpstorm-stubs, to which "autoload": { "files": ["PhpStormStubsMap.php"] } has been added yesterday. So now installing MediaWiki 1.31 without --no-dev results in a broken install which immediately fatals because the Composer autoloader registration process tries to execute a file that doesn't exist, causing breakage like this.

So apparently explicit commit references are an antipattern that we should avoid in all our publicly released software.

Event Timeline

Tgr created this task.Jun 27 2019, 7:44 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 27 2019, 7:44 PM
Tgr updated the task description. (Show Details)Jun 27 2019, 7:46 PM

This was first noticed when a TravisCI job failed with

+php maintenance/install.php --dbtype mysql --dbuser root --dbname its_a_mw --dbpath /home/travis/build/SemanticMediaWiki/mw --pass nyan TravisWiki admin --scriptpath /TravisWiki
PHP Warning:  require(/home/travis/build/SemanticMediaWiki/mw/vendor/composer/../jetbrains/phpstorm-stubs/PhpStormStubsMap.php): failed to open stream: No such file or directory in /home/travis/build/SemanticMediaWiki/mw/vendor/composer/autoload_real.php on line 66
Warning: require(/home/travis/build/SemanticMediaWiki/mw/vendor/composer/../jetbrains/phpstorm-stubs/PhpStormStubsMap.php): failed to open stream: No such file or directory in /home/travis/build/SemanticMediaWiki/mw/vendor/composer/autoload_real.php on line 66
PHP Fatal error:  require(): Failed opening required '/home/travis/build/SemanticMediaWiki/mw/vendor/composer/../jetbrains/phpstorm-stubs/PhpStormStubsMap.php' (include_path='.:/home/travis/.phpenv/versions/7.1.11/share/pear') in /home/travis/build/SemanticMediaWiki/mw/vendor/composer/autoload_real.php on line 66
Fatal error: require(): Failed opening required '/home/travis/build/SemanticMediaWiki/mw/vendor/composer/../jetbrains/phpstorm-stubs/PhpStormStubsMap.php' (include_path='.:/home/travis/.phpenv/versions/7.1.11/share/pear') in /home/travis/build/SemanticMediaWiki/mw/vendor/composer/autoload_real.php on line 66
Tgr added a subscriber: Nikerabbit.Jun 27 2019, 7:49 PM

Per codesearch Translatewiki is the only code currently affected (and that's not really publicly released software; still, heads-up, @Nikerabbit). Not sure how to check which past releases are affected.

Ugh, but makes sense :(

We only used phpstorm-stubs for phan purposes, and got rid of it in master. For older releases, I think we should just copy the stubs that we need into tests/phan/stubs and be done with it.

Tgr added a comment.Jun 27 2019, 7:59 PM

composer.json@1b990608 (which is currently pinned in MW 1.31) and composer.json@2018.1.2 are identical apart from dev requirements, so probably the simplest fix for 1.31 is replacing "jetbrains/phpstorm-stubs": "dev-master#1b9906084d6635456fcf3f3a01f0d7d5b99a578a" with "jetbrains/phpstorm-stubs": "2018.1.2#1b9906084d6635456fcf3f3a01f0d7d5b99a578a".

MW 1.32 is also affected; 1.27 isn't.

Reedy added a subscriber: Reedy.Jun 27 2019, 8:00 PM

MW 1.32 is also affected; 1.27 isn't.

1.27 isn't supported ;)

Tgr added a comment.Jun 27 2019, 8:04 PM

1.27 isn't supported ;)

It's supported for the entire week per wiki!
Anyway...

composer.json@1b990608 (which is currently pinned in MW 1.31) and composer.json@2018.1.2 are identical apart from dev requirements, so probably the simplest fix for 1.31 is replacing "jetbrains/phpstorm-stubs": "dev-master#1b9906084d6635456fcf3f3a01f0d7d5b99a578a" with "jetbrains/phpstorm-stubs": "2018.1.2#1b9906084d6635456fcf3f3a01f0d7d5b99a578a".

The docs are confusing but seem to say the #<commit> notation only works for branches, not tags. So either drop the pinning to that specific commit or un-vendorize it as Legoktm said.

Change 519507 had a related patch set uploaded (by markahershberger; owner: markahershberger):
[mediawiki/core@REL1_31] Use a tag instead of a commit hash.

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

composer.json@1b990608 (which is currently pinned in MW 1.31) and composer.json@2018.1.2 are identical apart from dev requirements, so probably the simplest fix for 1.31 is replacing "jetbrains/phpstorm-stubs": "dev-master#1b9906084d6635456fcf3f3a01f0d7d5b99a578a" with "jetbrains/phpstorm-stubs": "2018.1.2#1b9906084d6635456fcf3f3a01f0d7d5b99a578a".
MW 1.32 is also affected; 1.27 isn't.

I didn't realize they started tagging releases now. If phan passes with said tag, let's use it.

Change 519794 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_31] Remove jetbrains/phpstorm-stubs from composer dev dependancies

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

Reedy added a comment.Jun 30 2019, 5:20 PM

Change 519794 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_31] Remove jetbrains/phpstorm-stubs from composer dev dependancies
https://gerrit.wikimedia.org/r/519794

Alternate fix as I've completely disabled mediawiki-core-php72-phan-docker on REL1_3[12] in https://gerrit.wikimedia.org/r/#/c/integration/config/+/519793/ as it was broken due to php version mismatches between the version it was trying to run phan on (7.0) and the 7.2 in the image

Change 519795 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_32] Remove jetbrains/phpstorm-stubs from composer dev dependancies

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

Reedy triaged this task as High priority.Jun 30 2019, 5:37 PM

Noting this is now blocking backports to REL1_3[12], such as T202211

Change 519507 abandoned by Gergő Tisza:
composer: Use more robust method of freezing jetbrains/phpstorm-stubs

Reason:
Done in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/ /519794/

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

Change 519794 merged by jenkins-bot:
[mediawiki/core@REL1_31] Remove jetbrains/phpstorm-stubs from composer dev dependancies

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

Change 519795 merged by jenkins-bot:
[mediawiki/core@REL1_32] Remove jetbrains/phpstorm-stubs from composer dev dependancies

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

Reedy closed this task as Resolved.Jun 30 2019, 9:31 PM
Reedy claimed this task.
Reedy edited projects, added MW-1.32-release; removed Patch-For-Review.

Thanks @Jamesmontalvo3 for originally reporting the issue and @MarkAHershberger for tracking down the exact trigger!

@Reedy should there be a point release?

Thanks @Jamesmontalvo3 for originally reporting the issue and @MarkAHershberger for tracking down the exact trigger!
@Reedy should there be a point release?

No reason why we couldn't, if it's causing wide enough problems we could do a maintenance release pretty soon

Needs a bit of prep work as it seems people haven't been doing RELEASE-NOTES... I'd definitely want to get T202211 merged and into it too (patches are up for review)

To be clear, "explicit commit references" already exist in composer.lock which should be committed to version control:

Committing this file to VC is important because it will cause anyone who sets up the project to use the exact same versions of the dependencies that you are using. Your CI server, production machines, other developers in your team, everything and everyone runs on the same dependencies, which mitigates the potential for bugs affecting only some parts of the deployments. Even if you develop alone, in six months when reinstalling the project you can feel confident the dependencies installed are still working even if your dependencies released many new versions since then. (See note below about using the update command.)

https://getcomposer.org/doc/01-basic-usage.md#commit-your-composer-lock-file-to-version-control

For instance, the way this would work, is you could set jetbrains/phpstorm-stubs to dev-master and the dependency would be pinned to a specific version. That version would be identical for everyone with the composer.lock file until someone updates the library (in which case, composer.lock would be modified).

For all of these reasons, composer.lock should be committed to the repo.

@dbarratt how would that work with composer.local.json? If I add extensions to composer.local.json, I believe I can run composer install to get those extensions without impacting the dependencies from composer.json. But then if I bump the version of one of those extension I need to run composer update to get the new version. That’d invalidate composer.lock I believe.

@dbarratt how would that work with composer.local.json? If I add extensions to composer.local.json, I believe I can run composer install to get those extensions without impacting the dependencies from composer.json. But then if I bump the version of one of those extension I need to run composer update to get the new version. That’d invalidate composer.lock I believe.

In an ideal scenario, MediaWiki would be a dependency of your project and you would have your own composer.json that you could customize however you would like. See T166956