\Drupal\Tests\rest\Functional\ResourceTestBase::assertResourceResponse contains

    if ($expected_status_code < 400) {
      $this->assertSame([static::$mimeType], $response->getHeader('Content-Type'));
    }
    else {
      $this->assertSame([static::$mimeType], $response->getHeader('Content-Type'));
    }

The code is the same for the if and the else block.

CommentFileSizeAuthor
#2 2905181-2.patch1 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
2905181-2.patch1 KB
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Issue tags: +Novice

Marking as 'Novice'.

It looks this problem was introduced in #2805281-10: ?_format=hal_json error responses are application/json, yet should be application/hal+json

The previous block was

if ($expected_status_code < 400) {
      $this->assertSame([static::$mimeType], $response->getHeader('Content-Type'));
    }
    else {
      $this->assertSame([static::$expectedErrorMimeType], $response->getHeader('Content-Type'));
    }

But then in that issue static::$expectedErrorMimeType was no longer needed. So maybe just replace error.

Status: Needs review » Needs work

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

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review

Setting back to needs review. #2 had a totally unrelated error. Not sure why

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Remove unless if statement in ResourceTestBase::assertResourceResponse » Remove useless if statement in ResourceTestBase::assertResourceResponse()
Version: 8.5.x-dev » 8.4.x-dev
Priority: Normal » Minor
Issue tags: +php-novice, +clean-up

Yep, the two branches of that if-test used to be different. As of #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json, that bug in D8 has been fixed. But #2805281 forgot to simplify this code accordingly.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Simplifying code, works for me :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

+1.

I stupidly didn't even notice the patch was ready — based on #3 adding that tag, I assumed it needed work :P

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

@Wim Leers yep sorry I added the tag because I thought a novice could mark it RTBC.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Ahh! :) Np in any case.

  • xjm committed 5ce3293 on 8.5.x 
    Issue #2905181 by tedbow: Remove useless if statement in...

  • xjm committed 4f0c4ac on 8.4.x 
    Issue #2905181 by tedbow: Remove useless if statement in...

xjm credited xjm.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

lol nice. Committed and pushed to 8.5.x and backported to 8.4.x as a test code cleanup. Thanks!

Status: Fixed » Closed (fixed)

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