2.2.0 introduced BC breaks

mxr576 commented 3 days ago

I am a strong advocate for type safety and the use of PHPStan in legacy projects. However, I encountered an issue stemming from the recent changes in the MinkBrowserKitDriver library, which introduced a breaking change in a minor version update.

The commit c1c3a2fd143e78ccfe8ec4363d42edcc46e3fdfa narrowed the type from no type to string for several parameters. This change was released in a minor version (2.2.0) instead of a major version, resulting in a backward compatibility (BC) break.

This change has caused test failures in downstream projects, such as Drupal core and contrib/custom modules, which previously worked with version 2.1.0 but now fail with version 2.2.0.

Example of Failure

In Drupal core, the following test fails:

1) Drupal\Tests\.......
Behat\Mink\Exception\DriverException: Only string values can be used for a radio input.

/mnt/files/local_mount/build/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:415
/mnt/files/local_mount/build/vendor/behat/mink/src/Element/NodeElement.php:118
/mnt/files/local_mount/build/web/core/tests/Drupal/Tests/UiHelperTrait.php:96
/mnt/files/local_mount/build/web/core/modules/user/tests/src/Functional/UserRegistrationTest.php:136
/mnt/files/local_mount/build/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

Changes in Downstream Projects

To address this issue, Drupal core has updated its minimum requirement to >=2.2.0 and modified integer method parameters to strings to comply with the new type constraints. You can view the changes made in Drupal core here.

Backward Compatibility Check

Using Roave/BackwardCompatibilityCheck, the following breaking changes were identified:

$ git clone https://github.com/minkphp/MinkBrowserKitDriver.git
$ cd MinkBrowserKitDriver
$ docker run --rm -v `pwd`:/app nyholm/roave-bc-check --from=v2.1.0

[BC] CHANGED: The parameter $baseUrl of Behat\Mink\Driver\BrowserKitDriver#__construct() changed from no type to a non-contravariant string|null
...
[BC] CHANGED: The parameter $xpath of Behat\Mink\Driver\BrowserKitDriver#getFormField() changed from no type to string

(Full list of changes omitted for brevity)

Conclusion

Releasing such significant type changes in a minor version has caused unexpected failures in dependent projects. It would be prudent to release such changes in a major version update to adhere to semantic versioning principles and avoid breaking backward compatibility.

Recommendation In the 2.x branch, trigger user level deprecations for invalid input by using trigger_error() instead of throwing an exception.

mxr576 commented 3 days ago

The Selenium driver and the behat/mink package may also introduced a similar problem.

stof commented 2 days ago

Passing something else than a string value for a radio note was never supported across Mink drivers. In older versions, it was either silently failing or loudly failing or doing a bad thing or doing a "good" thing by luck, depending on the driver being used and the kind of values (passing an integer might have worked by luck thanks to an implicit casting to string happening somewhere).

Note that you don't need to bump the min driver version: version 2.1 of the driver was already accepting strings as valid values (and it was the only kind of values that was tested for radio buttons).

and for parameters like $baseUrl or $xpath, the phpdoc has always documented the supported argument type as being string|null or string respectively. If you were passing values not matching the documented contract, you were not covered by any backward compatibility behavior for those usages.

stof commented 2 days ago

Note that Roave/BackwardCompatibilityCheck does not consider the phpdoc when checking for signature change. It look only at the native PHP signature, which means that any case of migrating from phpdoc argument types to native argument types is considered as a BC break by that tool, while it is a compatible change (when requiring PHP 7.2+ to support variance rules for non-final classes) for any project which considers the phpdoc contract as part of its public API for semver (which is approximately 100% of projects that use phpdoc)

mxr576 commented 2 days ago

(passing an integer might have worked by luck thanks to an implicit casting to string happening somewhere).

Probably this hidden secret gem caused the problem and maybe the only problem, you can be right about the that... implicit casting in PHP is both a bug and a feature. I can also imagine that since at the end of the days ,everything is transferred as string on the wire, the browsers accepted an integer selector as long as it was valid, but now the client side (this PHP lib) does not accept it.

My question is, considering everything, could the 2.x branch restore the original, but unwanted behavior in a "lenient" mode and become fully strict in v3?

You are also right about the Roave/BackwardCompatibilityCheck limitations, I have used it to double check/prove my point, but I also know from experience that it could return false-positives (the length of the list was already surprising to me, but the method that I found problematic were there.)

minkphp / MinkBrowserKitDriver