Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 1935538-19-rest-update.patch | 23.33 KB | effulgentsia |
#18 | 1935538-18-rest-update.patch | 20.54 KB | effulgentsia |
#17 | 1935538-17-rest-update.patch | 22.78 KB | effulgentsia |
#17 | interdiff.txt | 2.32 KB | effulgentsia |
#16 | 1935538-16-rest-update.patch | 22.78 KB | linclark |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll since info.yml got in.
Comment #3
klausiwhy 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!
Comment #4
Crell CreditAttribution: Crell commentedIf 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.)
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll 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).
Comment #6
Crell CreditAttribution: Crell commentedThe 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.
Comment #7
webchick#5: 1935538-05-rest-test-update.patch queued for re-testing.
Comment #9
xjmProbably just need to upload the
.txt
file in #5 with a.patch
extension.Comment #11
xjmxpost.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedChanges in the REST tests since I posted this, have to reroll.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedHere'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.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedJust a silly typo fix.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedAgreed. Done here. This changes nearly every line so no point in an interdiff.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for fixing tests
Comment #21
webchickOh, 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!
Comment #22
webchickActually. 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.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedShouldn't the change notice be for #1935548: Remove JSON-LD module?
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedMoved the change notice requirement to #1816354-43: Add a REST module, starting with DELETE.