See #2824851-189: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior. Let's see if this works.

CommentFileSizeAuthor
#7 2929124-7-test-do-not-commit.patch2.56 KBeffulgentsia
#3 2929124-3.patch2.7 KBwim leers
#3 interdiff.txt858 byteswim leers
#2 2929124-2.patch2.7 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2929124-2.patch2.7 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt858 bytes
new 2929124-3.patch2.7 KB

Wim--

The last submitted patch, 2: 2929124-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Took a sneek peak at the test results and they look good. Seems fair enough. Thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker

This blocks #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, so tagging blocker.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new 2929124-7-test-do-not-commit.patch2.56 KB

#3 looks good. I just want to see if we have any test coverage that prevents this version.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Suspicious effulgentsia, I like it :D

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2929124-7-test-do-not-commit.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Reviewed & tested by the community

Great!

effulgentsia’s picture

effulgentsia commented

Removing credit from myself.

  • effulgentsia committed 8203805 on 8.5.x 
    Issue #2929124 by Wim Leers, tstoeckler: Remove EntityResource::...
effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Fixed

Pushed #3 to 8.5.x. Thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This wasn't in my patch but was committed:


diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index 050c7f5..15396fd 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -11,9 +11,7 @@ use Drupal\Core\Entity\FieldableEntityInterface;
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityStorageException;
-use Drupal\Core\Field\FieldItemListInterface;
 use Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException;
-use Drupal\Core\TypedData\PrimitiveInterface;
 use Drupal\rest\Plugin\ResourceBase;
 use Drupal\rest\ResourceResponse;
 use Psr\Log\LoggerInterface;

That's ok, but led to some surprises when rerolling #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior. I thought PHPStorm or git was behaving erratically :)

Status: Fixed » Closed (fixed)

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