Problem/Motivation

In our composer we use ~ often. In the composer doc is says that the caret:

is the recommended operator for maximum interoperability when writing library code.

Furthermore when there is a patch release for a dependency which we'd like to help ensure users using composer to manage their project's dependencies get this will help them. See #2768953: Prevent insecure Guzzle from being installed when using composer to manager your project dependencies.

Proposed resolution

Change composer dependency versions to ^1.2.3 style using the caret. This would also allow for users managing their own dependencies to update to later minor versions when needed (outside of core releases).

Remaining tasks

  • Decide if we want to adopt ^1.2.3 style versioning
  • Patch composer.json to adjust

User interface changes

n/a

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#21 2769841-21.patch12.27 KBalexpott
#21 19-21-interdiff.txt856 bytesalexpott
#19 13-19-interdiff.txt8.37 KBalexpott
#19 2769841-19.patch12.27 KBalexpott
#13 interdiff_8-13.txt433 bytesedgewl2
#13 prefer_carat_over_tilde-2769841-13.patch3.91 KBedgewl2
#11 interdiff_8-11.txt43.93 KBedgewl2
#11 prefer_carat_over_tilde-2769841-11.patch47.42 KBedgewl2
#8 prefer_carat_over_tilde-2769841-8.patch3.44 KBedgewl2
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Patch composer.json to adjust

Do we potential have to adapt our component composer.json files as well?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Why not? Let's review the lot.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Acquia commented

Decide if we want to adopt ^1.2.3 style versioning

Is this not yet decided? I'm in the 'yes' vote camp... it seems like a lot of Drupal/Composer solutions are already defaulting to that style now.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue tags: +Novice

+1
Is the next step to roll a patch? Tagging novice for that. Let's see what happens when we change to a carrot.

edgewl2’s picture

edgewl2 CreditAttribution: edgewl2 commented
Status: Active » Needs review
FileSize
prefer_carat_over_tilde-2769841-8.patch3.44 KB

Can you check this out? It really can be helpful.

Status: Needs review » Needs work

The last submitted patch, 8: prefer_carat_over_tilde-2769841-8.patch, failed testing.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

After updating to tilde, we need to run composer update nothing, or something like that to update the lock file.

edgewl2’s picture

edgewl2 CreditAttribution: edgewl2 commented
Status: Needs work » Needs review
FileSize
prefer_carat_over_tilde-2769841-11.patch47.42 KB
interdiff_8-11.txt43.93 KB

I think this patch has what it takes

edgewl2’s picture

edgewl2 CreditAttribution: edgewl2 commented
Status: Needs review » Needs work
edgewl2’s picture

edgewl2 CreditAttribution: edgewl2 commented
Status: Needs work » Needs review
FileSize
prefer_carat_over_tilde-2769841-13.patch3.91 KB
interdiff_8-13.txt433 bytes

Can you check this out?

AjitS’s picture

AjitS
he/him
Primary language Marathi
Location Pune
CreditAttribution: AjitS as a volunteer and at QED42 commented
Issue summary: View changes
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

This does what is outlined in the IS, by switching to carat (^). Tests are passing. Looks good to me. I also did a quick check, we've got all the composer.json files updated.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Prefer carat over tilde in composer.json » Prefer caret over tilde in composer.json
Issue summary: View changes

https://en.wikipedia.org/wiki/Carat_(mass) vs https://en.wikipedia.org/wiki/Caret :) Sorry.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think this worth adding something to \Drupal\Tests\ComposerIntegrationTest to ensure the tilde does not make a return. It'd be simple to have a patch revert on of these changes and I think ensuring the caret is worth it.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue tags: -Novice

Removing the Novice tag as adding a test is less easy for a newcomer.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2769841-19.patch12.27 KB
13-19-interdiff.txt8.37 KB

Testing all the composer.json we have.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

