This probably would not happen on DrupalCI

But this code in \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase
$this->assertSame([str_replace($this->entity->id(), static::$firstCreatedEntityId, $this->entity->toUrl('canonical')->setAbsolute(TRUE)->toString())], $response->getHeader('Location'));

causes a error for me locally

1) Drupal\Tests\rest\Functional\EntityResource\User\UserJsonAnonTest::testPost
Failed asserting that Array &0 (
0 => 'http://www.local.dev/d8_3_rest/user/4'
) is identical to Array &0 (
0 => 'http://www.local.dev/d8_4_rest/user/4'
).

This is because "3" is replaced in "d8_3_rest" where it should only be replaced in "user/3"

CommentFileSizeAuthor
#13 2834848-13.patch1.68 KBtedbow
#9 interdiff-3-9.txt1.17 KBtedbow
#9 2834848-9.patch2.04 KBtedbow
#3 2834848-3.patch2.05 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 as a volunteer commented
Issue summary: View changes
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2834848-3.patch2.05 KB

Here is a fix the problem happens twice in the same function.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented 
  1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1086,4 +1086,18 @@ protected function assert406Response(ResponseInterface $response) {
+   * Creates a canonical URL for specific entity id.
...
+   *   The entity id.

    s/id/ID/ 

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1086,4 +1086,18 @@ protected function assert406Response(ResponseInterface $response) {
+   * @return string The absolute canonical URL.
+   * The absolute canonical URL.

    Needs newline before it.

    Trailing comment must be removed.
    Comment below is not indented correctly. 

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1086,4 +1086,18 @@ protected function assert406Response(ResponseInterface $response) {
+    return substr_replace($this->entity->toUrl('canonical')
+      ->setAbsolute(TRUE)
+      ->toString(), $entity_id, -1, strlen($entity_id));

    The formatting here makes it hard to see what's going on. Let's change it to

    $url = …
return substr_replace(…)
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work

Great catch BTW!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Version: 8.3.x-dev » 8.2.x-dev

Also affects 8.2.x.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method

Introduced in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
2834848-9.patch2.04 KB
interdiff-3-9.txt1.17 KB

@Wim Leers thanks for review. This patch address all issues in #5

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1086,4 +1086,18 @@ protected function assert406Response(ResponseInterface $response) {
+   * @param string $entity_id

Super nit: string|int, per \Drupal\Core\Entity\EntityInterface::id().

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -618,7 +618,7 @@ public function testPost() {
+    $this->assertSame([$this->createCanonicalUrlForId(static::$firstCreatedEntityId)], $response->getHeader('Location'));

@@ -640,7 +640,7 @@ public function testPost() {
+    $this->assertSame([$this->createCanonicalUrlForId(static::$secondCreatedEntityId)], $response->getHeader('Location'));

So I wouldn't create a new method. And the whole substr thing here is strange. How about just doing (for example)...

    $location = $this->entityStorage->load(static::$firstCreatedEntityId)->toUrl('canonical')->setAbsolute(TRUE)->toString();
    $this->assertSame([$location], $response->getHeader('Location'));
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work

#11 makes a great point. I also don't see a reason why we can't just do that instead. Much simpler.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
2834848-13.patch1.68 KB

OK here is @alexpott's suggestion for #11. Thanks!

No interdiff because new lines are completely different.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

That totally makes sense. Nice simplification!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2834848-13.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Nice fix!

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • xjm committed ce118a0 on 8.4.x 
    Issue #2834848 by tedbow, Wim Leers, alexpott: REST test fails depending...

  • xjm committed a8ef94e on 8.3.x 
    Issue #2834848 by tedbow, Wim Leers, alexpott: REST test fails depending...
xjm’s picture

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

Nice clean solution. Committed to 8.4.x, and backported to 8.3.x as a testing bugfix. Thanks everyone!

Status: Fixed » Closed (fixed)

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