Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Component: base system » rest.module
Status: Active » Needs review
Issue tags: +WSCCI
FileSize
21.41 KB
66.88 KB

This patch depends on #1924220: Support serialization in hal+json and #1931976: Support deserialization for hal+json (needs more language handling tests).

It also required a few small changes in the REST module. Specifically, on PATCH it ensures that the id and uuid properties from the new entity do not overwrite those of the original.

Anonymous’s picture

Reroll since info.yml got in.

klausi’s picture

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
@@ -155,6 +155,11 @@ public function patch($id, EntityInterface $entity) {
+      // Requests cannot overwrite id or uuid, so skip them.
+      if (in_array($name, array('id', 'uuid'))) {
+        continue;
+      }

why is the UUID protected from writing? I can see that most use cases will never do that, but why artificially restricting it?

Otherwise this looks good!

Crell’s picture

If we can't rely on uuid not changing, we can't build any functionality based off of uuid being, er, universally unique. If you want an object to have a different uuid... make a new object. (If we allow it, someone somewhere is going to change it and then we'll never be able to rely on the uuid again.)

Anonymous’s picture

Reroll due to changes made in the REST test and HAL serialization getting in.

The .patch file includes the latest patch from #1931976: Support deserialization for hal+json (needs more language handling tests).

Crell’s picture

The for-review patch is dead simple and seems fine. If anything, I'd ask if we should be centralizing our definition of default so that we don't have to restate it in 50 places.

The dependent issue is still CNW, though, so we can't RTBC this one.

webchick’s picture

Issue tags: -WSCCI

#5: 1935538-05-rest-test-update.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI

The last submitted patch, 1935538-05-rest-test-update.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Probably just need to upload the .txt file in #5 with a .patch extension.

Status: Needs review » Needs work

The last submitted patch, 1935538-05-rest-test-update.patch, failed testing.

xjm’s picture

xpost.

Anonymous’s picture

Changes in the REST tests since I posted this, have to reroll.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
23.34 KB

Here's a partial reroll, it still fails one of the tests. I won't have a chance to come back to it until this evening.

Status: Needs review » Needs work

The last submitted patch, 1935538-13-rest-update.patch, failed testing.

Anonymous’s picture

I think I might need Klaus to take a look at these fails. I don't see a clear reason why in these three instances, switching to HAL would give a 500 instead of a 403, but it might be more obvious to him.

Anonymous’s picture

FileSize
22.78 KB

The last patch reverted one of eff's fixes from the deserialization patch, so this just reintroduces that change. It doesn't fix the test fails, eff said he would take a look.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
22.78 KB

Just a silly typo fix.

effulgentsia’s picture

Status: Needs review » Needs work
FileSize
20.54 KB

This just removes hunks unrelated to this issue. I moved them into #1931976-28: Support deserialization for hal+json (needs more language handling tests). I'll follow up with one more reroll for #6.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
23.33 KB

If anything, I'd ask if we should be centralizing our definition of default so that we don't have to restate it in 50 places.

Agreed. Done here. This changes nearly every line so no point in an interdiff.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing tests

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oh, JSON-LD. We blow a trumpet for thee. Do we need another patch to remove the module as a whole?

In the meantime, this looks like it cleans up an awful lot of hard-coding as well as switching to a more recognized(?) format, so woohoo.

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title: Switch REST to default to HAL » Change notice: Switch REST to default to HAL
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Actually. I imagine this will require a change to a change notice somewhere, else a new one to be made if it doesn't already exist.

Anonymous’s picture

Shouldn't the change notice be for #1935548: Remove JSON-LD module?

effulgentsia’s picture

Title: Change notice: Switch REST to default to HAL » Switch REST to default to HAL
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Moved the change notice requirement to #1816354-43: Add a REST module, starting with DELETE.

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