Problem/Motivation

This bug came out in #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits.

TL;DR Mink and Symfony cannot search for elements that contain a pipe ('|') in locator value.

See
https://github.com/minkphp/Mink/pull/720
https://github.com/symfony/symfony/pull/20229

Proposed resolution

If they will fix this upstream, just refresh composer.lock

Otherwise, decide a workaround.

Remaining tasks

Fix Symfony.

User interface changes

None.

API changes

None.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

FileSize
2.16 KB
dawehner’s picture

Should we introduce a workaround to not use pipe operators in random strings for now?

Status: Needs review » Needs work

The last submitted patch, 2: pipe-in-locator.patch, failed testing.

mpdonadio’s picture

#3, would this cause a regression where we explicitly need pipes? Option value/label pairs come to mind.

claudiu.cristea’s picture

Where we need pipes, we explicitly add them. Random string generator doesn't guarantee a pipe.

klausi’s picture

Parent issue: » #2807237: PHPUnit initiative
klausi’s picture

Issue summary: View changes

While we wait on feedback in the upstream project I have forked Mink on Github and included that in our PHPUnit initiative feature branch https://github.com/klausi/drupal/commit/9db23a28bfb26ca1bf82e95e5fd6d157...

claudiu.cristea’s picture

Great that https://github.com/minkphp/Mink/pull/720 was merged. But we still have to wait for a new release. I guess, commit pinned versions are not allowed in our composer.json (?)

EDIT: The last version 1.7.1 was released in March, so I guess a new one should be under way.

klausi’s picture

Status: Needs work » Needs review
FileSize
2 KB

We can just pin the recent commit in the meantime.

claudiu.cristea’s picture

Status: Needs review » Needs work

Yes but let's include the test from #2

klausi’s picture

Status: Needs work » Needs review
FileSize
4.16 KB

Right.

Status: Needs review » Needs work

The last submitted patch, 12: mink-pipes-2808085-12.patch, failed testing.

klausi’s picture

Title: Pipe char in locators break Mink element search » Pipe char in locators break Mink and Symfony element search

Argl, the tests are failing because the Symfony Crawler class has the same problem as Mink. We use the Symfony Crawler in the Goutte Driver backend of our browser tests. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Dom...

So we will also have to patch Symfony :(

klausi’s picture

Assigned: Unassigned » klausi

I will try to come up with a pull request against Symfony copying the approach from Mink.

claudiu.cristea’s picture

Pff... I missed that :(

klausi’s picture

klausi’s picture

Assigned: klausi » Unassigned
Status: Needs work » Needs review
FileSize
8.52 KB
6.19 KB

The issue for symfony got committed, so this should now finally pass.

claudiu.cristea’s picture

+++ b/core/composer.json
@@ -8,6 +8,7 @@
+        "symfony/dom-crawler": "2.8.x-dev",

@@ -35,7 +36,7 @@
-        "behat/mink": "~1.7",
+        "behat/mink": "1.7.x-dev",

Hm. I don't think we can commit this to core. These are not even pinned to a commit. So, if somebody will run composer update on his project will get new unstable versions. Do we have some policy in place related to composer.json?

klausi’s picture

The versions are exactly pinned in composer.lock. I think it is fine to commit this for now n 8.3.x until a new stable version of Mink and Symfony is released and then update to that in composer.json.

If you run composer update you will get many updates right now anyway.

jmuzz’s picture

+1 for committing it now with @klausi's plan.

INSTALL.txt does instruct git users to use composer install. UPDATE.txt doesn't have corresponding instructions for people using git to obtain drupal. Perhaps as a followup instructions could be added to UPDATE.txt explaining that composer install should be used after pulling new code and not composer update. That way people won't end up using versions that weren't tested against drupal core. I have been using composer update until now though and it hasn't been causing me any problems.

klausi’s picture

FileSize
8.48 KB
1.07 KB

Symfony dom crawler has been released.

klausi’s picture

Opened https://github.com/minkphp/Mink/issues/725 to get a Mink release.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

catch credited stof.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Do we use dom-crawler anywhere other than testing? If so it could move to require-dev maybe - not an issue having dev versions there.

I agree this is OK for 8.3.x though, let's make sure we update both once there are tagged releases.

I added commit credit for stof and nicolas.grekas here too.

  • catch committed 5a88934 on 8.3.x
    Issue #2808085 by klausi, claudiu.cristea, stof, nicolas.grekas: Pipe...
dawehner’s picture

I'm quite happy how Drupal more and more fixes stuff upstream. Thank you a lot @klausi and @claudiu.cristea

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +8.3.0 release notes