Title explains the idea.

Why? It's is an internal field.

Why not? We added the drupal_internal__ prefix to avoid spec conflict with type and id. That conflict doesn't exist here.

Thoughts?

CommentFileSizeAuthor
#19 interdiff-commit.txt2.43 KBgabesullice
#15 3005999-15.patch3.1 KBgabesullice
#15 interdiff.txt1.78 KBgabesullice
#12 3005999-12.patch1.32 KBgabesullice

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented

Perhaps it's not actually an internal field... since there isn't a spec defined key for revision IDs.

This would be a good place to standardize with a profile.

Or maybe it is...

Maybe this issue should be "add a version_id key to resource objects' meta object and alias revision ID fields to drupal_internal__{id}

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Category: Bug report » Task

Perhaps it's not actually an internal field... since there isn't a spec defined key for revision IDs.

Indeed.

This would be a good place to standardize with a profile.

See https://github.com/json-api/json-api/issues/600.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » e0ipso

I see I failed to respond to one part:

Maybe this issue should be "add a version_id key to resource objects' meta object and …

I agree with this. Even if the JSON API spec chooses the exact same name, that won't actually be a problem! So: 👍

… alias revision ID fields to drupal_internal__{id}

I'm not sure about this one yet. As long as it's not yet reserved, this is technically premature. On the other hand, I do agree that it's super similar to the "entity ID" case, especially once we have meta.version_id.

So I'm tentatively +1.

Let's hear what @e0ipso thinks.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I'm good with this for consistency. I'm OK with aliases.

One thought, this impacts correction of auto generated schemas.

One question, is there any impact on the revision propertie/s on the payload?

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: e0ipso » Unassigned

Even if the JSON API spec chooses the exact same name, that won't actually be a problem!

That won't even happen, because version_id has an underscore in it :)

One question, is there any impact on the revision propertie/s on the payload?

,"meta":{"version_id":1234567890}

^ I count 33 bytes. 33 * max 50 resources = 1650 bytes.

drupal_internal__

^ 16 bytes. 16 * max 50 resources = 800 bytes.

So, yes. It does. Likely less with gzip. As with anything I guess it's a trade-off:

The trade-off, though, is that a uniform interface degrades efficiency, since information is transferred in a standardized form rather than one which is specific to an application's needs. The REST interface is designed to be efficient for large-grain hypermedia data transfer, optimizing for the common case of the Web, but resulting in an interface that is not optimal for other forms of architectural interaction.

That's REST, good or bad :shrug:

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

[…] an underscore in it :)

D'oh, of course!

#7: where did that long quote at the end come from? I cannot find it either here nor in https://github.com/json-api/json-api/issues/600.

gabesullice’s picture

gabesullice
Primary language English
commented

#7: where did that long quote at the end come from?

Roy Fielding. I linked as directly to it as I could via the word "REST" in #7.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

D'oh, ofc, I see.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Active
gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: Unassigned » gabesullice
Status: Active » Needs review
StatusFileSize
new 3005999-12.patch1.32 KB

Here's the first salvo. Let's see what breaks.

Status: Needs review » Needs work

The last submitted patch, 12: 3005999-12.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented

While the test ran, I drafted a CR: https://www.drupal.org/node/3008622

gabesullice’s picture

gabesullice
Primary language English
commented
Assigned: gabesullice » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.78 KB
new 3005999-15.patch3.1 KB

Alright, here we update the test expectations.

I'm not going to add the version_id key to any resource object's meta members in this issue.

I think that should be a separate issue as part of the versioning epic. Until we support revisions, it would be useless extra data.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

I think that should be a separate issue as part of the versioning epic. Until we support revisions, it would be useless extra data.

Agreed.

This still needs a change record.

Per #6, @e0ipso supports this change. So RTBC'ing.

Two übernits though:

  1. +++ b/tests/src/Functional/BlockContentTest.php
@@ -110,7 +110,7 @@ class BlockContentTest extends ResourceTestBase {
-          'revision_id' => 1,
+          'drupal_internal__revision_id' => 1,

    I'd move this line to sit before or after drupal_internal__id, so that they're visually close. 

  2. +++ b/tests/src/Functional/MediaTest.php
@@ -153,7 +153,7 @@ class MediaTest extends ResourceTestBase {
-          'vid' => 1,
+          'drupal_internal__vid' => 1,

    I'd move this line to sit before or after drupal_internal__mid, so that they're visually close. 

  3. +++ b/tests/src/Functional/NodeTest.php
@@ -149,7 +149,7 @@ class NodeTest extends ResourceTestBase {
+          'drupal_internal__vid' => 1,
           'drupal_internal__nid' => 1,

    Like this :)

  • gabesullice committed 8ed0986 on 8.x-2.x 
    Issue #3005999 by gabesullice, Wim Leers, e0ipso: Revision ID should be...
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

This still needs a change record.

Nope. See #14.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new interdiff-commit.txt2.43 KB

I fixed your nits on commit.

Status: Fixed » Closed (fixed)

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