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

Comments

alexpott created an issue. See original summary.

dawehner’s picture

Patch composer.json to adjust

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

alexpott’s picture

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

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

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

Status: Active » Needs review
FileSize
3.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

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

edgewl2’s picture

Status: Needs work » Needs review
FileSize
47.42 KB
43.93 KB

I think this patch has what it takes

edgewl2’s picture

Status: Needs review » Needs work
edgewl2’s picture

Status: Needs work » Needs review
FileSize
3.91 KB
433 bytes

Can you check this out?

AjitS’s picture

Issue summary: View changes
heddn’s picture

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

Title: Prefer carat over tilde in composer.json » Prefer caret over tilde in composer.json
Issue summary: View changes
alexpott’s picture

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

Issue tags: -Novice

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

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
12.27 KB
8.37 KB

Testing all the composer.json we have.

heddn’s picture

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

Status: Reviewed & tested by the community » Needs work

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

heddn’s picture

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

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

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’s picture

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

cilefen’s picture

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

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

Status: Fixed » Needs work
cilefen’s picture

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

Eric_A’s picture

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

Because this patch contains a test preventing tildes.

cilefen’s picture

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

cilefen’s picture

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

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

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

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

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

Status: Needs work » Fixed

Cool thanks.

Status: Fixed » Closed (fixed)

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