Problem/Motivation

The optional $ignoreCase parameter of assertContains() is deprecated and will be removed in PHPUnit 9.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3126848-10.patch4.62 KBmondrake
#9 3126848-9.patch4.62 KBmondrake
#4 3126848-4.patch4.55 KBmondrake
#2 3126848-2.patch2.6 KBmondrake

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
StatusFileSize
new 3126848-2.patch2.6 KB

At least in one test, the parameter seems just redundant given the usage of strlower.

Status: Needs review » Needs work

The last submitted patch, 2: 3126848-2.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 3126848-4.patch4.55 KB
daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All test code changes look good to me.
By removing the the 2 lines from the ignored warnings that are thrown by PHPUnit 8, we have testing that all cases of the use of the parameter are removed.
For me it is RTBC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
@@ -258,7 +258,7 @@ public function testPageCache() {
+    $this->assertContains('cookie', explode(',', strtolower($this->drupalGetHeader('Vary'))), 'Vary header was sent.');

@@ -274,7 +274,7 @@ public function testPageCache() {
+    $this->assertStringNotContainsStringIgnoringCase('cookie', $this->drupalGetHeader('Vary'), 'Vary: Cookie header was not sent.');

These are functionally the same except one is inverted, so I think we should pick one way and use it consistently - using assertStringContainsStringIgnoringCase() in the first seems cleaner as there is an explicit method for it.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#6 actually there is a subtle difference - we want to check positively there is a 'cookie' substring in the main string, but also that is bound by commas, or first or last part of the string. So we explode by ',' and check there is one token which equals 'cookie'.

On the negative check, that is irrelevant since it's enough no instance of the 'cookie' subtring exists.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll... and it's more than just core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 3126848-9.patch4.62 KB

Rerolled. The assertStringContainsString patch missed to figure out these were to be case insensitive. Probably because
the string under test is just emtpy, anyway.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 3126848-10.patch4.62 KB

Needed another reroll.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Re #7 I agree there is a subtle difference but if the exact comma separated string "cookie" is the thing under test shouldn't it be tested the same in both cases? It is probably irrelevant for the test in question and also perhaps out of scope to change here though!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 125b33ab76 to 9.1.x and c58a76feaa to 9.0.x. Thanks!

  • alexpott committed 125b33a on 9.1.x 
    Issue #3126848 by mondrake, longwave, daffie: Replace usage of the $...

  • alexpott committed c58a76f on 9.0.x 
    Issue #3126848 by mondrake, longwave, daffie: Replace usage of the $...

Status: Fixed » Closed (fixed)

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