Fixes #614 for PHP 8.1, Symfony 6, Drush 11, etc. #615

mikemadison13 · 2022-08-04T20:14:16Z

Updates the following:

drupal related dependencies to allow 10.x
drush to allow drush 11
behat/mink-goutte-driver to allow 2.x
symfony packages to allow ^6

Berdir · 2022-08-04T22:09:55Z

Overlaps with #610

mikemadison13 · 2022-08-09T11:51:00Z

@Berdir yeah, i know it overlaps with a couple of other efforts. i'm hoping we can get at least one of them merged, though. my behat testing is currently blocking me from being able to execute CI on my PHP 8.1 project update.

mikemadison13 · 2022-09-02T16:51:46Z

the failure I'm hitting seems to be related to symfony/panther#291, digging more.

mikemadison13 · 2022-09-02T17:24:22Z

@jhedstrom digging into the issues in the build failure, it looks like https://github.com/FriendsOfBehat/MinkExtension did some refactoring in version 2 which is causing the set client error in the build:

In GoutteFactory.php line 117:
                                                    
Method Goutte\Client::setClient() does not exist

I've started poking at this a little bit, it looks like they are checking for Goutte1 in their code (see https://github.com/FriendsOfBehat/MinkExtension/blob/master/src/Behat/MinkExtension/ServiceContainer/Driver/GoutteFactory.php#L115) which could be the root cause.

I'm going to go open an issue over there to see if we can make forward progress.

mikemadison13 · 2022-09-02T17:35:55Z

actually, it looks like there is a PR that fixes this FriendsOfBehat/MinkExtension#16 and it works, but not for the Drupal 7 stuff. I think we may need to split the drupalextension and have one version that is for newer versions of symfony. any thoughts?

jhedstrom · 2022-09-02T18:25:41Z

@mikemadison13 yeah, I think we could start a 5.x and drop support for Drupal 7 in that branch?

mikemadison13 · 2022-09-02T20:33:46Z

it's unclear to me if there's anything in #610 that we need to roll into this. i don't think so based on what i see there, but that PR does change the behat.yml.dist file from goutte to browserkit_http. i'm not sure if that's still needed? @claudiu-cristea might have some input?

mikemadison13 · 2022-09-02T20:35:14Z

i think we can also close #602 as well given this?

jhedstrom · 2022-09-02T21:53:07Z

composer.json

