Problem/Motivation

While working on #3131186: Replace assertions involving calls to drupalGetHeader() with session-based assertions, where possible, I realized there are cases like

$this->assertSame('HIT', $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'));

that are better covered by direct usage of the appropriate WebAssert method, like in this case would be:

$this->assertSession()->responseHeaderEquals('X-Drupal-Dynamic-Cache', 'HIT');

Regex for finding: getSession\(\)->getResponseHeader\(

Steps to reproduce

Proposed resolution

Replace the instances.

Remaining tasks

User interface changes

no

API changes

no

Data model changes

no

Release notes snippet

no

CommentFileSizeAuthor
#14 3164589-14.patch15.13 KBpaulocs
#14 interdiff-11-14.txt7.91 KBpaulocs
#11 3164589-11.patch7.56 KBankithashetty
#10 interdiff_3164589_7-10.txt3.92 KBankithashetty
#10 3164589-10.patch7.54 KBankithashetty
#7 3164589-7.patch2.88 KBsuresh prabhu parkala
#5 3164589-5.patch2.87 KBs_bhandari

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +Novice
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
s_bhandari’s picture

s_bhandari commented

Hi,

I am working on it. Will update on this as soon as possible.

Thanks.

s_bhandari’s picture

s_bhandari commented
Status: Active » Needs review
StatusFileSize
new 3164589-5.patch2.87 KB

Hi,

Applied a patch for the same. Please review it and let me know for any observation.

Thanks.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work
suresh prabhu parkala’s picture

suresh prabhu parkala commented
Status: Needs work » Needs review
StatusFileSize
new 3164589-7.patch2.88 KB

Please review!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Thanks for working on this. This only covers NodeBlockFunctionalTest so far, a search of the codebase shows there are about another 20 cases to consider.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Assigned: Unassigned » ankithashetty
ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Assigned: ankithashetty » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3164589-10.patch7.54 KB
new interdiff_3164589_7-10.txt3.92 KB

Updated the patch and attached an interdiff along with it. Kindly review the same.

Thank you.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
StatusFileSize
new 3164589-11.patch7.56 KB

Updated the patch. Please review.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

This is looking good but I think there are still some calls we can replace in the following files:

  • modules/system/tests/src/Functional/Routing/RouterTest.php
  • modules/basic_auth/tests/src/Functional/BasicAuthTest.php

Also:

+++ b/core/modules/system/tests/src/Functional/Cache/PageCacheTagsTestBase.php
@@ -69,7 +69,7 @@ protected function verifyPageCache(Url $url, $hit_or_miss, $tags = FALSE) {
     $message = new FormattableMarkup('Dynamic page cache @hit_or_miss for %path.', ['@hit_or_miss' => $hit_or_miss, '%path' => $url->toString()]);
-    $this->assertSame($hit_or_miss, $this->getSession()->getResponseHeader('X-Drupal-Dynamic-Cache'), $message);
+    $this->assertSession()->responseHeaderEquals('X-Drupal-Dynamic-Cache', $hit_or_miss);

$message is now redundant, I think we can just delete this line.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: Unassigned » paulocs

I'll work on it

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-11-14.txt7.91 KB
new 3164589-14.patch15.13 KB

Updated patch with changes from comment #12 and also I did more changes on PageCacheTagsTestBase.php file.

Interdiff is also attached.

Cheers, Paulo.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Thanks, this looks good now. I applied the patch and looked for remaining cases - there are three, but they have to stay as they aren't directly asserting anything:

core/tests/Drupal/Tests/BrowserTestBase.php
669:    return $this->getSession()->getResponseHeader($name);

core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
79:    $content_type = $this->getSession()->getResponseHeader('Content-type');
113:    $content_type = $this->getSession()->getResponseHeader('Content-type');

  • catch committed 421f994 on 9.1.x 
    Issue #3164589 by ankithashetty, paulocs, S_Bhandari, Suresh Prabhu...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.1.x, thanks!

This is eligible for backport to 9.0.x and 8.9.x, but the patch doesn't cherry-pick - however I think it's OK to leave the commit in 9.1.x

Status: Fixed » Closed (fixed)

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