Updated: Comment #0
Problem/Motivation
The documentation for EntityType::$links reads as follows:
Link templates using the URI template syntax.
Links are an array of standard link relations to the URI template that should be used for them. Where possible, link relationships should use established IANA relationships rather than custom relationships.
Um... come again? I have no idea what "Links are an array of standard link relations to the URI template that should be used for them" means. I also shouldn't have to google "IANA relationships."
The change notice is slightly less opaque:
https://drupal.org/node/2020491
Proposed resolution
Let's please rewrite this whole docblock so that it's comprehensible to mere mortals.
- First of all, the first line should be a lot more about "what the heck is this for" with less implementation detail. Maybe something like:
Link patterns for the entity.
- Then, we can go on to explain that they use the "URI template syntax" (and define WTF that is or link a definition; I'm assuming it means the curlies and crap for arguments in the router?)
- IANA should also have some
@link
action to the same reference the change notice uses. - The current second paragraph needs a serious rewrite to make sense.
- An example near the top of the docblock (perhaps with
node/{node}
,node/{node}/edit
, etc.) would go a long way to illustrating the important point: This is the only way Drupal knows where the heck your entities are and how to perform operations on them.
Comment | File | Size | Author |
---|---|---|---|
#29 | 2047283-29-entity-link-docs.patch | 2.93 KB | linclark |
#29 | interdiff.txt | 1.59 KB | linclark |
#26 | 2047283-EntityTypeLinksDoc-26.patch | 2.63 KB | kattekrab |
#26 | interdiff-2047283-17-26.txt | 3.73 KB | kattekrab |
#17 | 2047283-EntityTypeLinksDoc-17.patch | 2.61 KB | kattekrab |
Comments
Comment #1
jhodgdon+1 to these suggested improvements.
I like the proposed first line, but maybe it should also say what the links are for, such as "Link patterns for viewing and administering the entity."?
I think it would also be helpful if besides describing "canonical" and "edit-form", it also listed the other types of link patterns that are supported by Drupal Core. This could be a list.
Comment #2
xjm#1 all sounds good to me.
Some of the documentation here will require some research and/or consulting with people who worked on this code originally, but it should be straightforward to roll the initial patch.
Comment #3
jhodgdonUm. How does a novice contributor figure out which link patterns from the very long list at http://www.iana.org/assignments/link-relations/link-relations.xml are actually used/supported in Core?
Comment #4
xjmI don't expect them to. ;) But a novice contributor can start a work-in-progress patch with my suggestions from the OP and your reply.
Comment #5
kattekrab CreditAttribution: kattekrab commentedOk - I'm diving in here and will give this a go.
Comment #6
kattekrab CreditAttribution: kattekrab commentedOk - first go.
This is a pretty inscrutable issue though - so I reckon it's going to need a lot more work. I don't know how much explaining is necessary for most devs to grok this, but it's certainly been bending my brain! :)
Here goes....
Comment #7
xjmThanks @kattekrab! That's looking like a great start. Seeing it in context, I have another suggestion:
I'd change the "eg." to:
We probably want a comma before "see" here.
Comment #8
xjmOne more suggestion: Later in the docblock, it says:
However, nowhere does it clarify that "placeholders" mean the bits wrapped in curlies in the example above. So, maybe we should say something like:
I also don't understand this part of the documentation:
Are the square brackets intended to denote that the actual key is the entity type machine name, e.g.
{node}
,{comment}
, etc.? Let's get some help from someone who knows the API to clarify the text description.Oh, one more thing:
I think this probably needs to be :
'/entity/{entityType}/{id}'
Comment #9
jhodgdonPlease don't use "eg." in documentation. It is almost never used correctly (including here). To use it correctly, it:
- Has to be meaning "for example" not "that is" (people confuse it with i.e. all the time, another reason not to use it -- just use "for example")
- Should be punctuated "... blah blah blah; e.g., blah blah blah".
- Definitely should not be the beginning of a paragraph (maybe the patch cut out some lines that it shouldn't have?).
Also, lists in documentation should always be preceded by a : on the previous line, and if possible should use : to imply key:value relationships within list items. See
https://drupal.org/node/1354#lists
And at the end of doc blocks, it's best to use @see not @link. @link should only be used within a paragraph of text.
Comment #10
kattekrab CreditAttribution: kattekrab commentedThanks so much @xjm and @jhodgdon for your patch reviews!
@xjm
done!
done!
done!
Yeah, me neither! :/
good idea! Who would that be?
done!
@jhodgdon
fixed!
Also, lists in documentation should always be preceded by a : on the previous line, and if possible should use : to imply key:value relationships within list items. See
https://drupal.org/node/1354#lists
done. I think... :/
I had copied the style of as it appeared in the code - so now it doesn't match. Is that ok?
done!
=D \o/
Comment #11
kattekrab CreditAttribution: kattekrab commentedOh - and I guess this needs review again.
Comment #12
jhodgdonThis looks good to me. The only (minor) thing is the line wrapping. Docs are supposed to wrap as close to 80 characters as possible without going over, and some of the lines are a lot shorter than the others.
Hmmm... So the first full paragraph now says that the keys are "the type of URI", and then in the next paragraph, it talks about "link relationships". It's not necessarily obvious how those are related -- following the "don't make me think" philosophy, can we make this clearer?
Comment #13
kattekrab CreditAttribution: kattekrab commentedRE #12
Aaaah yes, line wrapping fixed.
I've taken out "The type of URI" and reworded that first par entirely by being much more descriptive.
I've explicitly made a connection between "link path pattern" & "entity link relationship" - although I'm not sure this is strictly true.
However in the front end interface - we have "Link Path" so I felt that adding "path" in to this doc block may aid understanding.
At least... that's what's going on in my head!
Anyway - here goes - take 3! :)
Comment #14
kattekrab CreditAttribution: kattekrab commentedJust for background info, in case someone else wants to sanity check all this, here's some nice references on URI patterns I found while trying to wrap my brain around this chunk of documentation.
in http://patterns.dataincubator.org/book/patterned-uris.html
and the W3 has some stuff too...
http://www.w3.org/2011/gld/wiki/223_Best_Practices_URI_Construction
Comment #15
jhodgdonThanks! I think there is still some room for improvement here.
I like the first line description, so let's keep that as it is.
Then the first full paragraph:
I couldn't really parse that sentence. Maybe it just needs to be something like this:
Path patterns are defined an an associative array where the key is the type of path, and the value is the path pattern. For example:
(the list)
Regarding the list, I think maybe {node} should be {id} in the 2nd and 3rd items, shouldn't it? The section below talking about what you can put in {} does not include {node}, and at least in 7.x the URLs have the node ID there?
Anyway, if you adopt that wording for the first paragraph, or something like it, maybe the 2nd paragraph could start with "The path types are more formally known as entity link relationships, and they should be given standard names from ..." (followed by the rest of the paragraph you have there) (I'd stay way from saying that they should be "defined using templates" from IANA because that might confuse the link paths with the link type names?).
Thoughts?
Comment #16
kattekrab CreditAttribution: kattekrab commentedThanks again for the great review.
Agree this is MUCH better.
"Path patterns are defined an an associative array where the key is the type of path, and the value is the path pattern. For example:
(the list)"
As for the list and whether it should be node or id? I genuinely have no idea.
And I also thought the link to the IANA and instruction to use those templates was the whole point of this docblock... so I dunno.
I think I really need to turn this over to someone who actually understands what it's about, because I'm really just guessing.
I'll make the above changes though - because they totally ad clarity - but the substantial stuff needs work by someone who groks it!
Comment #17
kattekrab CreditAttribution: kattekrab commentedComment #18
kattekrab CreditAttribution: kattekrab commentedOops - setting to needs review.
Comment #19
kattekrab CreditAttribution: kattekrab commentedoh - and un-assigning myself - so someone more qualified can bring it home!
Comment #20
jhodgdonMoving temporarily to Entity System component so the maintainers can review this patch for accuracy.
I think it reads well and is ready to commit, if it's accurate.
Comment #21
kattekrab CreditAttribution: kattekrab commented"someone in entity system" ?
Comment #22
Berdirformerly: "In the past; in earlier times". Shouldn't this be formally?
Looks ok, you want to ask @linclark and @Crell for an approval I think, not sure who else worked on it.
Comment #23
jhodgdonGood catch Berdir. I missed that one. :)
Entity system maintainers are listed as: fago, catch, Fando, and Berdir in MAINTAINERS.txt. Berdir says OK. That's probably good enough for us to at least commit it (it's just documentation and is definitely better than what it was before). So let's fix the formerly/formally thing and get this done, unless one of the other 3 objects to something.
Comment #24
linclark CreditAttribution: linclark commentedThe link relations in the IANA registry are a small subset of valid link relations. Any URI can be a valid link relation.
Do we intentionally scope it to IANA link relations?
Comment #25
jhodgdonI have no idea if we are trying to limit to IANA. I guess the previous documentation said "where possible", so we should probably put that back in: ... "and where possible, they should be given standard names..."
Thanks for reviewing linclark!
Comment #26
kattekrab CreditAttribution: kattekrab commentedThanks so much @berdir and @linclark for reviewing this!
Patch updated:
- formerly > formally
- inserted 'where possible'
Comment #27
jhodgdonI think this looks good, and the concerns have been addressed. Tentatively setting to RTBC, but let's leave it here for a bit (at least until it turns green and in case any of the other entity system maintainers want to chime in).
Comment #28
linclark CreditAttribution: linclark commentedUnfortunately, I'm fairly certain we do not intentionally limit this to IANA links, so the text is not quite there yet.
Comment #29
linclark CreditAttribution: linclark commentedI added a section on custom link relations and edited some of the text from the original comment.
Comment #31
linclark CreditAttribution: linclark commented#29: 2047283-29-entity-link-docs.patch queued for re-testing.
Comment #32
jhodgdonI am not sure I'm entirely happy with this change. You took out the part that said the path types were formally known as "entity link relationships" (which I see should really be called "relations", which is odd but apparently standard) -- isn't that still true?
Also... I went back to the original issue summary. We really need to edit that whole section of documentation, starting at the top of the "links" section, so that it all holds together. Sorry for missing that in my previous reviews. I think we need to make sure the whole section holds together.
Comment #33
linclark CreditAttribution: linclark commentedI think that we conflate two things when we say that the paths we define for our system to use internally are the same thing as a REST's APIs link relations. There is a correspondence between them, but they are not the same thing.
If we are going to say that these things are formally called something, it should not be "entity link relations". Link relations are between resources, which aren't necessarily coupled to entities.
Comment #34
jhodgdonOK... Good points.
Can we go back to the beginning of the links section and make sure it all hangs together then?
Comment #35
andypostNow we have related bug with comment module, that generates 'canonical' link without fragment #2010202-12: Deprecate comment_uri()
Does 'canonical' allows to have '#comment-123' fragment and should we add this one to 'links'?
Comment #36
linclark CreditAttribution: linclark commentedI'm suggesting that we simplify the way that this works in #2113345: Define a mechanism for custom link relationships. I would like to postpone this issue until there is a consensus in that one.
Comment #36.0
linclark CreditAttribution: linclark commentedUpdated issue summary.
Comment #37
Crell CreditAttribution: Crell commentedComment #38
Crell CreditAttribution: Crell at Palantir.net commentedBumping to 8.1 along with its dependency.
Comment #39
clemens.tolboom