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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.3.x-dev » 8.2.x-dev
Status: Active » Needs review
FileSize
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
CreditAttribution: dawehner at Chapter Three 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 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three 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 CreditAttribution: 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
CreditAttribution: Mile23 as a volunteer 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
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

cburschka’s picture

cburschka
they
CreditAttribution: cburschka 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
CreditAttribution: cburschka 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
CreditAttribution: cburschka 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 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three 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.