Problem/Motivation

From #3031379-258: Add a new test type to do real update testing:

As long as we are following up, this appears to introduce a dependency on symfony/browser-kit, but does not declare the dependency. It's currently only installed becuase it's required y behat/mink-browser-kit-driver.

It's also getting installed at version 4.3 on php 7.1 with a composer update, but that adds string typehinting to the request method which we aren't compatible with pre-php 7.2

Can we explicitly require-dev symfony/browser-kit: ^3.4.0 (and we should be fine with type-hinting when we update to 4.3 on Drupal 9 with a minimum php version of 7.2)

Proposed resolution

require-dev symfony/browser-kit:~3.4.0, based on #3.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 3086332.patch2.5 KBheddn
#4 3086332-4.patch2.5 KBheddn
#4 interdiff_2-4.txt804 bytesheddn
#2 3086332.patch2.5 KBheddn

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Active » Needs review
StatusFileSize
new 3086332.patch2.5 KB
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented 
  1. +++ b/composer.json
@@ -25,7 +25,8 @@
+        "symfony/browser-kit": "^3.4.0"

    Shouldnt this be ~3.4.0 ? 

  2. +++ b/composer.lock
@@ -3652,6 +3652,9 @@
+            "bin": [
+                "bin/composer"
+            ],

    I dont think we should add bin files for composer here.. is there a reason this is here?

heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new interdiff_2-4.txt804 bytes
new 3086332-4.patch2.5 KB

As an aside (and I'm not arguing w/ the IS about version 4.3 adding string type hinting) but that seems like something that should also be resolved upstream in the symfony project. https://github.com/symfony/symfony/blob/4.4/composer.json currently lists PHP 7.1 as the required version.

Re #3: I noticed bin/composer added too. It is being added by composer to "name": "composer/composer". I'm not sure if it is my version of composer (1.9.0) or what is the cause. I can't _not_ introduce that change.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Title: Explicitly require-dev symfony/browser-kit: ^3.4.0 » Explicitly require-dev symfony/browser-kit: 3.4.0
Issue summary: View changes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

^ vs ~: We currently use ^ for other dev dependencies, and probably should here, too, for widest latitude: https://git.drupalcode.org/project/drupal/blob/8.8.x/composer.json#L22

In fact, we have two issues to let us run the DrupalMinkClient tests against browser-kit ^4.2: #3086557: Make DrupalMinkClient::request() compatible with max dependencies and #3086005: Make DrupalMinkClientTest::followMetaRefresh() compatible with max dependencies

So I'd RTBC #2.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
StatusFileSize
new 3086332.patch2.5 KB

Re-uploading #2 so it is last.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Needs review
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs review » Reviewed & tested by the community

Ah. I was just basing it off the other dev deps we put in here:

https://git.drupalcode.org/project/drupal/blob/8.8.x/composer.json#L26-28

https://symfony.com/roadmap does inform us that it really doesnt matter whether we use ^ or ~, simply due to the fact that there wont ever be a 3.5 of any of these dependencies.

So, #2/#4/#6 are essentially identical in impact.

RTBC Both and let the committer decide?

  • catch committed cae168a on 8.8.x 
    Issue #3086332 by heddn, Mixologic, Mile23: Explicitly require-dev...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed cae168a and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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