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

Allow symfony/finder 4 #3744

Merged
merged 1 commit into from
Nov 8, 2018
Merged

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Oct 16, 2018

The symfony/finder component is not a core Drupal dependency. So it not technically locked to ^3.4.

I have reviewed the usage that is made of this component in drush. The only thing that is used is the Symfony\Component\Finder\Finder class.

I have checked every method of this class that is used by drush. Then I have read the CHANGELOG of the component and did not see any obvious impact for drush. Then I made a diff between the 3.4 branch and the master branch of this class file and manually checked changes that could break drush usage of this component. I did not find any.

It would be great to accept v4 of this component, so projects that require drush and this component directly can use the latest version.

@greg-1-anderson
Copy link
Member

Tests are green so I'm +1 on this.

@weitzman: The greater question is should we ensure that we actually test with multiple versions of the Symfony components? As it is, this change means that we'll be testing with ^4 all the time. This is probably fine; we could consider adding a lowest test configuration to the functional tests. That wouldn't be hard, but I don't know if it would be worth the extra time. Maybe convert the php 5.6 test to a lowest test?

@deviantintegral
Copy link
Contributor

Maybe convert the php 5.6 test to a lowest test?

👍 to this.

This comes up if you run core's command to update PHPUnit for testing on PHP 7, and then try to install drush afterwards. You have to force a downgrade with composer require symfony/finder:^3.4 and then install drush, which works but isn't intuitive.

@weitzman weitzman merged commit a5b3737 into drush-ops:master Nov 8, 2018
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 this pull request may close these issues.

None yet

4 participants