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

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

Merged
merged 35 commits into from
Nov 7, 2022
Merged

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

merged 35 commits into from
Nov 7, 2022

Conversation

claudiu-cristea
Copy link
Contributor

@claudiu-cristea claudiu-cristea commented Oct 15, 2022

@claudiu-cristea
Copy link
Contributor Author

@jhedstrom any idea why the tests are not running with the last commit?

@Berdir
Copy link
Contributor

Berdir commented Oct 15, 2022

I see that 5.x is open as development version, makes sense to introduce the hard break here. Won't have time for a closer look in the next week or so, but looks great at first glance.

@claudiu-cristea
Copy link
Contributor Author

claudiu-cristea commented Oct 15, 2022

@Berdir unfortunately applied to my project I hit this (which I‘ve experienced also with the #615 PR) symfony/symfony#47505. I really think that it’s a symfony/mime bug. And it happens a lot with Drupal forms, just think at forms with files, i.e. file[0] fields

@claudiu-cristea
Copy link
Contributor Author

claudiu-cristea commented Oct 17, 2022

@jhedstrom @Berdir @mikemadison13 we have green with GitHub actions:

@claudiu-cristea claudiu-cristea changed the title Try to drop goutte drv Drop Goutte Driver. Support D10, Drush 11, Symfony 6 Oct 17, 2022
@pfrenssen
Copy link
Collaborator

Replacing Travis with Github actions makes sense imo. And @jhedstrom also seems to be on board with this, ref jhedstrom/DrupalDriver#254

@mikemadison13
Copy link
Contributor

do we have any updates here? if this is going to continue on, it would be awesome if we could merge #615 so we can unblock d10 and then do the feature development here. thanks so much!

@claudiu-cristea
Copy link
Contributor Author

@mikemadison13 I don't get why everybody wants to see #615 merged but avoid to give this a chance by running the tests. I've been tested with both on and both are having exactly the same effect, just that this one is a definitive fix.

@mikemadison13
Copy link
Contributor

totally a fair question. i always try to do feature development separate from maintenance, that's my big thing. a 100+ files changing in a PR feels like iterating on a feature / expanding capabilities, which is different activity from just updating dependencies to allow an install. i don't fundamentally care which PR gets merged, i'm just saying i'd like one of them to get merged soon (and if 615 is easier to test, then we should start there).

@claudiu-cristea
Copy link
Contributor Author

As I already told, the big amount of file changes is a result of dropping Drupal 7/8 support and fixing the documentation. Actually, #615 is also dropping the support but forgets to remove the tests and fix the documentation. I also had to switch from Travis CI to GitHub Actions and the tests stop running because of some quota limit.

@mikemadison13
Copy link
Contributor

i'm happy for this to get merged if it can be merged quickly. i don't really care which one goes in. i just would like to be able to unblock the behat tests for D10 :) that's really my only goal. yours, mine, or another.

@claudiu-cristea
Copy link
Contributor Author

@claudiu-cristea
Copy link
Contributor Author

Opened #622 as follow-up

composer.json Outdated
"composer-exit-on-patch-failure": true,
"patches": {
"drupal/drupal-driver": {
"https://github.com/jhedstrom/DrupalDriver/pull/253": "https://patch-diff.githubusercontent.com/raw/jhedstrom/DrupalDriver/pull/253.diff"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of patching, I'll merge jhedstrom/DrupalDriver#253 and then we can update composer.json to pull dev until we release 5.0.

@jhedstrom jhedstrom merged commit e169b96 into jhedstrom:master Nov 7, 2022
@jhedstrom
Copy link
Owner

This is in! Thanks all for sticking with this, and pushing it forward!

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

5 participants