Rules composer.lock file is out of date and we need to refresh it or remove it.

Composer.json no longer has some development dependencies, removed in #3043884: Composer require failure and consequently the composer.lock file no longer matches the composer.json requirements. Hence composer.lock is locking us in to old versions of components, for example it has

"name": "drupal/coder",
"version": "8.2.12",

but there is now a 8.3 branch of Coder. It also has:

"squizlabs/php_codesniffer",
"version": "2.9.1"

whereas there is now a version 3 branch which is used on drupal.org. In testing on 8.8 we initially get codesniffer 3.4.1 and coder 8.3.1 then codesniffer is downgraded to 2.9.1 by composer install and coder is downgraded to 8.2.12

If composer.lock is deleted then there will be some follow-up work required to get Travis build testing running, because PHPunit and PHPCS will need to run slightly differently.

CommentFileSizeAuthor
#5 3106691-5.remove_composer_lock.patch14.58 KBjonathan1055
#2 3106691-2.remove_composer_lock.patch9.79 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 3106691-2.remove_composer_lock.patch9.79 KB

To see what happens with the testbot when composer.lock is removed.

jonathan1055’s picture

jonathan1055 commented

Of course, composer.lock is not used in drupal.org testing so the fact that we have a green pass does not actually mean a great deal. If TR confirms that deleting rather than updating composer.lock is the way to go then I'll work on the .travis.yml changes too.

tr’s picture

tr commented

Rules core does not depend on any external composer packages. It is only projects built with Rules that may require external dependencies and may want to / have to impose version requirements on those dependencies. I believe that composer.lock is currently in Rules because @fago was maintaining an external CI set up to do his own code sniffing and testing. It's that specific project that benefits from a composer.lock, but the Rules module itself doesn't need or use composer.lock. I don't think we should force dependencies that are not specifically needed by the module. The problem with keeping composer.lock is that Rules is forcing certain dependencies on any site that might want to install Rules with composer. I don't see any reason why composer.lock should be in the git repository just for the benefit of that one external testing project which hasn't been used by @fago for years.

there will be some follow-up work required to get Travis build testing running, because PHPunit and PHPCS will need to run slightly differently.

Yes, I imagine it might, and I'm good with that happening - just post a patch.

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3106691-5.remove_composer_lock.patch14.58 KB

In addition to deleting composer.lock this patch has the changes to .travis.yml as we are now running PHPCS from the drupal installed version in $DRUPAL_ROOT/vendor/bin/phpcs not our own version which had been installed in modules/rules/vendor/bin/phpcs. I took the opportunity to add more info about phpcs by running --version, showing the sniffs and giving a summary of the coding standards faults. I have also added a couple of things to phpcs.xml.dist to set -colors and exclude *interdif*

  • TR committed 546d0f6 on 8.x-3.x authored by jonathan1055 
    Issue #3106691 by jonathan1055: Update or remove Rules composer.lock
tr’s picture

tr commented
Status: Needs review » Fixed

Committed.

jonathan1055’s picture

jonathan1055 commented

Thanks. This also has another benefit. We had been getting the error at PHP7.3 Core 8.8 and Core 8.9

PHP Warning:  "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? in /home/travis/build/jonathan1055/rules/vendor/squizlabs/php_codesniffer/CodeSniffer/File.php on line 1766

But this is now fixed when codesniffer remains at the proper version and is not downgraded.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.