Posted by linclark on March 6, 2013 at 8:32pm
13 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | rest.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | WSCCI |
Issue Summary
As explained in Proposing an alternative to application/vnd.drupal.ld+json, the new plan is to use HAL as the primary format for REST.
Comments
#1
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.
#2
Reroll since info.yml got in.
#3
+++ 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!
#4
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.)
#5
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).
#6
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.
#7
#5: 1935538-05-rest-test-update.patch queued for re-testing.
#8
The last submitted patch, 1935538-05-rest-test-update.patch, failed testing.
#9
Probably just need to upload the
.txtfile in #5 with a.patchextension.#10
The last submitted patch, 1935538-05-rest-test-update.patch, failed testing.
#11
xpost.
#12
Changes in the REST tests since I posted this, have to reroll.
#13
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.
#14
The last submitted patch, 1935538-13-rest-update.patch, failed testing.
#15
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.
#16
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.
#17
Just a silly typo fix.
#18
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.
#19
Agreed. Done here. This changes nearly every line so no point in an interdiff.
#20
Thanks for fixing tests
#21
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!
#22
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.
#23
Shouldn't the change notice be for #1935548: Remove JSON-LD module?
#24
Moved the change notice requirement to #1816354-43: Add a REST module, starting with DELETE.
#25
Automatically closed -- issue fixed for 2 weeks with no activity.