Updated: Comment #N

Problem/Motivation

In #1970360: Entities should define URI templates and standard links, we decided to use IANA link relations as the keys for the entity links. I believe we want to revisit this decision for the following reasons.

  1. It requires anyone who's adding an entity link to learn about link relations. This is one of the more complex, less understood parts of REST, and even most of the core devs right now aren't familiar with the related standards.
  2. It couples the entity system and the REST API. Other parts of the system will be looking in the annotation for specific keys. This means that you can't change the link relation that it maps to later on if something more appropriate is standardized.
  3. It requires developers to create new link relations even for links which aren't suitable for exposure in a REST API.

Proposed resolution

After some discussion in the REST team, we came to the following proposed resolution:

1) Create a new plugin type, "Link Relationship". It will use YAML discovery (since there's no relevant code variation, just metadata variation). The YAML file would look something along these lines:

links:
  canonical:
      relationship: canonical
      see also: http://www.iana.org/assignments/link-relations/link-relations.xhtml
  create-form:
    relationship: http://drupal.org/rel/create-form
    description: A form where a resource of this type can be created
    see also: http://whatever.com
  foobar:
    relationship: http://bob.com/rel/foobar
    description: 
  panelizer-form:
    relationship: http://drupal.org/rel/panelizer-form
    description: Configure the panelization of this resource

Each key is a nominally arbitrary string that defines the link relationship. The relationship key defines the actual link relationship itself, which could be a URI or a standardized relationship name, such as canonical, edit-form, etc. (This is good REST-citizen behavior.) Description, See also, Name, and other possible metadata keys we can figure out later in this issue.

2) Core ships with definitions for all IANA-defined link relationships: http://www.iana.org/assignments/link-relations/link-relations.xhtml and possibly *some* microformat-defined relationships if they make sense in context: http://microformats.org/wiki/existing-rel-values

3) Core also ships with a small set of additional defined relationships for things like create-form that are not defined by a standards group but we know we'll need.

4) Contrib is free to provide additional definitions for other relationships, such as the panelizer-form example above.

5) The above defines the existance/definition of a link. To actually declare a link relevant for a given entity type, entities do... exactly what they do now. The annotation does not change, and remains:

@link = {
  "canonical" = "/node/{node}"
}

However, rather than the key being an IANA-defined link relation it is... the machine name of a plugin that defines a link relation, which in the degenerate case just so happens to be the same as an IANA-defined link relation. It's totally kosher to define new ones, and the keys do not have to match up with anything other than good DX.

The rest of how we leverage links remains the same. The advantage is that it divorces "I want to associate a link to an entity type" from "I need to know WTF a link relationship is". Instead, "I need to know WTF a link relationship is" gets coupled to "I want to define a new link relationship concept", which is where it belongs. If you're just using existing link relationships, all you need to know is a list of machine names and descriptions in a YAML file. The only required link is canonical, which is already true.

6) Because they're plugins, we can use derivatives or plugin-info-hook-bonus-mode or whatever to define relations for all entity reference fields, so that what we're currently doing with entity reference fields is auto-defined/documented.

7) Side benefits:
A) We now have a way to define new link relations, which we hadn't figured out before.
B) We now have the data in place to provide automated link relation documentation on the site, which we don't have at present. (Actually doing so is a non-critical follow-up.)
C) It makes it easier for us to do automated stuff with link relations, either in REST/Serializer modules or elsewhere, because we have a known definition list as well as usage list.

Remaining tasks

Implement the above. (Contact Crell or LinClark if you want to help but need it broken down further.)

Also for a follow-up, we should develop a way to let modules add links to an entity on a per-entity basis, which would be useful for, say, book module to leverage "up", "next", and "prev" for navigation in both REST and as an HTML page in a clean fashion. That's for another issue, however.

User interface changes

None.

API changes

* Fix #2150747: Switch back to URI templates in entity annotations first. For the vast majority of use cases and users, there is no other API change here, only additions.
* Creating a new plugin type.

#1970360: Entities should define URI templates and standard links
#2047283: Improve documentation for EntityType::$links
#2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items
#2084463: Convert contextual links to a plugin system similar to local tasks/actions

CommentFileSizeAuthor
#126 interdiff.txt3.36 KBeffulgentsia
#126 2113345-126.patch39.07 KBeffulgentsia
#122 2113345-122.patch37.5 KBWim Leers
#121 2113345-120.patch37.55 KBWim Leers
#121 interdiff.txt20.01 KBWim Leers
#120 2113345-119.patch35.23 KBWim Leers
#120 interdiff.txt1.57 KBWim Leers
#118 2113345-118.patch34.21 KBWim Leers
#118 interdiff.txt3.6 KBWim Leers
#117 interdiff.txt2.46 KBdawehner
#117 2113345-117.patch32.23 KBdawehner
#108 interdiff.txt2.43 KBdawehner
#108 2113345-108.patch32.29 KBdawehner
#107 2113345-107.patch31.69 KBtedbow
#107 interdiff-98-107.txt4.75 KBtedbow
#104 2113345-104.patch26.95 KBtedbow
#98 interdiff-2113345-89-98.txt8.25 KBtedbow
#98 2113345-98.patch30.69 KBtedbow
#89 interdiff-2113345-80-89.txt490 bytestedbow
#89 2113345-89.patch30.84 KBtedbow
#80 2113345-80.patch32.28 KBkristiaanvandeneynde
#80 interdiff-80-77.txt23.52 KBkristiaanvandeneynde
#77 2113345-77.patch31.46 KBWim Leers
#77 interdiff.txt3.79 KBWim Leers
#76 interdiff-2113345-69-76.txt2.66 KBtedbow
#76 2113345-76.patch31.13 KBtedbow
#69 interdiff-2113345-67-69.txt4.03 KBtedbow
#69 2113345-69.patch31.37 KBtedbow
#67 interdiff-2113345-64-67.txt4.51 KBtedbow
#67 2113345-67.patch31.38 KBtedbow
#64 interdiff-2113345-58-64.txt4.15 KBtedbow
#64 2113345-64.patch31.45 KBtedbow
#58 interdiff-2113345-57-58.txt3.09 KBtedbow
#58 2113345-58.patch30.66 KBtedbow
#57 interdiff.txt2.79 KBdawehner
#57 2113345-57.patch30.26 KBdawehner
#55 interdiff.txt5.35 KBdawehner
#55 2113345-55.patch29.9 KBdawehner
#44 interdiff.txt11.38 KBdawehner
#44 2113345-43.patch29.17 KBdawehner
#41 interdiff.txt3.83 KBdawehner
#41 2113345-41.patch29.04 KBdawehner
#39 convert.php_.txt619 bytesdawehner
#38 interdiff.txt33.19 KBdawehner
#38 2113345-38.patch27.63 KBdawehner
#36 interdiff.txt2.23 KBdawehner
#36 2113345-36.patch26.07 KBdawehner
#33 interdiff.txt3.52 KBdawehner
#33 2113345-33.patch23.59 KBdawehner
#26 interdiff.txt2.99 KBdawehner
#26 2113345-25.patch23.21 KBdawehner
#24 2113345-24.patch20.6 KBdawehner
#24 convert.php_.txt598 bytesdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Also we have serious long-lasting WTF with comment URIs and another major issue #2113323: Rename Comment::permalink() to not be ambiguous with ::uri()
Comments as entities should have URI to be accessed via REST comment/123 but UX needs ability to allow users to easily copy/paste/share links to exact comment in thread by providing #comment-123 fragment

