Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In #3126797: [D8 only] Add forwards-compatibility shim for assertString(Not)ContainsString()replacements in phpunit 6&7 we backported a BC layer to 8.8.x and in #3128746: Replace assertions involving calls to strpos() with more accurate string assertions we improved it in 8.9.x but not in 8.8.x. That leaves us with inconsistencies seen in #3131379: CONCAT_WS requires at least 3 arguments. where a test using assertStringContainsString() passes on 8.9.x but not on 8.8.x.
Proposed resolution
Backport the improvements.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
The other issues already have the release notes about this BC layer.
Comment | File | Size | Author |
---|---|---|---|
#5 | 3135747-5.patch | 3.09 KB | alexpott |
#5 | 3135747-5-8.9.x-and-test-only.patch | 1.31 KB | alexpott |
#5 | 2-5-interdiff.txt | 1.53 KB | alexpott |
#2 | 3135747-2.patch | 2.91 KB | alexpott |
#2 | 3135747.test-only.patch | 1.12 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottNote that the test-only patch should be committed to 8.9.x to keep the tests aligned.
Comment #4
mondrakeGood catch... nitpick - why not just adding new assertions for FormattableMarkup, instead of changing the existing ones?
Comment #5
alexpottSure why not.
Comment #6
mondrakeLGTM
Comment #7
junglea) The first argument of
PHPUnit\Framework\Assert::assertContains()
must be a string, an instance of FormattableMarkup is not acceptableb) The 2nd argument of
PHPUnit\Framework\Assert::assertContains()
must be a array, traversable or string, an instance of FormattableMarkup is not acceptable as well,While writing tests, usually, FormattableMarkup instance is not converted to a string. Before passing to
assertContains()
, casting the $needle and $haystack to string fixed it.No casting to $message works still.
For example
Output
Comment #8
mondrake@jungle I suspect it's a D8.8 only bug because we have introduced a MarkupInterfaceComparator, #3082340: Replace assertEquals() overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests with a MarkupInterfaceComparator implementation, in D8.9 only - but am not sure.
Comment #9
alexpott@jungle this does not need a change record. This is not a change it's a fix. It's only copying the fix from #3128746: Replace assertions involving calls to strpos() with more accurate string assertions to here.
PHP has some very very funky type coercion behaviour. In Drupal 9 when you call \PHPUnit\Framework\Assert::assertStringContainsString() from a file that doesn't have
declare(strict_types=1);
directive (which I think is all Drupal code) then PHP will coerce objects etc... into strings. We need to backport this behaviour to 8.8.x because we've already backported it to 8.8.x.Comment #10
alexpottIt's a D8.8 only bug because we fixed it in 8.9.x in #3128746: Replace assertions involving calls to strpos() with more accurate string assertions
Comment #12
jungle@alexpott, you are right. #9 removing Needs change record from here is reasonable.
Combined with #8 and #10, it makes sense to me now. Thanks, @mondrake and @alexpott!
Comment #15
catchCommitted/pushed the relevant patches to 8.9.x and 8.8.x, good catch.
Comment #17
mondrakeThanks for the explanation in #9, @alexpott. Definitely makes more sense than #8...
Interesting. So the problem is (was) with the methods' signature, rather than anything else. If the method's arguments are typehinted to string, then if a non-string argument is passed in, BUT it IS castable to string, then it is casted to string before execution is handed over to the method. That's why the PHPUnit 7 version of the trait is not failing... because in there, the methods' arguments have a
string
typehint. You'll never stop learning...