Opened 6 years ago
Closed 4 years ago
#47381 closed enhancement (fixed)
Remove the Composer lock file from version control
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch has-unit-tests commit |
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 (18)
This ticket was mentioned in βSlack in #cli by johnbillion. βView the logs.
6 years ago
#3
@
6 years ago
- Milestone changed from 5.3 to Future Release
This still needs discussion and a patch. Going to punt this one.
#4
@
4 years 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:
- 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.
- 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
- Make it easier for contributors to test against multiple different PHP versions locally.
- 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
andsvn:ignore
- [ ] Fix the version constraints for a subset of the
dev
dependencies - in particularly WPCS and PHPCS - in thecomposer.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.
4 years ago
#7
@
4 years ago
- Milestone changed from Future Release to 5.9
- Owner set to hellofromTonya
- Status changed from new to assigned
Moving this ticket into the 5.9 milestone as it's needed to unblock adding PHPUnit Polyfills (happening in ticket #46149).
This ticket was mentioned in βPR #1511 on βWordPress/wordpress-develop by βjrfnl.
4 years ago
#8
- Keywords has-patch has-unit-tests added; needs-patch removed
## Composer: remove lock file (trac 47381)
As per the comment I left in the actual Trac ticket, there is currently no reason to have a composer.lock
file as:
- External runtime dependencies are not managed via Composer.
- Managed updates of the non-runtime dependencies can be done by locking the version used in the
composer.json
file to a precise version instead of using acomposer.lock
file. - And having the
composer.lock
file in place makes it a lot more difficult to run the tests against all supported PHP versions.
With that in mind, I'm now removing the lock
file and adding it to .gitignore
.
π Important: Whoever commits this change should make sure it is also added to .svnignore
!
I've reviewed the current dev dependencies, the version constraints of those and the broader spectrum of what we know about these projects. Please find a summary for each below.
### PHPUnit
- The existing version constraint was incorrect as WP currently supports running the tests on both PHPUnit 5.x (for PHP 5.6 and 7.0), as well as 7.x (for PHP 7.1-7.4).
- The version used for PHPUnit was locked at
7.5.20
, which:- Makes it more difficult for contributors to test changes locally on anything but PHP 7.1-7.4.
- Necesitates all sorts of work-arounds in the CI unit test scripts to get the tests running on other PHP versions.
- As both PHPUnit 5.x as well as 7.x are no longer supported and no new versions within those majors will ever be released anymore, we don't have to be concerned about minor/patch updates.
- Removing the
lock
file has no impact for the 7.x version as WP is already using the latest version7.5.20
, but will allow for runningcomposer install
/composer update
on PHP 5.6-7.0 and getting the right PHPUnit version installed. - Running
composer install
/composer update
on PHP 8.0 will still require the--ignore-platform-reqs
addition until more recent PHPUnit versions will be supported. This is being addresses in Trac ticket 46149. - Current work-arounds in CI can now be removed and CI can use Composer to install the appropriate PHPUnit version for test runs.
### WordPress Coding Standards
- The project uses SemVer and adheres to it within the context of a coding standard, i.e. new features (sniffs) can be introduced in minors, but not in patch versions.
- Updates to new patch versions of this plugin will, generally speaking, not have a significant impact on the developer workflow, so can be safely allowed.
These are mostly small tweaks and bugfixes.
- Removing the
lock
file has no impact as WP is already using the latest version2.3.0
. - With the current version constraint, updates to new minors as well as majors will need to be specifically managed, which is a good thing as updates may require new "ignore annotations" to be added, allow for existing ones to be removed or for the ruleset to be adjusted.
- The next version is expected to be a major,
3.0.0
and will include additional external dependencies.
Once WPCS 3.0.0 has been released, we can re-evaluate whether the version constraint should be locked at a specific version or should allow for patch updates automatically.
- Side-note: I am one of the maintainers of this tool.
Also see: βhttps://github.com/WordPress/WordPress-Coding-Standards/releases
### PHPCompatibility WP
Note: this is an exclusion ruleset for the underlying PHPCompatibility library, so the impact of updates to either needs to be considered.
#### PHPCompatibilityWP
- The project allows the full PHPCompatibility
9.*
range under the hood. More about this below. - Updates to new patch versions of this plugin will generally speaking have a positive impact on the developer workflow, as it means that new PHP features polyfilled by WP will no longer be flagged by the tool.
- Removing the
lock
file effectively updates the dependency from version2.1.0
to2.1.2
with the positive impact that a few new constants polyfilled in WP 5.8 are now accounted for (excluded from being flagged). - Removing the
lock
file effectively updates an underlying dependency - PHPCompatibilityParagonie, which is also an exclusion ruleset - from version1.3.0
to1.3.1
without impact. - With the current version constraint, updates to new minors as well as majors will need to be specifically managed, which is a good thing as updates may require new "ignore annotations" to be added, allow for existing ones to be removed or for the ruleset to be adjusted.
- The next version is expected to be a major,
3.0.0
, and will include additional external dependencies and will coincide with the release of PHPCompatibility10.0.0
. - Side-note: I am one of the maintainers of this tool.
Also see:
- βhttps://github.com/PHPCompatibility/PHPCompatibilityWP/releases
- βhttps://github.com/PHPCompatibility/PHPCompatibilityParagonie/releases
#### PHPCompatibility
- The project uses SemVer and adheres to it within the context of a coding standard, i.e. new features (sniffs) can be introduced in minors, but not in patch versions.
- Any and all updates to this tool _may_ impact the developer workflow as new things may get flagged.
While this may be inconvenient when this happens unexpectedly, IMO it is still a good thing as any php cross-version incompatibilities in the WordPress code base should be fixed as soon as possible.
This is different to WPCS in that new things flagged by WPCS will generally not impact end-users, while new things flagged by PHPCompatibility will impact end-users.
- It _could_ be considered to add this dependency explicitly to the
composer.json
file to manage updates to new versions with more control, but considering the end-user impact of things being flagged and the risk of creating version constraint conflicts with the "parent" packagePHPCompatibilityWP
and/or with WPCS, I would recommend against this. - Removing the
lock
file has no impact as WP is already using the latest version9.3.5
. - The next version is expected to be a major,
10.0.0
and will include additional external dependencies.
Once version 10.0.0 has been released, we can re-evaluate whether the package should be added to the
composer.json
file explicitly.
- Side-note: I am one of the maintainers of this tool.
Also see: βhttps://github.com/PHPCompatibility/PHPCompatibility/releases/
### PHP_CodeSniffer
- Until now, this was not an explicit dependency.
- PHPCS is an underlying dependency for both WPCS as well as PHPCompatibility, which both only set a minimum, but no maximum, supported version within PHPCS majors as their version constraint for it.
- The project uses a variant to SemVer and changes in PHPCS can have a big impact on the results of both WPCS (included sniffs from PHPCS changing behaviour) as well as PHPCompatibility (tokenizer changes).
- Depending on what gets committed first, removing the
lock
file either has no impact or effectively updates the dependency from version3.5.5
to3.6.0
without impact (as per Trac 53477). - All the same, as updates to PHPCS can have a significant impact on two of the developer workflows, I'd recommend making updates to this tool managed and have added it as an explicit dependency with a fixed version constraint to the
composer.json
file for that reason. - Whenever the WPCS and/or PHPCompatibility dependency will be updated in the future, the version constraint for PHP_CodeSniffer should also be re-evaluated and possibly updated.
- Side-note: I am one the most active outside contributor to this tool.
Also see: βhttps://github.com/squizlabs/php_codesniffer/releases
### The DealerDirect Composer plugin
- The project uses SemVer and adheres to it.
- Updates to new patch versions of this plugin will not have a significant impact on the developer workflow, so can be safely allowed.
- Removing the
lock
file effectively updates the dependency from version0.7.0
to0.7.1
without impact. - With the current version constraint, updates to new minors -
0.8.0
or1.0.0
- will need to be specifically allowed (as Composer treats minors below 1.0.0 as majors). - As of WPCS 3.0.0/PHPCompatibilityWP 3.0.0, this dependency will no longer be needed as both WPCS as well as PHPCompatibilityWP will already require it and manage the supported versions.
Once either these majors is released and WP updates to it, the explicit dependency for this tool in the
composer.json
file should be removed.
- Side-note: I am one of the maintainers of this tool.
Also see: βhttps://github.com/Dealerdirect/phpcodesniffer-composer-installer/releases
## Build/Test: use a custom autoloader for the PHPUnit 9.x mock object classes
... to prevent those being loaded automatically via the autoload-dev
directives when a Composer installed PHPUnit 5.x version is used, as that would break the test run.
It is expected that this autoloader will be removed soon, as it should no longer be needed when the PHPUnit version constraints are widened.
Notes:
- The autoloader file will be loaded from the Test bootstrap.
- The autoloader will always be registered and directed to queue itself _before_ the Composer autoload file (which will already have been registered).
- The autoloader will only actually load the WP copies of the files/classes when PHP 8.0 in combination with PHPUnit 7.x is detected. In all other cases, the autoloader will bow out, which effectively then defers to the Composer autoload file to load the files as shipped with the installed PHPUnit version.
## WIP/ CI changes
_No description yet_ I have asked @desrosj for a second opinion and will finalize the commit after that.
Trac ticket: https://core.trac.wordpress.org/ticket/47381
βaristath commented on βPR #1511:
4 years ago
#9
I'm not familiar with the GHA caching options mentioned above, however, it looks like a reasonable request, the code makes sense, and testing the PR locally nothing breaks. :+1:
βjrfnl commented on βPR #1511:
4 years ago
#10
After today's additional feedback from @desrosj and having discussed this ticket in the βlivestreamed mob programming session, this PR is now ready for commit.
#11
@
4 years ago
- Keywords commit added
@SergeyBiryukov The GH PR has been updated and is now ready for commit. Would you like me to generate patch files for each of the commits ? Or one patch file containing all three commits ?
I have opened a separate ticket to further investigate switching out the Composer caching workflow steps for the predefined Composer caching action. See #53841
That ticket can be seen as a future iteration on the current changes, and is non-blocking.
βjrfnl commented on βPR #1511:
4 years ago
#15
This ticket was mentioned in βSlack in #core by sergey. βView the logs.
4 years ago
#17
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening the ticket to allow for (potentially) backporting [51543] and [51545] to a select number of older WP branches in combination with a select group of the commits which will be made for #46149.
This backport is intended to make it easier for future security backports, which are bundled with tests, to be backported without having to adjust those patches for supported PHPUnit versions.
This backport will also make it easier for plugins and themes, which have integration tests based on the WP test suite, to still run their test suite in combination with multiple WP versions.
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 twocomposer 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 thetravus.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 assodium_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 thetravis.yml
file, and also not assume the global installation of dependencies.