linclark’s picture

That issue would remain the same even if we make this change.

linclark’s picture

Added third problem:

It requires developers to create new link relations even for links which aren't suitable for exposure in a REST API.

Crell’s picture

We discussed this issue at length on the REST team call today. Here's the solution we came up with:

1) Create a new plugin type, "Link Relationship". It will use YAML discovery (since there's no relevant code variation, just metadata variation). The YAML file would look something along these lines:

links:
  canonical:
      relationship: canonical
      see also: http://www.iana.org/assignments/link-relations/link-relations.xhtml
  create-form:
    relationship: http://drupal.org/rel/create-form
    description: A form where a resource of this type can be created
    see also: http://whatever.com
  foobar:
    relationship: http://bob.com/rel/foobar
    description: 
  panelizer-form:
    relationship: http://drupal.org/rel/panelizer-form
    description: Configure the panelization of this resource

Each key is a nominally arbitrary string that defines the link relationship. The relationship key defines the actual link relationship itself, which could be a URI or a standardized relationship name, such as canonical, edit-form, etc. (This is good REST-citizen behavior.) Description, See also, Name, and other possible metadata keys we can figure out later in this issue.

2) Core ships with definitions for all IANA-defined link relationships: http://www.iana.org/assignments/link-relations/link-relations.xhtml and possibly *some* microformat-defined relationships if they make sense in context: http://microformats.org/wiki/existing-rel-values

3) Core also ships with a small set of additional defined relationships for things like create-form that are not defined by a standards group but we know we'll need.

4) Contrib is free to provide additional definitions for other relationships, such as the panelizer-form example above.

5) The above defines the existance/definition of a link. To actually declare a link relevant for a given entity type, entities do... exactly what they do now. The annotation does not change, and remains:

@link = {
  "canonical" = "/node/{node}"
}

However, rather than the key being an IANA-defined link relation it is... the machine name of a plugin that defines a link relation, which in the degenerate case just so happens to be the same as an IANA-defined link relation. It's totally kosher to define new ones, and the keys do not have to match up with anything other than good DX.

The rest of how we leverage links remains the same. The advantage is that it divorces "I want to associate a link to an entity type" from "I need to know WTF a link relationship is". Instead, "I need to know WTF a link relationship is" gets coupled to "I want to define a new link relationship concept", which is where it belongs. If you're just using existing link relationships, all you need to know is a list of machine names and descriptions in a YAML file. The only required link is canonical, which is already true.

6) Because they're plugins, we can use derivatives or plugin-info-hook-bonus-mode or whatever to define relations for all entity reference fields, so that what we're currently doing with entity reference fields is auto-defined/documented.

7) Side benefits:
A) We now have a way to define new link relations, which we hadn't figured out before.
B) We now have the data in place to provide automated link relation documentation on the site, which we don't have at present. (Actually doing so is a non-critical follow-up.)
C) It makes it easier for us to do automated stuff with link relations, either in REST/Serializer modules or elsewhere, because we have a known definition list as well as usage list.

8) Not addressed: We chewed through how to dovetail this with entity operations, as discussed previously in #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items. We eventually decided to punt on that question until we get input from Peter and Wim. It looks like contextual links may have wandered off to their own plugins anyway (#2084463: Convert contextual links to a plugin system similar to local tasks/actions), which would solve that question.

Also for a follow-up, we should develop a way to let modules add links to an entity on a per-entity basis, which would be useful for, say, book module to leverage "up", "next", and "prev" for navigation in both REST and as an HTML page in a clean fashion. That's for another issue, however.

Crell’s picture

Issue summary: View changes

Added new problem.

Crell’s picture

Issue summary: View changes

Add more related links

Crell’s picture

Issue summary: View changes
Issue tags: +beta blocker

I am going to go ahead and bump this to beta blocker. It cleans up our link handling considerably, and sets a stage for later expansion. Lin, do you have any bandwidth at all for this?

yukare’s picture

I am working on a contrib module(customfilter), and from DX point of view, this will be a big win, there are many link relations that are not possible now. Contrib must be able to add new links when necessary.

pwolanin’s picture

It would be helpful to update/clarify the issue summary since the annotations now refer to routes instead of paths.

Crell’s picture

pwolanin: The issue you mention needs to be rolled back precisely because it breaks this and other patches.

linclark’s picture

I do not have the bandwidth to take this on. @yukare, since you have a use case in mind, could you take this on?

juampynr’s picture

I could help with this too, but I need further guidance.

Crell’s picture

Title: Do not use link relations as keys for entity links annotation » Define a mechanism for custom link relationships
Issue summary: View changes
Status: Active » Postponed
Issue tags: -Needs issue summary update
Related issues: +#2051591: Define a mechanism for custom link relationships, +#2150747: Switch back to URI templates in entity annotations, +#1970360: Entities should define URI templates and standard links, +#2084463: Convert contextual links to a plugin system similar to local tasks/actions

Retitling and merging with the prior issue: #2051591: Define a mechanism for custom link relationships. Also, it's blocked on #2150747: Switch back to URI templates in entity annotations. Sad panda.

xjm’s picture

Is this issue critical, as in, we would not release Drupal 8.0 without it? It sounds like maybe it should be based on:

I am working on a contrib module(customfilter), and from DX point of view, this will be a big win, there are many link relations that are not possible now. Contrib must be able to add new links when necessary.

If so let's bump the priority. On the other hand, if this is something we could add in 8.1, let's switch to the "beta target" tag and keep it at major. Thanks everyone!

Crell’s picture

Priority: Major » Critical

