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.

CommentFileSizeAuthor
#22 interdiff.txt1.07 KBklausi
#22 mink-pipes-2808085-22.patch8.48 KBklausi
#18 interdiff.txt6.19 KBklausi
#18 mink-pipes-2808085-18.patch8.52 KBklausi
#12 mink-pipes-2808085-12.patch4.16 KBklausi
#10 mink-pipes-2808085-10.patch2 KBklausi
#2 pipe-in-locator.patch2.16 KBclaudiu.cristea
pipe-in-locator.patch2.16 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
FileSize
pipe-in-locator.patch2.16 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

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

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

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

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

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

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Parent issue: » #2807237: PHPUnit initiative
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
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

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

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

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
mink-pipes-2808085-10.patch2 KB

We can just pin the recent commit in the meantime.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs review » Needs work

Yes but let's include the test from #2

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
mink-pipes-2808085-12.patch4.16 KB

Right.

Status: Needs review » Needs work

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

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
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

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Assigned: Unassigned » klausi

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

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

Pff... I missed that :(

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue summary: View changes

Symfony issue: https://github.com/symfony/symfony/pull/20229

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Assigned: klausi » Unassigned
Status: Needs work » Needs review
FileSize
mink-pipes-2808085-18.patch8.52 KB
interdiff.txt6.19 KB

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

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented 
+++ 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

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

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

jmuzz CreditAttribution: jmuzz commented

+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

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
FileSize
mink-pipes-2808085-22.patch8.48 KB
interdiff.txt1.07 KB

Symfony dom crawler has been released.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

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

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs review » Reviewed & tested by the community

Looks good.

catch credited nicolas.grekas.

catch credited stof.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
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

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

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

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +8.3.0 release notes