Followup for #2948073-12: ContentTranslationUITestBase stragglers:
@alexpott:
I think we should back out your changes and file a separate issue about the changes to Mink and perhaps a PR against mink to fix unless this is a major version thing because they've changed their API.
Behat\Mink\WebAssert has next annotations for these methods:
/**
* Checks that page HTML (response content) contains text.
*
* @param string $text
*
* @throws ExpectationException
*/
public function responseContains($text)
/**
* Checks that page HTML (response content) does not contains text.
*
* @param string $text
*
* @throws ExpectationException
*/
public function responseNotContains($text)
However, we have many tests where we pass it not a string, but an object.
Example: all cases with AssertLegacyTrait::assertRaw(t(...)); because assertRaw() works via assertSession()->responseContains().
Why do our tests pass? Because in our current version of Mink there is a casting value inside the methods responseContains/responseNotContains like:
$regex = '/'.preg_quote($text, '/').'/ui';
$this->assert((bool) preg_match($regex, $actual), $message);
But new version of Mink changed (proof) internal processing for this value to:
$this->assert(stripos($actual, $text) !== false, $message);
As result, when once we switch to a new Mink version, we get a many errors in our tests like:
Object of class Drupal\Core\StringTranslation\TranslatableMarkup could not be converted to int
Because stripos casting $text value to string (doc):
If needle is not a string, it is converted to an integer and applied as the ordinal value of a character.
In fact, we do not follow the annotation of a function when we call it with object value. Therefore, there is a suggestion to fix this through a wrapper.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | interdiff-2-5.txt | 784 bytes | Anonymous (not verified) |
| #5 | 2948700-5.patch | 1.77 KB | Anonymous (not verified) |
| #2 | 2948700-2.patch | 1 KB | Anonymous (not verified) |
Comments
Comment #1
Anonymous (not verified) commentedvaplas created an issue. See original summary.
Comment #2
Anonymous (not verified) commentedComment #3
alexpott@vaplas I think a comment should be added to the PR that made the change about the behaviour change. Yes we were relying on implicit rather documented behaviour but they might choose to add a cast and broaden the API.
Comment #5
Anonymous (not verified) commented#3: Thanks for the additional explanation of this point! Done: https://github.com/minkphp/Mink/pull/743#issuecomment-369251178
#4: because on one call deeper for responseContains. Corrected.
Comment #6
dawehnerMany other assertions we use requires you to cast it to a string, if you have some object. What about instead of converting it automatically, throw a deprecation notice/exception so people can adapt tests easily?
Comment #7
Anonymous (not verified) commentedI agree with @aik099, who said about strpos/stripos next:
Even more, this easter egg was made by a misanthrope.
Why functions that work with string, casting objects to int? ¯\_(ツ)_/¯
But we do not need to be a misanthrope. If you want to check the value which is stored in Translate of Markup object, why you should manual casting this?
We also have #2549805: [Meta] Remove all usage of FormattableMarkup in tests apart from explicit tests of that API for remove for removing objects, where their use creates unnecessary complexity.
Comment #8
Anonymous (not verified) commentedOne more pull request https://github.com/minkphp/Mink/pull/760 with same result.
Also #2956279-6: [Policy] Do not use dev-master as a dependency for composer packages also suggests using a special commit instead of the general dev version.
Comment #9
mile23We already wrap WebAssert, so +1 on #5. We might miss out on legitimately passing in ordinal needles, but I think that's just fine.
Comment #11
martin107 commentedI am picking over the various lines of thought here...and I want to point out a contrarian perspective.
From the issue summary
My side point is that inserting a t() call inside a assert() or responseContains() call is a very common legacy pattern in the codebase. It is also directly against drupal's test guide lines which acknowledges that by design all testing is to be done in english unless specifically testing the string translation service itself.
https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial
When I say a common pattern, I can unfortunately put a number on our technical debt ( which I would also grade a minor issue ) there are 330 test failure here ...
https://www.drupal.org/project/drupal/issues/2976394#comment-12757009
They seem to be exclusively related to instances where there is a t() within a test class.
I am still thinking about the appropriate way forward here, but I just thought I should point that out..
Comment #12
martin107 commentedI would like to see the solution from #5 go forward.
I also like #6
But I can't find a good way of getting it to work in the face of a common scenario.
For example an everyday occurrence is an issue that requires code changes that lead to a small amount of extra test code to be added to a existing test file.
If the test file is say more than 150 lines long then the probability of it including an inappropriate t() call is high.
and so when the next developer comes to run the test locally before submitting the patch, then he/she is going to get struck with deprecation notices to fix out of scope work.
I would love to find a good solution to this thorny issue. but I think the only solution is to write a php parsing script which detects and removes t() calls within *Test.php files.
So in short RTBC.
Comment #13
alexpottCrediting @martin107 for comments explaining the thinking process to get to making the issue rtbc.
Comment #14
alexpottWhilst we can remove this once the underlying project moves to PHP7 scalar typehints we're not there yet and this is a helpful for devs.
Committed 1a4a66b and pushed to 8.7.x. Thanks!