Although it would be very small, there would, technically, be an API change from this issue. (The value you put in the annotation would change from "IANA defined relation" to "Drupal machine name", which we just so happen to plan to make the same string for the currently in-use relations".) It also provides us a sane way to create link relations from an ER field, which right now I don't think we have. I'm therefore going to be aggressive and bump this to critical.

If Dries feels the above would be an "allowed in 8.x" API change, then I am OK with downgrading to Major/beta-target. But it's definitely something we must do within the early 8.x cycle for our REST support to be fully baked.

catch’s picture

Issue tags: -beta blocker

Let's not block beta on it then, sounds like the API change is mostly theoretical.

Crell’s picture

Status: Postponed » Active
andypost’s picture

Another problem here is "bundlability" that breaks "create-form" because it needs mostly always second argument (bundle)

Crell’s picture

That shouldn't be an issue. The link template can include a bundle name. (I had it documented in the Annotation back when the links array in the annotation used URI templates, but apparently that got removed when it was converted to routes.)

mgifford’s picture

catch’s picture

Priority: Critical » Major

(The value you put in the annotation would change from "IANA defined relation" to "Drupal machine name", which we just so happen to plan to make the same string for the currently in-use relations".)

Sounds like if anything, we could add a backwards compatibility layer for this, so I'm moving this back to major - it's a pure API/feature addition otherwise so seems fine to add in 8.1.x.

If a contrib module is actually blocked from an 8.x port due to this, then we could revisit the status again.

Crell’s picture

I am 93.2% sure this could be done as strictly API additions for PHP code, so I agree with downgrading. It is likely that the Drupal-specific link relationship names would change, though (they'd start using namespaced curies), so that would impact any REST client that is using them. That is likely to be a much smaller number of people, though.

Crell’s picture

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

I consider this 8.1-safe, so refiling.

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.

Wim Leers’s picture

Issue tags: +REST
dawehner’s picture

Status: Active » Needs review
FileSize
598 bytes
20.6 KB

In general I think the issue summary overestimates the amount of thoughts people put into the link annotation on entities. I think they rather just think: links are the way to generate paths for entities, whatever I'll just copy and adapt them.

Status: Needs review » Needs work

The last submitted patch, 24: 2113345-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
23.21 KB
2.99 KB

Now we could for example expose them via HTTP, not sure though whether that implementation is even remotely right.

Status: Needs review » Needs work

The last submitted patch, 26: 2113345-25.patch, failed testing.

kristiaanvandeneynde’s picture

The idea behind the interdiff seems about right; i.e.: the relations showing up as <link> elements in the <head> part.

Don't know if the code is correct by simply looking at it, would have to try it out.

Edit: Perhaps we'd want to throw exceptions in Entity::toUrl() and the like when a link template is requested that doesn't have a definition?

dawehner’s picture

t; i.e.: the relations showing up as
elements in the part.

Well, this code is about HTTP headers actually, not about HTML.

Edit: Perhaps we'd want to throw exceptions in Entity::toUrl() and the like when a link template is requested that doesn't have a definition?

I thought we already do.

kristiaanvandeneynde’s picture

Well, this code is about HTTP headers actually, not about HTML.

Hah, never mind me! In that case the code looks about right.

I thought we already do.

You're talking about throw new UndefinedLinkTemplateException("No link template '$rel' found for the '{$this->getEntityTypeId()}' entity type");, I'm talking about throwing an exception if the relation is specified on the entity annotation, but does not have a definition. As in: the plugin this issue introduces.

Edit: It would ensure that all link relations have a valid definition somewhere. Although there may be better places to throw that exception, perhaps.

dawehner’s picture

You're talking about throw new UndefinedLinkTemplateException("No link template '$rel' found for the '{$this->getEntityTypeId()}' entity type");, I'm talking about throwing an exception if the relation is specified on the entity annotation, but does not have a definition. As in: the plugin this issue introduces.

Oh yeah, that is a good idea indeed!

Crell’s picture

This is a great start, dawehner, thank you!!! I especially love that you've included, it looks like, the entire IANA registry by default. :-)

  1. +++ b/core/lib/Drupal/Core/Http/LinkRelationship.php
    @@ -0,0 +1,44 @@
    +  /**
    +   * @return string|null
    +   */
    +  public function getDescription() {
    +    return isset($this->pluginDefinition['description']) ? $this->pluginDefinition['description'] : NULL;
    +  }
    

    Shouldn't these return empty string, not null? That's more type safe, but results in essentially the same result and is still a falsy value.

  2. +++ b/core/lib/Drupal/Core/Http/LinkRelationshipManager.php
    @@ -0,0 +1,50 @@
    +    $plugin_interface = LinkRelationship::class;
    +    $this->root = $root;
    +    $this->pluginInterface = $plugin_interface;
    

    Nit: Why is the temporary $plugin_interface variable needed?

  3. +++ b/core/lib/Drupal/Core/Http/LinkRelationshipManager.php
    @@ -0,0 +1,50 @@
    +    }
    +    return $this->discovery;
    +  } ¶
    

    Dreditor says whitespace.

  4. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -59,6 +90,15 @@ public function get(EntityInterface $entity) {
    +    // Add link relationship headers.
    +    foreach ($this->linkRelationshipManager->getDefinitions() as $name => $definition) {
    

    Here's where this gets interesting. HAL (and I presume JsonAPI?) has its own mechanism for links. There's also, in parallel, the HTTP header spec for links. Which should we be using? Either or even both are legal.

    HTTP (as done here) is certainly easier to implement. However, is that useful for the actual payloads? Does it help when we want to also expose these in HTML, or Atom, which have a formal expectation of link elements (and in some cases they are required)?

    Doing both is probably unnecessary overkill (and bandwidth eating), but we should think through how we want to handle this.

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -59,6 +90,15 @@ public function get(EntityInterface $entity) {
    +        $uri = $entity->toUrl($name)->setAbsolute(TRUE)->toString(TRUE)->getGeneratedUrl();
    

    Side note: I don't know if I love or hate this line...

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -59,6 +90,15 @@ public function get(EntityInterface $entity) {
    +        $link_header = '<' . $uri . '>; rel="' . $name . '"'; ¶
    

    Trailing whitespace, says Dr. Editor.

dawehner’s picture

Status: Needs work » Needs review
FileSize
23.59 KB
3.52 KB

Thank you for your review crell!

Nit: Why is the temporary $plugin_interface variable needed?

Good catch, yeah there is no reason for it.

HTTP (as done here) is certainly easier to implement. However, is that useful for the actual payloads? Does it help when we want to also expose these in HTML, or Atom, which have a formal expectation of link elements (and in some cases they are required)?

Yeah for now I would argue we should focus on HTTP links, they are useful for everyone and doesn't really require any additional bikesheding.

Fixed the other nits and added some documentation.

Status: Needs review » Needs work

The last submitted patch, 33: 2113345-33.patch, failed testing.

The last submitted patch, 33: 2113345-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.07 KB
2.23 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 36: 2113345-36.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
27.63 KB
33.19 KB

Worked a bit on it.

dawehner’s picture

FileSize
619 bytes

Oh yeah a new script

Status: Needs review » Needs work

The last submitted patch, 38: 2113345-38.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.04 KB
3.83 KB

This time even with a working patch.

Wim Leers’s picture

Component: entity system » rest.module

Moving to REST module, because this does not affect the entity system anywhere; this only affects core itself + rest.module.

  1. +++ b/core/core.link_relationship.yml
    @@ -0,0 +1,251 @@
    +  description: "Refers to a collection of records, documents, or other\n      materials of historical interest."
    ...
    +  description: "Identifies the entity that blocks access to a resource\n      following receipt of a legal demand."
    ...
    +  description: "Refers to a copyright statement that applies to the\n    link's context."
    

    These newlines look bizarre… I think they're just there because of copy/pasting? Let's remove the newlines?

  2. +++ b/core/lib/Drupal/Core/Http/LinkRelationshipInterface.php
    @@ -0,0 +1,38 @@
    +   * Returns the name/identifier of the link relationship.
    

    IANA name?

  3. +++ b/core/lib/Drupal/Core/Http/LinkRelationshipInterface.php
    @@ -0,0 +1,38 @@
    +   * Returns the description of the link relationship.
    

    IANA name description?

  4. +++ b/core/lib/Drupal/Core/Http/LinkRelationshipInterface.php
    @@ -0,0 +1,38 @@
    +  /**
    +   * Returns some extra notes about this link relationship.
    +   *
    +   * @return string
    +   */
    +  public function getNotes();
    

    Upon first reading this, I have absolutely no idea what this means.

  5. +++ b/core/lib/Drupal/Core/Http/LinkRelationshipManager.php
    @@ -0,0 +1,52 @@
    +      $this->discovery = new YamlDiscovery('link_relationship', [
    +          'core' => $this->root . '/core',
    +        ] + array_map(function (Extension $extension) {
    +          return $this->root . '/' . $extension->getPath();
    +        }, $this->moduleHandler->getModuleList()));
    +    }
    

    Fancy! :)

    I'd personally put the first array on a single line, that'd greatly improve legibility.

  6. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -344,4 +360,27 @@ public function calculateDependencies() {
    +    foreach ($this->linkRelationshipManager->getDefinitions() as $name => $definition) {
    +      if ($entity->hasLinkTemplate($name)) {
    

    By starting with the link relationship manager, you effectively blacklist any custom link relations that an entity might have.

    Why not flip it around? foreach entity links and then just add an assert('$this->linkRelationshipManager->hasDefinition($name)', 'MESSAGE');, with the message warning you to register this custom relationship name.

    Or… why even have this link relationship manager? We don't use the definition anywhere in here.

  7. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -528,4 +528,32 @@ protected function configEntityValues($entity_type_id) {
    +  protected function assertLinkHeader($entity, array $link_relationships = [
    +    'canonical',
    +    'edit-form'
    +  ]) {
    

    This formatting looks bizarre.

Grayside’s picture

Maybe I'm missing something that should be obvious by context, but how do you use the relation types to create a relation with a URL?

dawehner’s picture

These newlines look bizarre… I think they're just there because of copy/pasting? Let's remove the newlines?

Any idea how I can adapt the script to ensure this happens?

IANA name?

IANA is just one of the instances which define the possible link relations. The standard itself is part of IETF

Upon first reading this, I have absolutely no idea what this means.

Well, I linked to the reference. There is not much to explain there anyway. Its some metadescription one doesn't need on actual runtime, but just on documentation time.

I'd personally put the first array on a single line, that'd greatly improve legibility.

I hope this change improves the readability.

By starting with the link relationship manager, you effectively blacklist any custom link relations that an entity might have.

Why not flip it around? foreach entity links and then just add an assert('$this->linkRelationshipManager->hasDefinition($name)', 'MESSAGE');, with the message warning you to register this custom relationship name.

Well, it would be an API break if we require custom link relations definitions here. I doubt we can/should do it here.

Or… why even have this link relationship manager? We don't use the definition anywhere in here.

Well, on top of this issue we can document available link relations provided by Drupal. This is though something that is out of scope of this issue.

Note: I renamed link relationship to link relation, as this is what the standard talks about.

Maybe I'm missing something that should be obvious by context, but how do you use the relation types to create a relation with a URL?

Well, entity types define link relations using link templates. Other systems can do whatever they want. This issue is about having a formal index of available ones.

tedbow’s picture

+++ b/core/lib/Drupal/Core/Http/LinkRelationManager.php
@@ -0,0 +1,52 @@
+      $directories = ['core' => $this->root . '/core'];
+      $directories += array_map(function (Extension $extension) {
+        return $this->root . '/' . $extension->getPath();
+      }, $this->moduleHandler->getModuleList());
+      $this->discovery = new YamlDiscovery('link_relation', $directories);

Much more readable to me!

Well, it would be an API break if we require custom link relations definitions here. I doubt we can/should do it here.

I agree that we shouldn't shouldn't support custom link relations that aren't discovered by the manager.

But as a developer who wasn't familiar with this issue but just looking at core as an example of how to do things I could see how this could be confusing.

Looking at annotations in something like the Node entity I feel like it would not be obvious that I could not just add link relations in my own entity and choose whatever keys I wanted. Or use a hook to add custom relations to an existing entity like node without registering them somewhere.

I think this could be made a little easier by:

  1. Put a comment in \Drupal\rest\Plugin\rest\resource\EntityResource::addLinkHeaders to specify that link must be registered and how
  2. And/or after looping through $this->linkRelationManager->getDefinitions() check to see there are any relations type on the entity type that aren't supported and put a warning in the log.

Note: I renamed link relationship to link relation, as this is what the standard talks about.

This seems more accurate.

Wim Leers’s picture

Well, it would be an API break if we require custom link relations definitions here. I doubt we can/should do it here.

But that's exactly what I said is the problem in the current patch: EntityResource does not iterate over all of the entity type's link templates: it only iterates over all relations in the relation manager, and then checks if the current entity type has a template for that. So, the current patch is the one that requires custom link relations to be defined in the relation manager.

So I'm precisely asking to keep BC, i.e. not requiring an additional thing (i.e. metadata for the relation manager) for an entity type's links to be exposed.

Or… why even have this link relationship manager? We don't use the definition anywhere in here.

Well, on top of this issue we can document available link relations provided by Drupal. This is though something that is out of scope of this issue.

I agree, that's kind of out of scope. All the more reason to not let EntityResource not require a relation to be defined in the relation manager before EntityResource can send such link headers?

dawehner’s picture

But that's exactly what I said is the problem in the current patch: EntityResource does not iterate over all of the entity type's link templates: it only iterates over all relations in the relation manager, and then checks if the current entity type has a template for that. So, the current patch is the one that requires custom link relations to be defined in the relation manager.

Well, its using that for a new feature, which my definition cannot have a BC break :)
I think you talked about checking for this link relationship when we create the URL, like inside Entity itself.

+    foreach ($this->linkRelationManager->getDefinitions() as $relation_name => $definition) {
+      if ($entity->hasLinkTemplate($relation_name)) {

So if the link template isn't there, nothing happens, which is fair, because the entity doesn't support that link relation, so we also don't want to have it exposed.

e. All the more reason to not let EntityResource not require a relation to be defined in the relation manager before EntityResource can send such link headers?
Right. I think we could support two types of documentation:

  • A list of any in theory possible supported link relations together with its description
  • A list of link relations supported by a specific entity type
Wim Leers’s picture

I see! That's fair. But I'd argue that people would expect all of their link relationships to be exposed. Perhaps that's a wrong expectation of mine though.

dawehner’s picture

I see! That's fair. But I'd argue that people would expect all of their link relationships to be exposed. Perhaps that's a wrong expectation of mine though.

Well, then this is some documentation thing. We already document every IANA link relation, of which not all could even make sense for all kind of entities.

Wim Leers’s picture

Status: Needs review » Needs work

Ok.

So, then is there anything else that needs further scrutiny? AFAICT there isn't, this just needs final polish, then it's ready. Would be great to also have +1s from klausi & Crell.

  1. +++ b/core/lib/Drupal/Core/Http/LinkRelation.php
    @@ -0,0 +1,44 @@
    + * @todo pointing to actual documentation.
    

    Let's do this.

  2. +++ b/core/lib/Drupal/Core/Http/LinkRelationInterface.php
    @@ -0,0 +1,46 @@
    + * Defines a single link relationship.
    
    +++ b/core/lib/Drupal/Core/Http/LinkRelationManager.php
    @@ -0,0 +1,52 @@
    + * Provides a plugin manager for link relationships.
    

    s/relationships/relations/

  3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -528,4 +528,32 @@ protected function configEntityValues($entity_type_id) {
    +   *   The entity
    

    Nit: missing trailing period.

  4. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -528,4 +528,32 @@ protected function configEntityValues($entity_type_id) {
    +  protected function assertLinkHeader($entity, array $link_relationships = [
    +    'canonical',
    +    'edit-form'
    +  ]) {
    

    Nit: strange formatting.

Crell’s picture

+++ b/core/lib/Drupal/Core/Http/LinkRelation.php
@@ -0,0 +1,44 @@
+class LinkRelation extends PluginBase implements LinkRelationInterface {

Thank you Dreditor for dying and taking my comment with you...

There's a missing layer of abstraction here. Currently, it appears the plugin always uses the link relationship as the machine name, unconditionally. That's no good for custom relationships. Any non-IANA relationships must be a URI, but we don't want to force people to type a full URI in an entity annotation. We need to specify the actual relationship name as a separate key/property for the relationship plugin.

See the "panelizer-form" example in the issue summary. The plugin name is panelizer-form, and that's what you'd use in the annotation. But the actual relationship as it would appear in the link header in HTTP (or similar) is a URI that points to Drupal.org.

dawehner’s picture

Thank you @crell and @Wim Leers!

There's a missing layer of abstraction here. Currently, it appears the plugin always uses the link relationship as the machine name, unconditionally. That's no good for custom relationships. Any non-IANA relationships must be a URI, but we don't want to force people to type a full URI in an entity annotation. We need to specify the actual relationship name as a separate key/property for the relationship plugin.

Gotcha, is there any place in the RFC, or so, we can point people to for documentation purposes?
What about turning things around and allowing to add the non-IANA relationship URL as additional key onto the relationship definition? I thinking this would be more logic for people, when they want to define custom ones.

dawehner’s picture

@Crell
IN these cases, checkout localStorage in your browser. There is a high chance it contains information you want.

Crell’s picture

Er, that's what I said we should do. :-) See the examples in the issue summary. Since we're already pre-defining basically all of the standard link relationships, I expect virtually all module-provided relationships to be custom and therefore needing a URI relationship definition.

I'm not sure what RFC reference you're looking for. The Link RFC we already reference talks about non-standard relationships, doesn't it?

dawehner’s picture

Status: Needs work » Needs review
FileSize
29.9 KB
5.35 KB

This addresses the feedback from crell

Status: Needs review » Needs work

The last submitted patch, 55: 2113345-55.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.26 KB
2.79 KB

Let's fix the test failure. Still trying to come up with proper documentation.

@crell
it would be great if you could maybe help out a bit here :)

tedbow’s picture

Re-rolled #57.

Also address nits in #50

Also

+++ b/core/lib/Drupal/Core/Http/LinkRelationManager.php
@@ -0,0 +1,52 @@
+  public function __construct($root, ModuleHandlerInterface $module_handler) {
+    $this->root = $root;

Is not calling the parent constructor intentional here? I didn't fix(if needed) in patch

Wim Leers’s picture

Status: Needs review » Needs work

Is not calling the parent constructor intentional here? I didn't fix(if needed) in patch

That looks like an important oversight indeed.

tedbow’s picture

Status: Needs work » Needs review

Re:#59

That looks like an important oversight indeed

Looking at it closer I don't think so.

The parent constructor \Drupal\Core\Plugin\DefaultPluginManager::__construct is actually used to get arguments for class based plugin discovery which we aren't doing here(using *.yml files). We overriding getDiscovery where these are used.

dawehner’s picture

The parent constructor \Drupal\Core\Plugin\DefaultPluginManager::__construct is actually used to get arguments for class based plugin discovery which we aren't doing here(using *.yml files). We overriding getDiscovery where these are used.

Yeah, there are many examples of plugin managers which don't do it. For example \Drupal\Core\Config\TypedConfigManager::__construct, well basically which doesn't use annotations basically.

Thank you @tedbow for #58, it gets the ball rolling again, hopefully.

We still somehow should try to document the big pictures of link templates vs. relations somehow. I am not even sure where and how, to be honest.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Http/LinkRelationManager.php
    @@ -0,0 +1,52 @@
    +class LinkRelationManager extends DefaultPluginManager {
    
    +++ b/core/tests/Drupal/KernelTests/Core/Http/LinkRelationsTest.php
    @@ -0,0 +1,30 @@
    +    /** @var \Drupal\Core\Http\LinkRelationManager $link_relation_manager */
    

    As a service, this should have an interface, even if it just extends another one.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Http/LinkRelationsTest.php
    @@ -0,0 +1,30 @@
    +    /** @var  $canonical */
    

    Extra line

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Http/LinkRelation.php
    @@ -0,0 +1,48 @@
    +  public function getRelationshipUrl() {
    +    return isset($this->pluginDefinition['relationship']) ? $this->pluginDefinition['relationship'] : '';
    +  }
    

    If there isn't one specified, should it default to the plugin name? Since that's what the IANA case is?

  2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -371,4 +386,33 @@ public function calculateDependencies() {
    +  protected function addLinkHeaders(EntityInterface $entity, CacheableResponseInterface $response) {
    +    foreach ($this->linkRelationManager->getDefinitions() as $relation_name => $definition) {
    +      if ($entity->hasLinkTemplate($relation_name)) {
    

    I'm concerned about the cardinality here. We're iterating over all relationship plugins, every time we render an entity to a response object. That's already several dozen plugin instances to create and iterate across. Then contrib will likely add more; perhaps not hundreds more, but it's an unbounded list.

    I am concerned about the performance impact of this approach. Wouldn't it be more efficient to iterate the link templates the entity actually has, then pull just those plugins out of the relation manager?

  3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -540,4 +541,29 @@ protected function configEntityValues($entity_type_id) {
    +  protected function assertLinkHeader(EntityInterface $entity, array $link_relationships = ['canonical', 'edit-form']) {
    

    Shouldn't this be linkHeaders()? Plural? Since it's checking an array.

I can't quite tell. Is the resulting output from this patch the following header:

Link: ; rel="edit-form";

or:

Link: ; rel="https://drupal.org/link-relations/edit-form";

Because the latter is, AFAIK, the correct one. I can't quite tell from the patch if that's what will happen, though, because of my first point above. Can someone confirm?

tedbow’s picture

ok this patch address @tim.plunkett's review in #62 and he also suggested to put in more @see tags from interfaces to manager.

As #63 1,2(not 3)

Status: Needs review » Needs work

The last submitted patch, 64: 2113345-64.patch, failed testing.

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/Http/LinkRelationManagerInterface.php
    @@ -0,0 +1,18 @@
    +/**
    + * Created by PhpStorm.
    + * User: ted.bowman
    + * Date: 8/1/16
    + * Time: 3:03 PM
    + */
    

    We know, no need to tell us. :-)

  2. +++ b/core/lib/Drupal/Core/Http/LinkRelationManagerInterface.php
    @@ -0,0 +1,18 @@
    +namespace Drupal\Core\Http;
    +use Drupal\Component\Plugin\PluginManagerInterface;
    

    Extra newline needed.

tedbow’s picture

Status: Needs work » Needs review
FileSize
31.38 KB
4.51 KB

We know, no need to tell us. :-)

Darn you foiled my plot to get my name into core. Fixed!

Extra newline needed.

Fixed

Also fixed a logic problem in addLinkHeaders

Shouldn't this be linkHeaders()? Plural? Since it's checking an array.

Yes changed.

Link: ; rel="edit-form";

or:

Link: ; rel="https://drupal.org/link-relations/edit-form";

It produces the 2nd 1.

I also had to change the order of the headers being sent to \Drupal\rest\Plugin\rest\resource\EntityResource::addLinkHeaders

This is because we are looping through
$entity->getEntityType()->getLinkTemplates()
and no longer
$this->linkRelationManager->getDefinitions()
This causes the relations to be in a different order but that shouldn't matter.

Status: Needs review » Needs work

The last submitted patch, 67: 2113345-67.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
31.37 KB
4.03 KB

Test failed b/c of new test #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available

This checks yaml syntax. Guess ''(double single quotes) is the problem. in the included yml file

Status: Needs review » Needs work

The last submitted patch, 69: 2113345-69.patch, failed testing.

dawehner’s picture

@tedbow
It would be great if you would have adapted the script https://www.drupal.org/files/issues/convert.php__9.txt instead of manually fixing it.

Wim Leers’s picture

Retesting. I can't reproduce the failure.

The last submitted patch, 69: 2113345-69.patch, failed testing.

The last submitted patch, 69: 2113345-69.patch, failed testing.

neclimdul’s picture

+++ b/core/core.link_relation.yml
@@ -0,0 +1,263 @@
+edit-form:
+  relationship: https://drupal.org/link-relations/edit-form
+  description: A form where a resource of this type can be created
...
+edit-form:
+  description: "The target IRI points to a resource where a submission form for\n      editing associated resource can be obtained."
+  reference: '[RFC6861]'

PECL and Symfony handle duplicate entries differently. Which is kinda good because you probably didn't want them anyways. :-D

tedbow’s picture

Status: Needs work » Needs review
FileSize
31.13 KB
2.66 KB

Ok. removed

+++ b/core/core.link_relation.yml
@@ -0,0 +1,263 @@
+edit-form:
...
+  description: A form where a resource of this type can be created

I am assuming we want to use the IANA version of "edit-form" instead of our own.

I have removed the occurrence of "https://drupal.org/link-relations/edit-form" in our test data.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.79 KB
31.46 KB

I found a bunch of nits, which I just fixed:

  1. +++ b/core/core.link_relation.yml
    @@ -0,0 +1,260 @@
    +# This are Drupal specific link relations.
    

    s/This/These/
    s/Drupal specific/Drupal-specific/

  2. +++ b/core/core.link_relation.yml
    @@ -0,0 +1,260 @@
    +# These are the one from IANA itself.
    

    s/one/ones/

  3. +++ b/core/lib/Drupal/Core/Http/LinkRelation.php
    @@ -0,0 +1,48 @@
    + * @todo pointing to actual documentation.
    

    I don't see what actual documentation this should point to. D.o handbook? We don't link to that from the code in general. Removed this.

  4. +++ b/core/lib/Drupal/Core/Http/LinkRelation.php
    @@ -0,0 +1,48 @@
    +  public function getDescription() {
    +  }
    

    This is missing.

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -51,6 +53,13 @@ class EntityResource extends ResourceBase implements DependentPluginInterface {
    +   * @var \Drupal\Component\Plugin\PluginManagerInterface
    
    @@ -67,11 +76,14 @@ class EntityResource extends ResourceBase implements DependentPluginInterface {
    +   * @param \Drupal\Component\Plugin\PluginManagerInterface $link_relation_manager
    

    Wrong typehints.

kristiaanvandeneynde’s picture

+++ b/core/core.link_relation.yml
@@ -0,0 +1,260 @@
+  description: "Refers to a collection of records, documents, or other\n      materials of historical interest."
...
+  description: "Identifies the entity that blocks access to a resource\n      following receipt of a legal demand."
...
+  description: "Refers to a copyright statement that applies to the\n    link's context."

As mentioned by yourself before: We should remove those newlines :)

Wim Leers’s picture

kristiaanvandeneynde’s picture

Did a quick search and replace.

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 80: 2113345-80.patch, failed testing.

The last submitted patch, 80: 2113345-80.patch, failed testing.

kristiaanvandeneynde’s picture

I don't get it, the test looks green. Does that mean it went red against 8.3.x?

dawehner’s picture

Status: Needs work » Needs review

I bet this was just a random failure somehow.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Just back to RTBC. I RTBC'd in #77, and #80 is a trivial search-and-replace of purely textual data (not code!), but testbot just fluked. So no reason not to go back to the previous status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 80: 2113345-80.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs reroll
tedbow’s picture

Status: Needs work » Needs review
FileSize
30.84 KB
490 bytes

A re-roll of #80. Also a small fix of the namespace case being wrong in \Drupal\KernelTests\Core\Http\LinkRelationsTest

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks!

Grayside’s picture

In looking through the code, it appears we're constrained to a single URL per relationship. Is that intended? There are some relations where multiple is common, such as describedBy, or things related to collections.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

I don't think anybody in this issue was aware of that, to be honest. I pinged dawehner, klausi, Crell, e0ipso and neclimdul for feedback.

Thanks for your continued insightful questions and remarks in the rest.module issue queue. It's greatly, greatly appreciated! Let me know if you want to get involved more closely.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Interesting point @Grayside
Its right, potentially every HTTP header allows you to specify acually multiple values, which is why PSR7 actually defines headers to be always an array.

At the moment though the code which creates those references cannot do anything about that. Link templates on entity types do currently not support an array of values,
so if we would like to support that, we would need to change way more than just the rest side of things. This is not an argument against doing it in general, but it would be just out of scope here. Do you mind opening up a follow up to potentially discuss that?

+        $response->headers->set('Link', $link_header, FALSE);

At least for now one could set a custom additional header already.

Grayside’s picture

Crell’s picture

I concur with dawehner. The single-link limitation is pre-existing, and this issue doesn't make it any better or worse.

Wim Leers’s picture

Thanks, @dawehner & @Crell! :) Much appreciated.

klausi’s picture

Status: Reviewed & tested by the community » Needs work

Good idea, I like the approach. Just some minor nitpicks:

  1. +++ b/core/lib/Drupal/Core/Http/LinkRelationInterface.php
    @@ -0,0 +1,63 @@
    +   * @return string
    +   *
    

    Return description missing.

  2. +++ b/core/lib/Drupal/Core/Http/LinkRelationManagerInterface.php
    @@ -0,0 +1,13 @@
    +/**
    + * Provides a plugin manager for link relations.
    + *
    + * @see \Drupal\Core\Http\LinkRelationInterface
    + */
    +interface LinkRelationManagerInterface extends PluginManagerInterface {
    +}
    

    why do we need an empty interface? Please add a comment.

    I think this interface should be removed, because we cannot add new methods to interfaces later anyway during the Drupal 8 release cycle.

  3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -333,4 +349,33 @@ public function calculateDependencies() {
    +  /**
    +   * Adds link headers to a response.
    +   *
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   *   The entity.
    +   * @param \Drupal\Core\Cache\CacheableResponseInterface|\Symfony\Component\HttpFoundation\Response $response
    +   *   The response.
    +   *
    +   * @see https://tools.ietf.org/html/rfc5988#section-5
    +   */
    +  protected function addLinkHeaders(EntityInterface $entity, CacheableResponseInterface $response) {
    

    second @param type is wrong, this method does not accept a Symfony response object. I think the type hint should be changed to the symfony response object, right?

    Then we should check for CacheableResponseInterface in the method and only then invoke addCacheableDependency() on it.

  4. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -550,4 +551,29 @@ protected function configEntityValues($entity_type_id) {
    +   * @param array $link_relationships
    +   *   The used link relationships.
    

    array of what? Should this be string[]?

  5. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -550,4 +551,29 @@ protected function configEntityValues($entity_type_id) {
    +   * @return bool
    +   */
    

    Return description missing.

    Note: with the migration to PHPUnit we remove all return values of assertion methods. So it would be forward thinking if this method would not have a return value at all :)

  6. +++ b/core/modules/rest/src/Tests/ReadTest.php
    @@ -118,9 +118,13 @@ public function testRead() {
             $this->assertResponse(200);
    -        $this->assertHeader('content-type', $this->defaultMimeType);
             $data = Json::decode($response);
             $this->assertFalse(isset($data['field_test_text']), 'Field access protected field is not visible in the response.');
    +        $this->assertLinkHeaders($entity, [
    +          'canonical' => 'canonical',
    +          'add-form' => 'https://drupal.org/link-relations/add-form',
    +          'edit-form' => 'edit-form',
    +        ]);
    

    Why is the line removed? Looks unrelated to this patch, the content type should still be the same?

  7. +++ b/core/tests/Drupal/KernelTests/Core/Http/LinkRelationsTest.php
    @@ -0,0 +1,29 @@
    +
    +  public function testAvailableLinkRelationships() {
    

    doc block missing.

  8. +++ b/core/tests/Drupal/KernelTests/Core/Http/LinkRelationsTest.php
    @@ -0,0 +1,29 @@
    +    $link_relation_manager = \Drupal::service('plugin.manager.link_relation');
    

    always avoid using \Drupal, use $this->container->get() instead.

tedbow’s picture

Status: Needs work » Needs review
FileSize
30.69 KB
8.25 KB

@klausi thanks for the review
1. fixed
2. removed the interface
3. Changed param to Response and checked for interface with if
4. change param hint to string[]
5. removed return from function
6. Added back line
7. Added doc block
8. changed to $this->container->get()

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

AFAICT that addressed every remark correctly :) Thanks, @klausi & @tedbow!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 2113345-98.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Patch is still green. Likely failed due to the testbot/l.d.o problems of last week.

Wim Leers’s picture

Title: Define a mechanism for custom link relationships » [PP-1] Define a mechanism for custom link relationships
Status: Reviewed & tested by the community » Postponed

Postponing on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, because this is updating test coverage that that patch is making obsolete.

Wim Leers’s picture

Title: [PP-1] Define a mechanism for custom link relationships » Define a mechanism for custom link relationships
Status: Postponed » Needs work
tedbow’s picture

Status: Needs work » Needs review
FileSize
26.95 KB

Just a re-roll with changes to old tests removed.

Still need @todo

This patch is currently still updating the "old" test coverage. We should update it to instead update #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method test coverage.

Status: Needs review » Needs work

The last submitted patch, 104: 2113345-104.patch, failed testing.

Wim Leers’s picture

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.75 KB
31.69 KB

Ok here is patch that should get some(all?) of the current failures passing.
Had to add 'url.site' to \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getExpectedCacheContexts. This removes more lines than adds which feels good.

I have NOT looked at whether we need to test for new things in the REST tests right now.

dawehner’s picture

While scanning through the code I realized that we don't cache the result of the plugin manager. This results into yml scanning on every request potentially.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

This looks great! :) Back to RTBC, just like in #99 (October 13, 2016). Only minor changes since then. Let's get this in!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 108: 2113345-108.patch, failed testing.

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

#108 had a CI error on 1 run. Back to green now

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 108: 2113345-108.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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.

effulgentsia’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: -API change

Doesn't look to me like this patch has any API changes, so untagging that. Also, I think this is still fine to get into 8.3, certainly until the alpha, so moving the version back to that.

  1. --- /dev/null
    +++ b/core/core.link_relation.yml
    

    Most of our other YML files that add multiple things per file use a plural in the filename. So, can we change this to core.link_relations.yml?

  2. +++ b/core/core.link_relation.yml
    @@ -0,0 +1,261 @@
    +# These are Drupal-specific link relations.
    +add-form:
    +  relationship: https://drupal.org/link-relations/add-form
    +  description: A form where a resource of this type can be created
    +create:
    +  relationship: https://drupal.org/link-relations/create
    +  description: A REST resource URL where a resource of this type can be created
    

    delete-form and possibly other Drupal-specific ones currently used by entity types isn't in here. Is that a problem? Is there an eventual plan to validate the link key names used in entity type annotations?

    Also, the descriptions should end with a period, for consistency with the IANA ones.

  3. +++ b/core/lib/Drupal/Core/Http/LinkRelationInterface.php
    @@ -0,0 +1,64 @@
    +  /**
    +   * If the relationship is not a IANA one, provide the URL to the definition.
    +   *
    +   * @return string
    +   *   The relationship URL.
    +   */
    +  public function getRelationshipUrl();
    

    Docs don't specify what the expected behavior is for IANA relationships.

  4. +++ b/core/lib/Drupal/Core/Http/LinkRelation.php
    @@ -0,0 +1,47 @@
    +  public function getRelationshipUrl() {
    +    return isset($this->pluginDefinition['relationship']) ? $this->pluginDefinition['relationship'] : $this->getName();
    +  }
    

    It's confusing to return getName(), which is not a URL, in a method named get*Url().

  5. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -125,6 +139,8 @@ public function get(EntityInterface $entity) {
    +    $this->addLinkHeaders($entity, $response);
    

    This looks like a behavior change (addition), and therefore, something we should add test coverage for.

Wim Leers’s picture

Status: Needs review » Needs work
  1. Well-spotted!
  2. Again well-spotted. I think that's just an oversight. Also, it really doesn't matter. What matters here is the infrastructure. It's easy to add more link relations later!
  3. So this method should be renamed to getRelationshipIanaNameOrUrl()?
  4. This was recommended by @Crell in #63.1. Hence my suggestion for point 3.
  5. You're right. We should add link header assertions to \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet().

This also made me notice that it's LinkRelationInterface but LinkRelationInterface::getRelationshipUrl(). i.e. relation vs relationship. Should that not be made consistent?

dawehner’s picture

FileSize
32.23 KB
2.46 KB

It's easy to add more link relations later!

Yeah afaik this issue was kind of start.

So this method should be renamed to getRelationshipIanaNameOrUrl()?

Good idea!

This addressed 1. to 4. I'll not be able to work on this particular issue due to personal choice though.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
34.21 KB

Thanks, @dawehner!

This:

  1. updates method docs slightly
  2. adds the delete-form and revision link relations
  3. adds test coverage

Status: Needs review » Needs work

The last submitted patch, 118: 2113345-118.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
35.23 KB

All of those failures in #118 are due to the test coverage I added, this has given us the answer to #115.2, where @effulgentsia asked:

delete-form and possibly other Drupal-specific ones currently used by entity types isn't in here. Is that a problem?

This patch adds all missing ones!

Now it should be green.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
20.01 KB
37.55 KB
16:30:15 <WimLeers> dawehner: what are your thoughts about "This also made me notice that it's LinkRelationInterface but LinkRelationInterface::getRelationshipUrl(). i.e. relation vs relationship. Should that not be made consistent?" ?
16:32:37 <dawehner> WimLeers: I would the term which is used in the RFC
16:32:44 <dawehner> WimLeers: I thought that is relation, but I'm not 100% sure anymore

https://tools.ietf.org/html/rfc5988 uses both "relationship" and "relation". 99% of the time it uses "relation" though.

Also:

16:42:29 <WimLeers> https://tools.ietf.org/html/rfc5988#section-6.2.1
16:42:33 <WimLeers> that says "relation name"
16:42:39 <WimLeers> it should be "relation type name"
16:42:40 <WimLeers> so gahhhh
16:42:49 <WimLeers> they're internally inconsistent
…
16:45:42 <WimLeers> and https://tools.ietf.org/html/rfc5988#section-4.1 + https://tools.ietf.org/html/rfc5988#section-4.2 indicate what the name of the getRelationshipIanaNameOrUrl() method should be
16:45:45 <WimLeers> ok, I'll work on renaming
17:15:53 <dawehner> WimLeers++

https://tools.ietf.org/html/rfc5988#section-4 says:

There are two kinds of relation types: registered and extension.

So I think it makes sense to introduce two constants: KIND_REGISTERED and KIND_EXTENSION. Then we can have a getKind() method. Which means we can do:

$rel_in_link_header = ($relation->getKind() === KIND_REGISTERED) ? $relation->getName() : $relation->getUri();

instead of:

$rel_in_link_header = $relation->getRelationshipIanaNameOrUrl();

or, instead, introduce isRegistered() (and perhaps also isExtension()), which then allows:

$rel_in_link_header = $relation->isRegistered() ? $relation->getName() : $relation->getUri();

But getName() only makes sense for "registered", and getUri() only makes sense for "extension", so getRegisteredName() and getExtensionUri() make more sense. That'd then become:

$rel_in_link_header = $relation->isRegistered() ? $relation->getRegisteredName() : $relation->getExtensionUri();

I bounced back and forth with @effulgentsia for the second half of what I wrote above. He +1'd it.


So this:

  1. renames core.link_relations.yml to core.link_relation_types.yml
  2. renames the relationship key in the YAML file to uri (per @see https://tools.ietf.org/html/rfc5988#section-4.2)
  3. renames the plugin.manager.link_relation service to plugin.manager.link_relation_type
  4. renames \Drupal\Core\Http\LinkRelationManager to \Drupal\Core\Http\LinkRelationTypeManager
  5. renames \Drupal\Core\Http\LinkRelation(Interface) to \Drupal\Core\Http\LinkRelationType(Interface)
  6. adds LinkRelationTypeInterface::isRegistered()
  7. adds LinkRelationTypeInterface::isExtension()
  8. renames LinkRelationInterface::getName() to \Drupal\Core\Http\LinkRelationTypeInterface::getRegisteredName()
  9. removes LinkRelationInterface::getRelationshipIanaNameOrUrl() in favor of its successor, \Drupal\Core\Http\LinkRelationTypeInterface::getExtensionName()
  10. and finally: expands the test coverage in LinkRelationsTest

I've only touched the patch to add test coverage and rename things. Plus, I made these changes in close collaboration after explicit approval of @effulgentsia. Finally, the 8.3 deadline is looming, and this patch has been blocking #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available since August 3, so we're approaching the "blocking for half a year" mark!

Wim Leers’s picture

#121's interdiff is correct, but the patch is not. A few relationship keys were not yet converted to key.

The last submitted patch, 121: 2113345-120.patch, failed testing.

effulgentsia’s picture

#122 looks great. Ticking some credit boxes.

effulgentsia’s picture

And one more.

effulgentsia’s picture

This patch removes unused use statements.

  • effulgentsia committed 0bc5498 on 8.4.x
    Issue #2113345 by dawehner, tedbow, Wim Leers, kristiaanvandeneynde,...

  • effulgentsia committed 00dde3c on 8.3.x
    Issue #2113345 by dawehner, tedbow, Wim Leers, kristiaanvandeneynde,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.4.x and cherry picked to 8.3.x.

A follow-up issue for the following would be great though:

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -340,4 +356,35 @@ public function calculateDependencies() {
+        $relationship = $relation_name;
+        if (!empty($definition['uri'])) {
+          $relationship = $definition['uri'];
+        }

Would be nice to use the interface here instead of the definition array. But that requires instantiating the plugin, which maybe has a performance cost that needs to be looked at. Which is why I'm ok punting it to a follow-up.

Wim Leers’s picture

Yay, this finally unblocked #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available. Rerolling that in the morning. Will open that follow-up in the morning as well.

Wim Leers’s picture

I said "in the morning". Turns out it took two full days to get #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available to a working state again. I encountered lots of REST misery along the way. That patch is now reviewable and hopefully RTBC'able :)

The requested follow-up is at #2848927: EntityResource::addLinkHeaders() should use LinkRelationType plugin instances rather than definitions. Patch included. Did the first half, the second half is a great novice task.

P.S.: should this issue not have a CR? I think it should.

Status: Fixed » Closed (fixed)

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

hchonov’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -70,11 +80,14 @@ class EntityResource extends ResourceBase implements DependentPluginInterface {
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, $serializer_formats, LoggerInterface $logger, ConfigFactoryInterface $config_factory) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, $serializer_formats, LoggerInterface $logger, ConfigFactoryInterface $config_factory, PluginManagerInterface $link_relation_type_manager) {

@@ -88,7 +101,8 @@ public static function create(ContainerInterface $container, array $configuratio
-      $container->get('config.factory')
+      $container->get('config.factory'),
+      $container->get('plugin.manager.link_relation_type')

Has that been an accepted API change? We just updated and got exceptions because we extend the constructors ... I think that in such cases we have to do it like in e.g. ContentEntityForm and define the new parameter as NULL and if not provided then retrieve it through \Drupal::service in the constructor, otherwise all classes extending from it will break. I guess we will be not the only ones affected.

dawehner’s picture

Constructors are not APIs ...

hchonov’s picture

Ok, I thought that we could not simply change them between releases and this is why we are defining new parameters in ContentEntityForm as NULL. So it means I didn't understand something properly...