assertStringContainsString() and related BC layer in 8.8.x does not work as expected [#3135747]

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	8.8.x: PHP 7.1 & MySQL 5.7 28,287 pass
#5	3135747-5-8.9.x-and-test-only.patch	1.31 KB	alexpott
#5	8.8.x: PHP 7.1 & MySQL 5.7 28,282 pass, 2 fail 8.9.x: PHP 7.1 & MySQL 5.7 28,493 pass
#5	2-5-interdiff.txt	1.53 KB	alexpott
#2	3135747-2.patch	2.91 KB	alexpott
#2	8.8.x: PHP 7.1 & MySQL 5.7 28,287 pass
#2	3135747.test-only.patch	1.12 KB	alexpott
#2	8.8.x: PHP 7.1 & MySQL 5.7 28,282 pass, 2 fail 8.9.x: PHP 7.1 & MySQL 5.7 28,493 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

12 May 2020 at 09:23

alexpott created an issue. See original summary.

Comment #2

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 12 May 2020 at 09:29

Status:

Active

» Needs review

File	Size
3135747.test-only.patch	1.12 KB
8.8.x: PHP 7.1 & MySQL 5.7 28,282 pass, 2 fail 8.9.x: PHP 7.1 & MySQL 5.7 28,493 pass
3135747-2.patch	2.91 KB
8.8.x: PHP 7.1 & MySQL 5.7 28,287 pass

Comment #3

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 12 May 2020 at 09:30

Note that the test-only patch should be committed to 8.9.x to keep the tests aligned.

Comment #4

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 12 May 2020 at 09:49

Good catch... nitpick - why not just adding new assertions for FormattableMarkup, instead of changing the existing ones?

File	Size
2-5-interdiff.txt	1.53 KB
3135747-5-8.9.x-and-test-only.patch	1.31 KB
8.8.x: PHP 7.1 & MySQL 5.7 28,282 pass, 2 fail 8.9.x: PHP 7.1 & MySQL 5.7 28,493 pass
3135747-5.patch	3.09 KB
8.8.x: PHP 7.1 & MySQL 5.7 28,287 pass

Comment #6

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 12 May 2020 at 10:00

Status:

Needs review

» Reviewed & tested by the community

LGTM

Comment #7

jungle

He/Him

Chongqing, China

CreditAttribution: jungle as a volunteer commented 12 May 2020 at 10:20

Issue tags:

+Needs change record

I have no idea why this is an 8.8.x only bug. The test only patch should pass against 8.9.x, and fail against 8.8.x, so the following comments are all 8.8.x related
a) The first argument of PHPUnit\Framework\Assert::assertContains() must be a string, an instance of FormattableMarkup is not acceptable

b) 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

$this->assertStringContainsString("bingofoo", new FormattableMarkup("foobarbingobongo", []), new FormattableMarkup("Foo error message", []) );

Output

There was 1 failure:

1) Drupal\Tests\PhpunitCompatibilityTraitTest::testAssertStringContainsString
Foo error message
Failed asserting that 'foobarbingobongo' contains "bingofoo".

#3126797: [D8 only] Add forwards-compatibility shim for assertString(Not)ContainsString()replacements in phpunit 6&7 has a "Needs change record" tag, so I am adding this tag here as well.

Comment #8

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 12 May 2020 at 10:27

@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

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 12 May 2020 at 10:37

Issue tags:

-Needs change record

@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

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 12 May 2020 at 10:38

It'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 #11

12 May 2020 at 10:40

The last submitted patch, 2: 3135747.test-only.patch, failed testing. View results

Comment #12

jungle

He/Him

Chongqing, China

CreditAttribution: jungle as a volunteer commented 12 May 2020 at 11:01

@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 #13

12 May 2020 at 11:11

The last submitted patch, 5: 3135747-5-8.9.x-and-test-only.patch, failed testing. View results

Comment #14

12 May 2020 at 12:18

catch committed c5f26ed on 8.9.x

Issue #3135747 by alexpott, mondrake, jungle: assertStringContainsString...

Comment #15

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 12 May 2020 at 12:21

Status:

Reviewed & tested by the community

» Fixed

Committed/pushed the relevant patches to 8.9.x and 8.8.x, good catch.

Comment #16

12 May 2020 at 12:23

catch committed a672fd1 on 8.8.x

Issue #3135747 by alexpott, mondrake, jungle: assertStringContainsString...

Comment #17

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 12 May 2020 at 12:44

Thanks 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...