WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 15 hours ago

#47381 new enhancement

Remove the Composer lock file from version control

Reported by: johnbillion Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-patch
Focuses: coding-standards Cc:

Description

WordPress develop includes a composer.json file in its root which is used to declare developer dependencies. Currently it's only used for WPCS but #46815 proposes using it for PHPUnit too in order to remove the dependency on a globally installed PHPUnit (and #47256 proposes using it for several external dependencies).

The existence of the composer.lock file in the VCS repo prevents PHPUnit from being added as a dependency because its version and its dependencies vary depending on the version of PHP that's in use, meaning the lock file also needs to vary. If the dependencies are installed (and the lock file is created and committed) using PHP 7.1 or higher then a user trying to install the dependencies using PHP 7.0 or lower will not be able to proceed because the PHP version dependency for PHPUnit 7 that's declared in the lock file won't be met.

The solution is to remove the lock file from version control so that any user on any supported PHP version can successfully install the dependencies. This will also allow the separate PHPUnit installation step on Travis CI to be removed.

In addition, composer.lock should be added to .gitignore and svn:ignore.


CCing some interested parties: @netweb @welcher @vinkla @Rarst @ayeshrajans @spacedmonkey @giuseppe.mazzapica

Change History (5)

#1 @ayeshrajans
2 years ago

Thanks for the ping @johnbillion. I had commented on other Trac issues as well, and I still think it's a good idea to simplify the logic in the travis.yml and not install PHPUnit globally.

Removal of composer.lock file is a big step, and we should evaluate the pros and cons.

Reproducible builds

One of the major reasons to have a composer.lock in the first place is to make sure we get the exact same build in two composer install calls. Drupal and Joomla, both similar software as us and adopted Composer throughout the software before us, still keep their lock files in the repo.

With the proposal to use composer to install other dependencies such as sodium_compat, I think it's important to lock down the non-dev dependencies to a specific commit hash and review dependency updates one by one. This adds some friction, but it's worth it for a software that powers 30% of the web :)

Projects such as WordPlate encourage the use of Composer more and more, and I think as the WordPress Core, we should follow what the rest of the industry/community does.

Dev-dependencies

Although I suggest that we lock non-dev dependencies with a lock files, dev dependencies are a different story though.

Our major dependency that we cannot lock down is PHPUnit. The current approach to install a different version globally isn't an elegant solution, but I understand why that workaround is there.

As a first step, perhaps we can use a composer require phpunit/phpunit:^4.0|6.0|^7.0 in the travus.yml file, and let composer pick the appropriate phpunit version depending on the platform.

npm package lock files
In the repo, we have the package-lock.json file present. I think we should be consistent in regards to PHP and JS dependencies.

--

Bottom line is that, I think we should keep the composer.lock file considering the tendency of us using composer dependencies in WordPress such as sodium_compat, polyfills, etc. It's considered a good practice to use lock files in major software, and I think we should follow suit.

Our current phpunit installation approach could use some improvements such as the use if a single composer require phpunit/phpunit:^x|^y|^z command and remove the if/else blocks in the travis.yml file, and also not assume the global installation of dependencies.

This ticket was mentioned in Slack in #cli by johnbillion. View the logs.


2 years ago

#3 @desrosj
20 months ago

  • Milestone changed from 5.3 to Future Release

This still needs discussion and a patch. Going to punt this one.

#4 @jrf
3 weeks ago

  • Focuses coding-standards added
  • Milestone changed from Future Release to 5.8

I'd very much like to revive this ticket as the existence of the committed composer.lock file is - as @johnbillion states quite eloquently above - a blocker for easily testing cross-version PHP.

Reason to have a committed composer.lock file

Let's look at the (valid) reasons for projects to have a committed composer.lock file:

  1. Runtime dependencies - i.e. things in require - which need to be locked to a specific version with which the software has been tested.

This reason does NOT apply to WordPress.
Yes, WordPress has runtime dependencies like Sodium Compat, PHPMailer, Requests and more. However, 1) those are not (currently) managed via Composer and 2) if that would change in the future, those could be locked at a fixed version constraint instead of allowing for a version range.

  1. Contributors only need to worry about their contribution working with one particular set of dependencies.

This reason again does NOT apply to WordPress as all WordPress code needs to work cross-version, so only, for instance running the tests against PHP 7.4 using PHPUnit 7 and sending in a pull request/patch and expecting it to be accepted, even though the tests don't pass on other PHP versions, is not acceptable.

So, having said that, I don't see a valid reason for having a committed composer.lock file.

Consistency with the NPM side of the project, to me, is not an argument as NPM and Composer/PHP are very different beasts and the use of a lock file for either should be evaluated on their own merits in each situation.

Reasons to remove it the committed composer.lock file

  1. Make it easier for contributors to test against multiple different PHP versions locally.
  2. Make it easier to reproduce the builds run on GH Actions locally without having to check in the GH Actions script what hacks are needed to get the builds to run.

Other considerations

You may say, but what about managing things like WordPressCS ? We don't want builds to start failing randomly because WPCS released a new version....
And yes, you are right to bring that up. Tools like WPCS should be fixed to a specific version when we remove the composer.lock file to prevent random build failures on a new release.
We should probably also add a fixed dependency to a specific PHP_CodeSniffer version as otherwise - with WPCS allowing for a wider range of PHPCS versions -, a new PHPCS release may have a similar effect.

In other words, that is something which can be managed via the composer.json file instead of via a lock file and tickets should be opened on a new release of those type of dependencies to do a managed update.

Moving forward

All in all, I'm very much in favour of removing the composer.lock file.

To do this, as far as I can see, a patch would be needed which covers the following:

  • [ ] Remove the composer.lock file.
  • [ ] Add the composer.lock file to .gitignore and svn:ignore
  • [ ] Fix the version constraints for a subset of the dev dependencies - in particularly WPCS and PHPCS - in the composer.json
  • [ ] Remove all "hacks" currently in place in the GH Actions workflow scripts to allow for running things cross-versions.
  • [ ] Make sure there is still a passing build after that 😁

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


15 hours ago

Note: See TracTickets for help on using tickets.