Update behat/mink and friends

Don't know why yet but I am not able to create the MR for this.

By no means meant to overtake your (not) MR, but I saw the button "Create MR" and clicked it, seems to work now?

I have updated the .json file and added patch, please review.

@Spokje. thank you! It turns out that I was not logged into GitLab. I haven't had that particular problem before and didn't even check it.

Thanks @viren18febS, but this issue is not only about updating the version, but also fixing the test-failures that come with the updates.
Also we very much prefer Merge Requests nowadays.

Just noting the failure

   1)
    Drupal\Tests\block\FunctionalJavascript\BlockDragTest::testDragAndDropBlocks
    Main menu should be positioned on header region
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'header'
    +'primary_menu'
    
    /builds/issue/drupal-3407834/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
    /builds/issue/drupal-3407834/core/modules/block/tests/src/FunctionalJavascript/BlockDragTest.php:52
    /builds/issue/drupal-3407834/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Not sure what is up with BlockDragTest.
Guessing it is related to the upgrade of the Syn library in the Selenium driver: https://github.com/minkphp/MinkSelenium2Driver/releases/tag/v1.7.0

BC break (when doing custom logic with syn):

    syn JS library was upgraded from v0.0.3 to v0.15.0

Syn is used for drag operations, we have had issues with it before.

@longwave in #3405696-#31/32 📌 Update composer dependencies for Drupal 10.2.0 Active

Looks like this either fails because of we're trying to drag something that isn't in the viewport and/or there's something special in Olivero that makes this fail.

I've found that the first dragTo always fails, but after the the exact same one will succeed.
Messed around a bit with trying to scroll elements into view, but couldn't find the right element to focus on and couldn't get it to work.

Also that would add to the fragility of the test, which isn't great.

I ended up converted the test to use the stark theme, which works.

If there are any special reasons we _should_ test this with the olivero, I feel that should be an extra test and living in the olivero namespace.

Thanks for looking at BlockDragTest. It was added quite recently in 🐛 Javascript breaking on block layout page after drag or select new region RTBC , perhaps we can revert the fix and check that the test fails as expected on Stark?

There is also some "interesting" code in DrupalSelenium2Driver::dragTo():

    try {
      parent::dragTo($sourceXpath, $destinationXpath);
    }
    catch (Exception $e) {
      // Do not care if this fails for any reason. It is a source of random
      // fails. The calling code should be doing assertions on the results of
      // dragging anyway. See upstream issues:
      // - https://github.com/minkphp/MinkSelenium2Driver/issues/97
      // - https://github.com/minkphp/MinkSelenium2Driver/issues/51
    }

The upstream issues are related to the Syn library so perhaps we should try removing this block again, hiding exceptions isn't always great and I assume makes tests that use dragTo() harder to debug.

Full support for removing the empty catch, however I also found that code and did remove it locally.
No exceptions did bubble up :/

perhaps we can revert the fix and check that the test fails as expected on Stark?

When I run BlockDragTest

- on 11.x c09c0e5, so one commit before the fix, with manually added BlockDragTest
- on the latest commit of this MR, with reverted block.js

I both get a:

Testing Drupal\Tests\block\FunctionalJavascript\BlockDragTest
F                                                                   1 / 1 (100%)