@@ -59,6 +61,11 @@
    }
  },
  "extra": {
+    "patches": {
+        "friends-of-behat/mink-extension": {
+            "setClient error": "https://github.com/FriendsOfBehat/MinkExtension/files/9480850/16.txt"


Let's add the PR number #16 here for easy reference to the issue in case we forget where this came from :)

I created mikemadison13#1 (against fork)

In my project the patch doesn't apply, friends-of-behat/mink-extension is locked to 2.3.1

i wonder if we need to set a minimum version to 2.7.1 of the mink-extension? I'm not sure how we make this PR work without that patch (because it fatal errs out). So we either need to pin way earlier (2.3.1 and remove the patch) or pin later (2.7.1) with. my preference would be the newer version. thoughts @jhedstrom ?

jhedstrom · 2022-09-02T21:53:58Z

composer.json

+        "friends-of-behat/mink-extension": {
+            "setClient error": "https://github.com/FriendsOfBehat/MinkExtension/files/9480850/16.txt"
+        }
+    },
    "branch-alias": {
      "dev-master": "4.3.x-dev"


you'll want to merge master in since this line has changed for 5.0.x now.

I created mikemadison13#1 (against fork)

i've rebased off of the upstream/master and have this change now.

claudiu-cristea · 2022-09-03T13:38:42Z

@mikemadison13 I don't have time for #610 and, honestly, I have no idea how to fix it. Feel free to include here anything that could be useful

claudiu-cristea · 2022-09-06T07:15:44Z

@mikemadison13 This PR is fixing the dependency issues I have when updating to PHP 8.1. Thank you. Could you, please fix the 2 minor remarks and let's get this in?

Berdir · 2022-09-07T19:57:26Z

composer.json

@@ -59,6 +61,11 @@
    }
  },
  "extra": {
+    "patches": {
+        "friends-of-behat/mink-extension": {
+            "setClient error from PR16": "https://github.com/FriendsOfBehat/MinkExtension/files/9480850/16.txt"


even with the version constraint, a patch is IMHO too unstable, sooner or later that won't apply anymore or will be merged. We could maybe combine it with a specific requirement on that specific version, but waiting for that to be merged and committed is probably the safer option.

@Berdir there have been a number of requests to merge the PR and it hasn't happened, so I don't fundamentally disagree with the concern, but I'm not sure if the maintainer can merge things at the moment given their location. we did try and make this "more stable" by pointing at a patch file vs. the actual PR. i guess we could explicitly pin to 2.7.1 to prevent accidental breakage / updates?

AlexSkrypnyk · 2022-10-06T08:53:18Z

Can we please get this merged. The CI passes, the patch is actually a "static" file - all looks like it is working.

If that patch won't apply in the future - we can always update this package and roll a new version.

This is currently blocks using Behat with Drupal 10 big time.

claudiu-cristea · 2022-10-15T14:13:06Z

@jhedstrom @Berdir @mikemadison13 wouldn't be better if we try to move away from the Goutte driver? See here how Moodle community did it stronk7/moodle@master...bump_goutte_driver_20

Berdir · 2022-10-15T15:12:14Z

Sure, that's what I always expected will need to happen eventually, see first comment in #606 :) Back then it was blocked on FriendsOfBehat/MinkExtension#14, but now that should be possible?

claudiu-cristea · 2022-10-15T15:31:17Z

Sure, that's what I always expected will need to happen eventually, see first comment in #606 :) Back then it was blocked on FriendsOfBehat/MinkExtension#14, but now that should be possible?

@Berdir could you, please, take a look at #620. It's not ready yet but I have a green there

EDIT:

Used few commits from Fixes #614 for PHP 8.1, Symfony 6, Drush 11, etc. #615 and inspired from stronk7/moodle@master...bump_goutte_driver_20
Will need a new 5.x branch as we drop support for D7, D8.
Needs more D7/8 cleanup
Not tested yet with Drupal 10

claudiu-cristea · 2022-10-17T07:51:38Z

@AlexSkrypnyk could you take a look and try #620? That drops Goutte Driver.

@mikemadison13 should we close this in favor of #620?

mikemadison13 · 2022-10-17T14:53:52Z

my strong preference would be to get this in ASAP so we can unblock d10. and then we can explore the feature dev for Goutte. but obviously i defer to @jhedstrom

claudiu-cristea · 2022-10-17T14:58:48Z

my strong preference would be to get this in ASAP so we can unblock d10

@mikemadison13 we're already there. See the tests on GitHub actions here https://github.com/claudiu-cristea/drupalextension/actions/runs/3266111531. I'm now investigating the failure with Drupal 10, which is happening in drupal/drupal-driver

EDIT: I had to implement GitHub actions as Travis CI hit some quota

mikemadison13 · 2022-10-17T15:08:49Z

i believe we would still need to merge in all of the symfony 6 stuff from this (or another PR) to get us to d10, right?

claudiu-cristea · 2022-10-17T15:11:29Z

i believe we would still need to merge in all of the symfony 6 stuff from this (or another PR) to get us to d10, right?

I've cherry picked some commits from here in #620. Actually, it supports Sf 6, Drush 11. I'm still working on D10 stuff. Very close...

AlexSkrypnyk · 2022-10-18T06:12:07Z

The upstream blocker was merged.

If I may suggest - can we update this PR, merge it, release it and then look at 620?

claudiu-cristea · 2022-10-18T07:03:23Z

@AlexSkrypnyk they are doing the same, with the exception that FriendsOfBehat/MinkExtension#16 wraps the BrowserKit Driver from Goutte Driver while #620 uses directly the BrowserKit Driver and drops the dependency on Goutte Driver

AlexSkrypnyk · 2022-10-26T03:30:44Z

@jhedstrom
Can we please merge and release this one and tackle #620. There are already several complex dependency resolution issues happening here. Bringing in a new dependency in this PR (which is 100% working and is ready to go) can potentially throw us back into discussions about approaches. I would suggest to have those discussions separately (in #620) and merge this one as is.

claudiu-cristea · 2022-10-26T10:21:08Z

@AlexSkrypnyk I don't see why this needs to be merged as #620 is solving exactly the same issue but as a definitive fix. Did you give #620 a try? If we merge this just to drop it in #620 somebody has to rework/reroll #620. Useless work. Why not, better, focusing on that to se if fulfills the scope?

AlexSkrypnyk · 2022-10-26T10:54:22Z

@claudiu-cristea
That PR adds a lot of changes that need to be discussed. I simply want to unblock downstream packages ASAP and then take time to review #620.

There is only 1 files change in this PR (composer.json) that will need a re-roll in #620.

Berdir · 2022-10-26T10:57:58Z

goutte v2 is just a wrapper around browserkit, I agree that it makes more sense to pursue that directly.

claudiu-cristea · 2022-10-26T11:36:52Z

@AlexSkrypnyk 90% of changes are only file removals, renaming, docs changes and Behat tags removals because it drops the Drupal 7/8 support

mikemadison13 · 2022-10-26T14:31:17Z

just to chime in to support the point @AlexSkrypnyk is making, it would be fantastic to get D10 unblocked asap. if that's with this pr great. if that's with 620 great, i don't have a lot of skin in the game. i just want to get behat unblocked. it feels safer to use the current approach first and then explore a new approach separately, but 🤷

AlexSkrypnyk · 2022-10-26T20:14:12Z

@Berdir @claudiu-cristea
It makes sense to replace goutte as it is a wrapper around browserkit and should be decommissioned. But it is about when to do it and how long it will take to review and accept changes in #620 (which I think are great).

#620 has a lot of changes: there are new local env configs, new CI configs, Drupal 8 deprecations, test artefacts and other changes that will take time to discuss. I guess it is up to project maintainer to decide if changes in that PR can be accepted as-is or more discussions are required.

We have this PR that is working and does not change any dependencies (so that downstream projects do not have to update theirs). It is ready to be merged right now. It has minimal changes and, therefore, less risk to introduce new issues.

I do not see a problem with merging this PR as-is and releasing a new version (could even be a patch version) to unblock downstream and then concentrate on #620 and spend as much time as required there.

jhedstrom · 2022-10-26T20:18:58Z

This could potentially go against the latest 4.x.x branch, and keep #620 against master (which will be 5.0.x)?

mikemadison13 · 2022-10-26T20:20:53Z

@jhedstrom i think we discussed cutting the 5.x branch for Drupal 10 given that we had to remove some of the earlier supported versions of things, right? just making sure i don't misremember. this branch will not work well with older things.

jhedstrom · 2022-10-26T20:25:23Z

Yeah, 5.x will drop D7 support and add D10 support, so scratch my thought above. Since 5.x is not released, and #620 is the eventual direction we'd want to go anyway, I'm inclined to go that route...

mikemadison13 · 2022-10-26T20:33:01Z

cool, if that's the route to go then as i said above i don't really care. i just would like the most expedient route to d10. so if we aren't merging this and cutting it (despite it being ready to go right now) when can we have the other? happy to help test once we have the dependencies merged.

claudiu-cristea · 2022-11-18T13:25:57Z

I guess this can be closed. The fix from #620 has been already merged in master. I would love to get some feedback from projects using the current master.

jhedstrom · 2022-12-22T22:30:34Z

Thanks for all the work here! I'm closing this out since #620 was merged.

AlexSkrypnyk · 2023-03-17T03:41:13Z

@jhedstrom
This PR was authored in October 2022. It was then closed in Dec 2022. It is now March 2023 and we still do not have a stable release with this update for 4.x or 5.x after 5 months :(

The reason for submitting this pull request was to prevent this specific situation from occurring.

Could you please advise what is the planned course of actions here? Is 5.x going to have a release soon? Can this PR be ported to 4.x and released?

jhedstrom · 2023-03-17T17:22:44Z

@AlexSkrypnyk thanks for the reminder. Since no major issues have been reported with the alpha release I've tagged RC1 for 5.0.0.

mikemadison13 force-pushed the 614 branch from 902ba18 to 0e0f0a2 Compare August 4, 2022 20:22

mikemadison13 force-pushed the 614 branch from 6b3722e to d2ae871 Compare September 2, 2022 17:15

mikemadison13 force-pushed the 614 branch 4 times, most recently from 2276754 to ef875ea Compare September 2, 2022 20:22

jhedstrom mentioned this pull request Sep 2, 2022

Drop Drupal 7 support #616

Open

mikemadison13 changed the title ~~Fixes #614 to allow symfony 6.~~ Fixes #614 for PHP 8.1, Symfony 6, Drush 11, etc. Sep 2, 2022

jhedstrom reviewed Sep 2, 2022

View reviewed changes

claudiu-cristea mentioned this pull request Sep 6, 2022

behat/mink-goutte-driver update #610

Closed

mikemadison13 added 2 commits September 6, 2022 07:49

Fixes jhedstrom#614 to allow symfony 6.

4365263

removing drupal 7 and php 7.3 builds.

2c66e73

mikemadison13 force-pushed the 614 branch from ef875ea to d1c8b03 Compare September 6, 2022 14:57

Berdir reviewed Sep 7, 2022

View reviewed changes

claudiu-cristea mentioned this pull request Oct 15, 2022

Drop Goutte Driver. Support D10, Drush 11, Symfony 6 #620

Merged

mikemadison13 added 2 commits October 17, 2022 07:49

dropping symfony 3 support.

d682de8

bumping friends-of-behat/mink-extension to 2.7.2.

fa1f4ea

mikemadison13 force-pushed the 614 branch from 0334ddf to fa1f4ea Compare October 17, 2022 14:53

mikemadison13 mentioned this pull request Nov 3, 2022

Enable Drupal 10 support acquia/drupal-spec-tool#63

Closed

jhedstrom closed this Dec 22, 2022

Fixes #614 for PHP 8.1, Symfony 6, Drush 11, etc. #615

Fixes #614 for PHP 8.1, Symfony 6, Drush 11, etc. #615

Conversation

mikemadison13 commented Aug 4, 2022

Berdir commented Aug 4, 2022

mikemadison13 commented Aug 9, 2022

mikemadison13 commented Sep 2, 2022

mikemadison13 commented Sep 2, 2022

mikemadison13 commented Sep 2, 2022

jhedstrom commented Sep 2, 2022

mikemadison13 commented Sep 2, 2022

mikemadison13 commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiu-cristea commented Sep 3, 2022

claudiu-cristea commented Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexSkrypnyk commented Oct 6, 2022 • edited Loading

claudiu-cristea commented Oct 15, 2022 • edited Loading

Berdir commented Oct 15, 2022

claudiu-cristea commented Oct 15, 2022 • edited Loading

claudiu-cristea commented Oct 17, 2022

mikemadison13 commented Oct 17, 2022

claudiu-cristea commented Oct 17, 2022 • edited Loading

mikemadison13 commented Oct 17, 2022

claudiu-cristea commented Oct 17, 2022

AlexSkrypnyk commented Oct 18, 2022

claudiu-cristea commented Oct 18, 2022 • edited Loading

AlexSkrypnyk commented Oct 26, 2022

claudiu-cristea commented Oct 26, 2022

AlexSkrypnyk commented Oct 26, 2022

Berdir commented Oct 26, 2022

claudiu-cristea commented Oct 26, 2022

mikemadison13 commented Oct 26, 2022

AlexSkrypnyk commented Oct 26, 2022

jhedstrom commented Oct 26, 2022

mikemadison13 commented Oct 26, 2022

jhedstrom commented Oct 26, 2022

mikemadison13 commented Oct 26, 2022

claudiu-cristea commented Nov 18, 2022

jhedstrom commented Dec 22, 2022

AlexSkrypnyk commented Mar 17, 2023

jhedstrom commented Mar 17, 2023

claudiu-cristea commented Sep 6, 2022 •

edited

Loading

AlexSkrypnyk commented Oct 6, 2022 •

edited

Loading

claudiu-cristea commented Oct 15, 2022 •

edited

Loading

claudiu-cristea commented Oct 15, 2022 •

edited

Loading

claudiu-cristea commented Oct 17, 2022 •

edited

Loading

claudiu-cristea commented Oct 18, 2022 •

edited

Loading