Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because composer update --prefer-lowest --prefer-stable fails.
Issue priority Major because it could cause issues with composer dependencies.

Problem/Motivation

When Symfony tests their code they run composer update --prefer-lowest --prefer-stable to try the lowest stable version of each dependency. Doing this in D8 fails.

The dependency that fails is "mikey179/vfsStream". We currently require version 1.* in core/composer.json. I'm looking at what is the minimum version that we need and specifying that in composer.json. For example, we currently lock to version 1.5, so could specify "~1.5" (or lower) in core/composer.json instead of "1.*".

Proposed resolution

1.2 is the minimum needed to pass the tests, therefore "~1.2" is the proposed setting.

Remaining tasks

none

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#1 set_better_version_for-2529082-1.patch903 bytestimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
set_better_version_for-2529082-1.patch903 bytes

We should be good with "~1.2"

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

Is there any reason not to require the latest stable version?

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

composer update will take the latest stable version, and as we ship with composer.lock composer install will currently take version 1.5 described in there (which is the latest stable version). However this version is not needed for Drupal 8, so there's no reason to require it.

"~1.2" will allow 1.2 or higher, but less than 2.0.0.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

We should certainly not pin down our exact used version, but well, if we are already on 1.5, by not go with that?

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

1.2 works, so I feel no reason to enforce a higher version, however we are not pinning down the version as you said, so the highest available version should be installed.

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt as a volunteer and at Golf Channel commented

#4,

Also, the version is pinned down in composer.lock so for any non-Composer user, they wont see a difference. Allowing a bigger range of dependencies allows composer users to install libraries that need different versions.

Let's say someone needs to use a dependency that needs <1.5 by pinning it to >=1.5 we are preventing someone from installing the library because the dependencies cannot be resolved. :(

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt as a volunteer and at Golf Channel commented
Status: Needs review » Reviewed & tested by the community

Patch looks good to me.

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt as a volunteer and at Golf Channel commented
Assigned: timmillwood » Unassigned
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Unfortunately, since #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead hasn't happened, the patch will need to include the changes to core/vendor/.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

@Mile23 - Why?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Er, yah, if the files are unchanged then no biggie, my mistake. But if they are, then we need them.

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt as a volunteer and at Golf Channel commented
Status: Needs work » Reviewed & tested by the community

The files will be unchanged because Composer by default installs the latest version.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Think I have passed the github rate limit throttling so here's the tests for those interested.
https://travis-ci.org/timmillwood/drupal/builds/70065984

alexpott’s picture

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

Thanks @timmillwood nice patch - I like the idea of being compatible with a wider variety of libraries. Committed 31da514 and pushed to 8.0.x. Thanks!

  • alexpott committed 31da514 on 8.0.x 
    Issue #2529082 by timmillwood: Set better version for mikey179/vfsStream

Status: Fixed » Closed (fixed)

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

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commented

timmillwood,

Is there an issue to update the testbot to run composer update --prefer-lowest --prefer-stable on one of the runs?

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

No, only for composer install.

Should be quite simple to add.