I obviously missed a few composer.json files. Good thing for automated testing. But it seems we've caught all the changes now and we have tests.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
19-21-interdiff.txt856 bytes
2769841-21.patch12.27 KB

Rerolled on top of #2876408: Update wikimedia/composer-merge-plugin to ~1.4 in composer.json to match composer.lock

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2769841-21.patch, failed testing.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs work » Reviewed & tested by the community

Seems like a random test failure. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2769841-21.patch, failed testing.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs work » Reviewed & tested by the community

Unrelated test failure:

Drupal\KernelTests\Core\Theme\StableTemplateOverrideTest::testStableTemplateOverrides
PHPUnit_Framework_Exception: Segmentation fault (core dumped)
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.4.x, thanks!

  • catch committed 7c48639 on 8.4.x 
    Issue #2769841 by edgewl2, alexpott, heddn: Prefer caret over tilde in...
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Related issues: +#2887000: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal
cilefen’s picture

cilefen CreditAttribution: cilefen commented

The trouble is that this is letting installs go to Symfony 3.3, which doesn't actually work yet.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

What I wrote in #29 is not true and sorry for blaming this issue. "~3.2" is equivalent to "^3.2" in terms of minor version updates. The only way to prevent Symfony minor updates is "~3.2.8". I changed the nature of #2887000: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal accordingly.

  • cilefen committed 1d22a7b on 8.4.x 
    Revert "Issue #2769841 by edgewl2, alexpott, heddn: Prefer caret over...
cilefen’s picture

cilefen CreditAttribution: cilefen commented

This issue prevents the use of tildes in composer.json but I feel that is what is really needed for the Symfony components.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Fixed » Needs work
cilefen’s picture

cilefen CreditAttribution: cilefen commented

It wonder if we could test somehow (nightly?) that a composer update produces a functioning install.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

I don't understand what the special symfony cases have to do with the conversion from tilde to caret. The revert to tilde doesn't fix the symfony cases, because we then still have to change the declared version to prevent updates to 3.3.
Shouldn't we just handle the symfony constraints as the exception that they are? Don't we still just want to generally use the recommended operator?

EDIT: you said so much in #2769841-30: Prefer caret over tilde in composer.json but why not recommit or re-RTBC then?

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Because this patch contains a test preventing tildes.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Oh, are you suggesting modifying the operator constraint in the other issue? Sorry if I misread.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Reviewed & tested by the community

I am convinced by that argument. Let's get this re-tested and if #2887000: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal ever happens it will have deal with the exception.

  • cilefen committed e204c22 on 8.4.x 
    Issue #2769841 by edgewl2, alexpott, heddn: Prefer caret over tilde in...
cilefen’s picture

cilefen CreditAttribution: cilefen commented

Ok, back in it goes! That's a good point in #35. Committed e204c22 and pushed to 8.4.x. Thanks, and sorry for the back-and-forth!

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Fixed
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Fixed » Needs work

The composer.json files are a list of dependencies. Version specifications are not optional in may cases.

The changes in this issue result in this outcome:

$ composer update
$ ./vendor/bin/phpunit -c core --testsuite unit


[25-Jun-2017 03:00:56 Australia/Sydney] PHP Fatal error:  Declaration of Symfony\Bridge\PhpUnit\SymfonyTestsListener::addSkippedTest(PHPUnit\Framework\Test $test, Exception $e, $time) must be compatible with PHPUnit_Framework_TestListener::addSkippedTest(PHPUnit_Framework_Test $test, Exception $e, $time) in /Users/paulmitchum/projects/drupal8/vendor/symfony/phpunit-bridge/SymfonyTestsListener.php on line 70

That is to say: The tildes are there for a reason.

Currently, we don't have a test for this, but we should.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

I think the issue you are referencing is #2887000: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal. The change introduced in this issue does not cause #42. I know because if you revert this, #42 still happens.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Fixed

Cool thanks.

Status: Fixed » Closed (fixed)

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