drupalextension
drupalextension copied to clipboard
jhedstrom
Fixes #614 for PHP 8.1, Symfony 6, Drush 11, etc.
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 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.
the failure I'm hitting seems to be related to https://github.com/symfony/panther/issues/291, digging more.
@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.
actually, it looks like there is a PR that fixes this https://github.com/FriendsOfBehat/MinkExtension/pull/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?
@mikemadison13 yeah, I think we could start a 5.x and drop support for Drupal 7 in that branch?
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 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
@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?
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.
@jhedstrom @Berdir @mikemadison13 wouldn't be better if we try to move away from the Goutte driver? See here how Moodle community did it https://github.com/stronk7/moodle/compare/master...bump_goutte_driver_20
Sure, that's what I always expected will need to happen eventually, see first comment in https://github.com/jhedstrom/drupalextension/pull/606 :) Back then it was blocked on https://github.com/FriendsOfBehat/MinkExtension/pull/14, but now that should be possible?
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 #615 and inspired from https://github.com/stronk7/moodle/compare/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
@AlexSkrypnyk could you take a look and try #620? That drops Goutte Driver.
@mikemadison13 should we close this in favor of #620?
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
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
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 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...
The upstream blocker was merged.
If I may suggest - can we update this PR, merge it, release it and then look at 620?
@AlexSkrypnyk they are doing the same, with the exception that https://github.com/FriendsOfBehat/MinkExtension/pull/16 wraps the BrowserKit Driver from Goutte Driver while #620 uses directly the BrowserKit Driver and drops the dependency on Goutte Driver
@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.
@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?
@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.
goutte v2 is just a wrapper around browserkit, I agree that it makes more sense to pursue that directly.
@AlexSkrypnyk 90% of changes are only file removals, renaming, docs changes and Behat tags removals because it drops the Drupal 7/8 support
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 🤷
@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.
This could potentially go against the latest 4.x.x branch, and keep #620 against master (which will be 5.0.x)?
@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.