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.

  1. 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.

  2. 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?)
  3. IANA should also have some @link action to the same reference the change notice uses.
  4. The current second paragraph needs a serious rewrite to make sense.
  5. 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

+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.

xjm’s picture

Issue tags: +Novice

#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.

jhodgdon’s picture

Um. 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?

xjm’s picture

I 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.

kattekrab’s picture

Assigned: Unassigned » kattekrab

Ok - I'm diving in here and will give this a go.

kattekrab’s picture

Status: Active » Needs review
FileSize
1.59 KB

Ok - 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....

xjm’s picture

Thanks @kattekrab! That's looking like a great start. Seeing it in context, I have another suggestion:

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -253,11 +253,17 @@ class EntityType extends Plugin {
+   * eg.
+   * - 'canonical' = '/entity/{entityType}'
+   * - 'edit-form' = '/node/{node}/edit'
+   * - 'version-history' = '/node/{node}/revisions'

I'd change the "eg." to:

The link patterns are defined in an associative array, keyed by the type of URI each represents. For example:

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -253,11 +253,17 @@ class EntityType extends Plugin {
+   * For a complete listing of IANA URI Template Relationships see

We probably want a comma before "see" here.

xjm’s picture

One more suggestion: Later in the docblock, it says:

...the following placeholders are supported:

However, nowhere does it clarify that "placeholders" mean the bits wrapped in curlies in the example above. So, maybe we should say something like:

Use placeholders wrapped in braces, e.g. {entityType}, to include a specific property of the entity in the template. By default, the following placeholders are supported:

I also don't understand this part of the documentation:

[entityType]: The entity type itself will also be a valid token for the ID of the entity. For instance, a placeholder of {node} used on the Node class would have the same value as {id}. This is generally preferred over "id" for better self-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:

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -253,11 +253,17 @@ class EntityType extends Plugin {
+   * - 'canonical' = '/entity/{entityType}'

I think this probably needs to be :
'/entity/{entityType}/{id}'

jhodgdon’s picture

Status: Needs review » Needs work

Please 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.

kattekrab’s picture

Thanks so much @xjm and @jhodgdon for your patch reviews!

@xjm

change the "eg." to:
The link patterns are defined in an associative array, keyed by the type of URI each represents. For example:

done!

We probably want a comma before "see"

done!

However, nowhere does it clarify that "placeholders" mean the bits wrapped in curlies in the example above. So, maybe we should say something like: Use placeholders wrapped in braces, e.g. {entityType}, to include a specific property of the entity in the template. By default, the following placeholders are supported:

done!

I also don't understand this part of the documentation:

[entityType]: The entity type itself will also be a valid token for the ID of the entity. For instance, a placeholder of {node} used on the Node class would have the same value as {id}. This is generally preferred over "id" for better self-documentation.

Yeah, me neither! :/

Let's get some help from someone who knows the API to clarify the text description.

good idea! Who would that be?

I think this probably needs to be :
'/entity/{entityType}/{id}'

done!

@jhodgdon

Please don't use "eg." in documentation.

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?

And at the end of doc blocks, it's best to use @see not @link.

done!

=D \o/

kattekrab’s picture

Status: Needs work » Needs review

Oh - and I guess this needs review again.

jhodgdon’s picture

Status: Needs review » Needs work

This 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?

kattekrab’s picture

Status: Needs work » Needs review
FileSize
2.71 KB

RE #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! :)

kattekrab’s picture

Just 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

How can we create more predictable, human-readable URIs?
Clean, clear URIs can be easier to remember and easier for developers to work with. This is especially true if the URIs can be algorithmically constructed or follow a common pattern. This allows URIs to be constructed or hacked in order to create new entry points into the dataset, e.g. determining the URI for a collection of similar resources based on knowledge of a single example URI.
...
There are clear benefits from having human-readable, hackable URIs. This solution achieves that by ensuring the same naming scheme that applies to the underlying data also applies to the URIs. This provides a clear relation between the URI and the type of thing that it describes.

and the W3 has some stuff too...
http://www.w3.org/2011/gld/wiki/223_Best_Practices_URI_Construction

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! 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:

+   * Path patterns are defined in an associative array showing the type of link
+   * keyed by the relevant form of the entity such as the main path or
+   * 'canonical' form, the edit form, or where to find revisions. For example:
+   * - canonical: /entity/{entityType}/{id}
+   * - edit-form: /node/{node}/edit
+   * - version-history: /node/{node}/revisions

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?

kattekrab’s picture

Thanks 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!

kattekrab’s picture

kattekrab’s picture

Status: Needs work » Needs review

Oops - setting to needs review.

kattekrab’s picture

Assigned: kattekrab » Unassigned

oh - and un-assigning myself - so someone more qualified can bring it home!

jhodgdon’s picture

Component: documentation » entity system

Moving 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.

kattekrab’s picture

"someone in entity system" ?

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
@@ -253,11 +253,19 @@ class EntityType extends Plugin {
+   * The path types are more formerly known as entity link relationships, and

formerly: "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.

jhodgdon’s picture

Status: Needs review » Needs work

Good 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.

linclark’s picture

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -253,11 +253,19 @@ class EntityType extends Plugin {
+   * The path types are more formerly known as entity link relationships, and
+   * they should be given standard names from the Internet Assigned Numbers
+   * Authority (IANA). A complete listing of IANA URI Template Relationships is
+   * available online here:

The 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?

jhodgdon’s picture

I 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!

kattekrab’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
2.63 KB

Thanks so much @berdir and @linclark for reviewing this!

Patch updated:
- formerly > formally
- inserted 'where possible'

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I 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).

linclark’s picture

Status: Reviewed & tested by the community » Needs work

Unfortunately, I'm fairly certain we do not intentionally limit this to IANA links, so the text is not quite there yet.

linclark’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
2.93 KB

I added a section on custom link relations and edited some of the text from the original comment.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, 2047283-29-entity-link-docs.patch, failed testing.

linclark’s picture

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

#29: 2047283-29-entity-link-docs.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

I 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.

linclark’s picture

I 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?

I 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.

jhodgdon’s picture

OK... Good points.

Can we go back to the beginning of the links section and make sure it all hangs together then?

andypost’s picture

Now 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'?

linclark’s picture

Status: Needs work » Postponed

I'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.

linclark’s picture

Issue summary: View changes

Updated issue summary.

Crell’s picture

Crell’s picture

Version: 8.0.x-dev » 8.1.x-dev

Bumping to 8.1 along with its dependency.

clemens.tolboom’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.