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

Php81 #606

Merged
merged 4 commits into from
Apr 26, 2022
Merged

Php81 #606

merged 4 commits into from
Apr 26, 2022

Conversation

Berdir
Copy link
Contributor

@Berdir Berdir commented Apr 24, 2022

This is based on #602, phpspec is now available and passes with this, but I suspect this is only the start.

Specifically, I expect we need to replace goutte with browserkit driver along the lines of https://www.drupal.org/project/drupal/issues/3176655. And the guzzle deprecation messages require guzzle 7, which in turn requires https://www.drupal.org/project/drupal/issues/3225966.

@Berdir
Copy link
Contributor Author

Berdir commented Apr 24, 2022

Ok, maybe we don't have to replace goutte just yet. That's also blocked on FriendsOfBehat/MinkExtension#14.

instead, switching to the friends-of-behat minkextension fork, which seems to be php8.1 compatible. Still leaves the guzzle problem related to the core issue, but switching to v7 is not just blocked on that but also getting rid of fabpot/goutte, so again the issue above.

also some php8.1 fixes and a wrong config ID.

However, I am very confused about the url generator error. I see several weird things around that, including the fact that optional service methods apparently do not seem properly supported by core (it should not call it with NULL but not call at all). Making it non-required causes deprecated warnings that need changes in DrupalDriver. But I don't yet see how that's 8.1 related.

I also included #601 to be able to run tests.

@Berdir
Copy link
Contributor Author

Berdir commented Apr 25, 2022

So I was able to fix the url generator problem in drupal-driver.... by removing a few line of code, specifically just one would have been sufficient but I thought I'd remove them all as they didn't seem to cause a problem when not there. I guess that's all leftover code from a very, very long time ago or just copied from D7 without adjusting.

Merge request for that here: jhedstrom/DrupalDriver#245

did a hack to include that commit here, somehow that only worked on D9, but there it passed: https://app.travis-ci.com/github/jhedstrom/drupalextension/jobs/568139241

What apparently happened is that this caused a php deprecation notice, which behat error handling converted to an exception and then that resulted in skipping that service and injecting NULL instead, which is a bug too but only a follow-up bug.

The guzzle php deprecation still happens, but it doesn't prevent tests from running, that will need to be patched for now or so, we can do a new MR to user browserkit directly once FriendsOfBehat/MinkExtension#14 lands, then you can possibly update to guzzle 7 with some composer alias trickery or when https://www.drupal.org/project/drupal/issues/3225966 gets in.

@jhedstrom jhedstrom merged commit c5c0382 into jhedstrom:master Apr 26, 2022
@@ -23,7 +23,7 @@
"require": {
"behat/behat": "~3.2",
"behat/mink": "~1.5",
"behat/mink-extension": "~2.0",
"friends-of-behat/mink-extension": "^2",
"behat/mink-goutte-driver": "~1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I need also behat/mink-goutte-driver:~2.0. Could we update to ~1.0 || ~2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I tried that, but don't remember why I didn't use that, possibly some conflict?

this was merged, so will require a new MR anyway. That said, I think the path forward is to use browserkit directly once FriendsOfBehat/MinkExtension#14 lands. Per https://github.com/minkphp/MinkGoutteDriver/releases/tag/v2.0.0, that's just a thin wrapper and you're supposed to switch over.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Berdir I've created #610 (didn't test locally)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Berdir this is blocking me adopting PHP 8.1

(...) that's just a thin wrapper and you're supposed to switch over.

To switch where? As Drupal Extension have this dependency, I suppose I need to switch here. But I can't find where

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the browserkit driver, as described in the 2.0.0 release notes: https://github.com/minkphp/MinkGoutteDriver/releases/tag/v2.0.0. And FriendsOfBehat/MinkExtension#14 was just merged and released two days ago, so that might be possible now.

I guess that will require changes in this project, but it seems to be the mid/longterm path forward.

Copy link
Contributor

@claudiu-cristea claudiu-cristea Jul 3, 2022

Choose a reason for hiding this comment

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

@Berdir, what you're saying is that we should replace behat/mink-goutte-driver with behat/mink-browserkit-driver because the former is only a wrapper around the latter. Well, I did this in #610 but it seems goutte driver is still requested by friends-of-behat/mink-extension as you can see in the test failure. I don't see anything we can do here, in drupal-extension

EDIT: By mistake I force pushed and the first approach has gone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it seems goutte driver is still requested by friends-of-behat/mink-extension as you can see in the test failure.

friends-of-behat/mink-extension only has a require-dev dependency on goutte-driver.

Not sure what's the earlier and the current MR, but the test fails I see currently in the MR are because of conflicts with older PHP versions (symfony 6 requires PHP 8.1) and drupal 9 vs symfony versions conflicts. Which makes sense, my guess is that this approach won't work until a new major version that can require Drupal 10 or something.

The current state works for me on php 8.1 beside those initial guzzle deprecation messages. I'm not against requiring or maybe additional supporting v2 so that you can pick a matching version depending on drupal and php version, just that it's not a long-term solution.

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.

4 participants