Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the sebastian/version dependeny #3130

Closed
jurgenhaas opened this issue Nov 2, 2017 · 9 comments · Fixed by #3139
Closed

Change the sebastian/version dependeny #3130

jurgenhaas opened this issue Nov 2, 2017 · 9 comments · Fixed by #3139

Comments

@jurgenhaas
Copy link
Contributor

For some development work I wanted to install dealerdirect/qa-tools on my D8 site which currently also has Drush 9 beta8. Both can not be installed together because of a version conflicht regarding sebastian/version:

  • Drush requires version ~1
  • dealerdirect/qa-tools requires version ^2.0

I couldn't find any issues with the 2.x version and wonder if Drush could change their requirement to "~1 | ^2" to allow both and not breaking any other dependencies?

@weitzman
Copy link
Member

weitzman commented Nov 2, 2017

Can you elaborate on "I couldn't find any issues with the 2.x version ". Have you run the drush test suite with that version? That would be a good first step. Drush uses this library to determine its own version.

@greg-1-anderson
Copy link
Member

Note that sebastian/version also appears in the composer.json in the isolation directory. If we use ^1|^2 in Drush's composer.json, we'll need to make the same change in the isolation tests. Once we start doing this, we should add highest/lowest testing for the isolation tests. These are super fast, so I don't think it would hurt to do highest/lowest/lock for both PHP versions.

@greg-1-anderson
Copy link
Member

I'll make a PR to try this.

@greg-1-anderson
Copy link
Member

Does Drush need to know its own version during preflight? I guess it might if we did any drush-version-specific path alteration for commandfile paths et. al.; Drush 9 currently does not do anything like this, although Drush 8 and earlier did.

@greg-1-anderson
Copy link
Member

greg-1-anderson commented Nov 2, 2017

So, we have sort of a problem:

$ composer why sebastian/version
drush/drush                dev-master  requires  sebastian/version (^1|^2)  
phpunit/php-code-coverage  2.2.4       requires  sebastian/version (~1.0)   
phpunit/phpunit            4.8.36      requires  sebastian/version (~1.0) 

In order to test sebastian/version: ^2, we would need a newer version of phpunit. Using a newer version of phpunit would probably prevent us from testing with sebastian/version: ^1. Making our tests work with two versions of phpunit at the same time sounds arduous.

Suggestion: stop using sebastian/version in Drush.

@jurgenhaas
Copy link
Contributor Author

Can you elaborate on "I couldn't find any issues with the 2.x version ".

I compared the Version classes of version 1 and 2 and there are only 2 changes: (1) they have added some comments and (2) they replaced @exec with proc_open when reading the git version information.

Have you run the drush test suite with that version? That would be a good first step. Drush uses this library to determine its own version.

No I haven't, the code changes don't really make any difference and I really wonder why they've introduced a major version change.

@weitzman
Copy link
Member

weitzman commented Nov 2, 2017

The reason why Drush historically has needed to know its own version is that we use that as an automatic cache key in the cache api which we we are considering removing. The factory cache bin saves stuff during preflight, so thats what triggers it. Its possible to unwind this stuff but it takes some effort.

@greg-1-anderson
Copy link
Member

sebastian/version bumped its major version in order to require php 5.6 or later. It seems the only reason it did that was to use a Symfony style checker.

If we upgrade to phpunit ^5.7, then the version constraint for sebastian/version is relaxed to ^1|^2, which would be perfect for our needs.

However, Drupal itself (as of Drupal 8.5.x) requires phpunit >=4.8.5 < 5, and, as previously mentioned, phpunit 4.x requires sebastian/version ^1.

Therefore, it seems to me that adding dealerdirect/qa-tools to Drupal 8 is a non-starter.

@greg-1-anderson
Copy link
Member

See #3139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants