Problem/Motivation

In Composer 1.3 and higher the hash value has been removed from the lock file. This value is used in Drupal\Tests\ComposerIntegrationTest. See https://github.com/composer/composer/commit/8313e8687756059ef318a01386de...

Proposed resolution

We have three options:

  1. Fix the \Drupal\Tests\ComposerIntegrationTest::testComposerLockHash to test the content-hash by include composer as a dev dependency
  2. Fix the \Drupal\Tests\ComposerIntegrationTest::testComposerLockHash to test the content-hash by copying \Composer\Package\Locker::getContentHash() to the test
  3. Remove the test

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#2 2843259-2.patch2.3 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.3.x-dev » 8.2.x-dev
Status: Active » Needs review
StatusFileSize
new 2843259-2.patch2.3 KB

I think we need to keep the test since it gives us confidence that composer updates have been done correctly. I've gone for option 2 as the smallest impact.

dawehner’s picture

dawehner
Primary language German
commented

Without reading the patch first, I would have chosen the same solution. Its a bit unfortunate as this might break in the future, when that algorithm changes, but it would annoy enough people on twitter that we would be informed pretty quickly.

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -126,4 +125,50 @@ public function testAllModulesReplaced() {
+    $relevantKeys = array(
+      'name',
+      'version',
+      'require',
+      'require-dev',
+      'conflict',
+      'replace',
+      'provide',
+      'minimum-stability',
+      'prefer-stable',
+      'repositories',
+      'extra',
+    );

Totally out of scope ... I believe composer patches should somehow be able to hook into that process, I guess? On the other hand its quite cool to not have merge conflicts anymore, as patches really just change the main composer.json and that's it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Well if the algorithm changes the test will probably fail so hopefully DrupalCI will tell us before twitter :)

eric_a’s picture

eric_a commented

Note that last december @Mile23 produced almost the exact same code in #2744463-111: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core!

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Approving this since it's the same solution as #2744463-111: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core :-)

It's probably better to have a commit message that says 'this is because of composer 1.3' than something not directly related as it would with that other issue.

  • catch committed 03cd7e5 on 8.3.x 
    Issue #2843259 by alexpott, Mile23: Drupal\Tests\ComposerIntegrationTest...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

cburschka’s picture

cburschka
they
commented
Status: Fixed » Needs review

Can we get this into 8.2.x too? The same patch should already work.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Reviewed & tested by the community

(In fact this seems to have been RTBC on 8.2.x before being committed to 8.3.x)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2843259-2.patch, failed testing.

cburschka’s picture

cburschka
they
commented
Status: Needs work » Reviewed & tested by the community

(The patch was tested for 8.3.x when it should be on 8.2.x)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d8ca4b4 and pushed to 8.2.x. Thanks!

As a test-only change this is harmless to cherrypick to 8.2.x

  • alexpott committed d8ca4b4 on 8.2.x authored by catch 
    Issue #2843259 by alexpott, Mile23: Drupal\Tests\ComposerIntegrationTest...

  • catch committed 03cd7e5 on 8.4.x 
    Issue #2843259 by alexpott, Mile23: Drupal\Tests\ComposerIntegrationTest...

  • catch committed 03cd7e5 on 8.4.x 
    Issue #2843259 by alexpott, Mile23: Drupal\Tests\ComposerIntegrationTest...

Status: Fixed » Closed (fixed)

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