Time: 00:30.537, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\block\FunctionalJavascript\BlockDragTest::testDragAndDropBlocks
TypeError: rowObject.matches is not a function
    at updateLastPlaced (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:138:24)
    at tableDrag.row.onSwap (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:175:9)
    at Drupal.tableDrag.row.swap (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:1500:10)
    at Drupal.tableDrag.dragRow (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:815:28)
    at HTMLDocument.<anonymous> (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:263:12)
    at HTMLDocument.dispatch (http://drupal.local/core/assets/vendor/jquery/jquery.min.js?v=3.7.0:2:39997)
    at v.handle (http://drupal.local/core/assets/vendor/jquery/jquery.min.js?v=3.7.0:2:37968)
TypeError: rowObject.matches is not a function
    at updateLastPlaced (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:138:24)
    at tableDrag.row.onSwap (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:175:9)
    at Drupal.tableDrag.row.swap (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:1500:10)
    at Drupal.tableDrag.dragRow (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:815:28)
    at HTMLDocument.<anonymous> (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:263:12)
    at HTMLDocument.dispatch (http://drupal.local/core/assets/vendor/jquery/jquery.min.js?v=3.7.0:2:39997)
    at v.handle (http://drupal.local/core/assets/vendor/jquery/jquery.min.js?v=3.7.0:2:37968)
TypeError: rowObject.matches is not a function
    at updateLastPlaced (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:138:24)
    at tableDrag.row.onSwap (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:175:9)
    at Drupal.tableDrag.row.swap (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:1500:10)
    at HTMLTableRowElement.<anonymous> (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:108:25)
    at Function.each (http://drupal.local/core/assets/vendor/jquery/jquery.min.js?v=3.7.0:2:3129)
    at ce.fn.init.each (http://drupal.local/core/assets/vendor/jquery/jquery.min.js?v=3.7.0:2:1594)
    at checkEmptyRegions (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:97:41)
    at tableDrag.row.onSwap (http://drupal.local/core/modules/block/js/block.js?v=11.0-dev:174:9)
    at Drupal.tableDrag.row.swap (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:1500:10)
    at Drupal.tableDrag.dragRow (http://drupal.local/core/misc/tabledrag.js?v=11.0-dev:815:28)

D:\htdocs\drupal\core\tests\Drupal\FunctionalJavascriptTests\WebDriverTestBase.php:134
D:\htdocs\drupal\vendor\phpunit\phpunit\src\Framework\TestResult.php:728

Which is the the last bulletpoint of the Steps to reproduce of 🐛 Javascript breaking on block layout page after drag or select new region RTBC .

This leaves me fully unsure and what exactly the original BlockDragTest was testing...

Left 2 remarks/questions in the MR (most of the code in there isn't mine).

Left 2 remarks/semi-answers on questions by @longwave in the MR.

Thanks for the quick turn-around @longwave.
Opened 📌 Deprecate "helpers" for WebAssert::responseHeaderEquals and UiHelperTrait::submitForm Active as a follow-up. Closed my threads.

Looks like it needs @mondrake input as one who managed this methods before

Couple of comments inline in the MR.

Thanks @mondrake, added some comments/changes on your comments.

Replied, I think if we need to do the BC dance, better deprecate immediately the override, and convert core usages to ::responseHeaderDoesNotExist() here in this issue.

See inline comment

I don't think this is going to land in 10.2.0, but @catch and I agreed that in this case at least we can deprecate in 10.3.0 for removal in 11.0.0.

See inline comment comment.

Can we agree on scoping of this issue and in possible follow-ups?

Doing all the deprecations in here would make the MR rather big, with all minimal changes, but the bulk of it would make it harder to review.

If we want to do deprecations in follow-ups, do we want a follow-up for each of them?
I see three of them currently:

1. Drupal\Tests\WebAssert::responseHeaderEquals($name, $value) with NULL as the $value argument.
2. Setting a checkbox with a non-boolean value in \Drupal\Tests\UiHelperTrait::submitForm
3. Setting text, number and radio button with a non-string value in \Drupal\Tests\UiHelperTrait::submitForm

Sure.

IMHO, only #28.1 needs to be done here, otherwise you wouldn't be able to bump the dependency. This way I'd set this to critical so we still have a chance of getting it in 10.2.

The rest can go separately with their own pace, in 10.3 or the future.

IMHO we don't need any deprecation to get this in.

The BC-layers present in fb454385b061a65207b8ae3698ce59198fe3fcd3 would keep us afloat, and we can add deprecations later, IMHO-2 we give each of the three a separate follow-up.

But the main concern seems to be if the core committers want this in 10.2.0, so let's await their guidance?

Sorry, I missed #26. If it's all 10.3 stuff, then back to normal. I still think #28.1 needs to be done here, though, and the rest separately. We'd be adding an override that we do not really want, so better have it deprecated upfront.

We'd be adding an override that we do not really want, so better have it deprecated upfront.

I would argue that we're adding two overrides and discovered one that should have been fixed long ago.

With the "pressure" of the 10.2.0 release gone, and looking at the amount of changes, although most of them are small/trivial, we get this in without deprecations and immediately start working on the three deprecations to minimize the sheer amount of changed code lines to review.

But basically this is all just minor scoping details, so open to any way we can get the update _and_ all the deprecations in shortly.

upgraded deprecation to 10.3

I agree with #28 we should to do the minimum here and maintain maximum compatibility for tests. I agree that postponing deprecations to a follow-up makes a lot of sense.

Given the how some of the typehinting has changed in behat/mink 1.11.0 it's not possible to wrap everything in a deprecation but we should do a best effort.

I've opened 📌 Update Behat from 1.10 to 1.11 Active to do the deprecations and I will create a new MR based on the all the previous work on this issue and close the two previous MR that it is built from. This will allow core-dev-pinned to get to the same version as core-dev and allow the W3C issue(s) to proceed without having to do to this upgrade too.

alexpott → closed merge request !5759

alexpott → closed merge request !6554

Creditting the authors of the previous MRs.

lgtm!

This looks great, agreed with deferring the deprecations to a follow-up. Committed/pushed to 11.x, thanks!

My fault - forgot to composer install before running phpstan...

Committed/pushed to 11.x, thanks!

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

I dared to open this issue last night because it seems to me the way type enforcement arrived to a minor release of the behat/* libraries weren't BC. I am surprised that nobody else reported this before and tried to convince maintainers to change their solution.

Why it is important? Because we would like to support 10.2.x and 10.3.x at the same time, but tests fails when the newer version of behat/mink is installed. It is good that this problem was mitigated in Drupal core, but contrib/custom space is still impacted. https://drupal.slack.com/archives/C223PR743/p1719331712444049

Tagging based on a comment by xjm in committer slack to consider.