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.
- 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.
- 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.
- 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.
Related Issues
#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
Comment | File | Size | Author |
---|---|---|---|
#126 | interdiff.txt | 3.36 KB | effulgentsia |
#126 | 2113345-126.patch | 39.07 KB | effulgentsia |
#122 | 2113345-122.patch | 37.5 KB | Wim Leers |
#121 | 2113345-120.patch | 37.55 KB | Wim Leers |
#121 | interdiff.txt | 20.01 KB | Wim Leers |
Comments
Comment #1
andypostAlso 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
fragmentComment #2
linclark CreditAttribution: linclark commentedThat issue would remain the same even if we make this change.
Comment #3
linclark CreditAttribution: linclark commentedAdded third problem:
Comment #4
Crell CreditAttribution: Crell commentedWe 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:
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:
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.
Comment #4.0
Crell CreditAttribution: Crell commentedAdded new problem.
Comment #4.1
Crell CreditAttribution: Crell commentedAdd more related links
Comment #5
Crell CreditAttribution: Crell commentedI 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?
Comment #6
yukare CreditAttribution: yukare commentedI 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.
Comment #7
pwolanin CreditAttribution: pwolanin commentedIt would be helpful to update/clarify the issue summary since the annotations now refer to routes instead of paths.
Comment #8
Crell CreditAttribution: Crell commentedpwolanin: The issue you mention needs to be rolled back precisely because it breaks this and other patches.
Comment #9
linclark CreditAttribution: linclark commentedI do not have the bandwidth to take this on. @yukare, since you have a use case in mind, could you take this on?
Comment #10
juampynr CreditAttribution: juampynr commentedI could help with this too, but I need further guidance.
Comment #11
Crell CreditAttribution: Crell commentedRetitling 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.
Comment #12
xjmIs this issue critical, as in, we would not release Drupal 8.0 without it? It sounds like maybe it should be based on:
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!
Comment #13
Crell CreditAttribution: Crell commentedAlthough 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.
Comment #14
catchLet's not block beta on it then, sounds like the API change is mostly theoretical.
Comment #15
Crell CreditAttribution: Crell commentedReactivating per #2259445: Entity Resource unification.
Comment #16
andypostAnother problem here is "bundlability" that breaks "create-form" because it needs mostly always second argument (bundle)
Comment #17
Crell CreditAttribution: Crell commentedThat 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.)
Comment #18
mgiffordComment #19
catchSounds 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.
Comment #20
Crell CreditAttribution: Crell commentedI 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.
Comment #21
Crell CreditAttribution: Crell at Palantir.net commentedI consider this 8.1-safe, so refiling.
Comment #23
Wim LeersComment #24
dawehnerIn 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.
Comment #26
dawehnerNow we could for example expose them via HTTP, not sure though whether that implementation is even remotely right.
Comment #28
kristiaanvandeneyndeThe 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?
Comment #29
dawehnerWell, this code is about HTTP headers actually, not about HTML.
I thought we already do.
Comment #30
kristiaanvandeneyndeHah, never mind me! In that case the code looks about right.
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.
Comment #31
dawehnerOh yeah, that is a good idea indeed!
Comment #32
Crell CreditAttribution: Crell commentedThis is a great start, dawehner, thank you!!! I especially love that you've included, it looks like, the entire IANA registry by default. :-)
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.
Nit: Why is the temporary $plugin_interface variable needed?
Dreditor says whitespace.
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.
Side note: I don't know if I love or hate this line...
Trailing whitespace, says Dr. Editor.
Comment #33
dawehnerThank you for your review crell!
Good catch, yeah there is no reason for it.
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.
Comment #36
dawehnerRerolled
Comment #38
dawehnerWorked a bit on it.
Comment #39
dawehnerOh yeah a new script
Comment #41
dawehnerThis time even with a working patch.
Comment #42
Wim LeersMoving to REST module, because this does not affect the entity system anywhere; this only affects core itself +
rest.module
.These newlines look bizarre… I think they're just there because of copy/pasting? Let's remove the newlines?
IANA name?
IANA name description?
Upon first reading this, I have absolutely no idea what this means.
Fancy! :)
I'd personally put the first array on a single line, that'd greatly improve legibility.
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 anassert('$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.
This formatting looks bizarre.
Comment #43
Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commentedMaybe 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?
Comment #44
dawehnerAny idea how I can adapt the script to ensure this happens?
IANA is just one of the instances which define the possible link relations. The standard itself is part of IETF
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 hope this change improves the readability.
Well, it would be an API break if we require custom link relations definitions here. I doubt we can/should do it 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.
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.
Comment #45
tedbowMuch more readable to me!
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:
This seems more accurate.
Comment #46
Wim LeersBut 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.
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 beforeEntityResource
can send such link headers?Comment #47
dawehnerWell, 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.
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:
Comment #48
Wim LeersI 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.
Comment #49
dawehnerWell, 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.
Comment #50
Wim LeersOk.
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.
Let's do this.
s/relationships/relations/
Nit: missing trailing period.
Nit: strange formatting.
Comment #51
Crell CreditAttribution: Crell commentedThank 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.
Comment #52
dawehnerThank you @crell and @Wim Leers!
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.
Comment #53
dawehner@Crell
IN these cases, checkout
localStorage
in your browser. There is a high chance it contains information you want.
Comment #54
Crell CreditAttribution: Crell commentedEr, 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?
Comment #55
dawehnerThis addresses the feedback from crell
Comment #57
dawehnerLet'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 :)
Comment #58
tedbowRe-rolled #57.
Also address nits in #50
Also
Is not calling the parent constructor intentional here? I didn't fix(if needed) in patch
Comment #59
Wim LeersThat looks like an important oversight indeed.
Comment #60
tedbowRe:#59
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.
Comment #61
dawehnerYeah, 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.
Comment #62
tim.plunkettAs a service, this should have an interface, even if it just extends another one.
Extra line
Comment #63
Crell CreditAttribution: Crell commentedIf there isn't one specified, should it default to the plugin name? Since that's what the IANA case is?
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?
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?
Comment #64
tedbowok 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)
Comment #66
Crell CreditAttribution: Crell commentedWe know, no need to tell us. :-)
Extra newline needed.
Comment #67
tedbowDarn you foiled my plot to get my name into core. Fixed!
Fixed
Also fixed a logic problem in addLinkHeaders
Yes changed.
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.
Comment #69
tedbowTest 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
Comment #71
dawehner@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.
Comment #72
Wim LeersRetesting. I can't reproduce the failure.
Comment #75
neclimdulPECL and Symfony handle duplicate entries differently. Which is kinda good because you probably didn't want them anyways. :-D
Comment #76
tedbowOk. removed
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.
Comment #77
Wim LeersI found a bunch of nits, which I just fixed:
s/This/These/
s/Drupal specific/Drupal-specific/
s/one/ones/
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.
This is missing.
Wrong typehints.
Comment #78
kristiaanvandeneyndeAs mentioned by yourself before: We should remove those newlines :)
Comment #79
Wim Leers#78: yes, but let's not hold up a patch for that. That is super trivial clean-up. This blocks #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available.
Comment #80
kristiaanvandeneyndeDid a quick search and replace.
Comment #84
kristiaanvandeneyndeI don't get it, the test looks green. Does that mean it went red against 8.3.x?
Comment #85
dawehnerI bet this was just a random failure somehow.
Comment #86
Wim LeersJust 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.
Comment #88
Wim LeersComment #89
tedbowA re-roll of #80. Also a small fix of the namespace case being wrong in \Drupal\KernelTests\Core\Http\LinkRelationsTest
Comment #90
Wim LeersThanks!
Comment #91
Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commentedIn 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.
Comment #92
Wim LeersI 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.Comment #93
dawehnerInteresting 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?
At least for now one could set a custom additional header already.
Comment #94
Grayside CreditAttribution: Grayside at Phase2 for Norwegian Cruise Line commentedComment #95
Crell CreditAttribution: Crell commentedI concur with dawehner. The single-link limitation is pre-existing, and this issue doesn't make it any better or worse.
Comment #96
Wim LeersThanks, @dawehner & @Crell! :) Much appreciated.
Comment #97
klausiGood idea, I like the approach. Just some minor nitpicks:
Return description missing.
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.
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.
array of what? Should this be string[]?
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 :)
Why is the line removed? Looks unrelated to this patch, the content type should still be the same?
doc block missing.
always avoid using \Drupal, use $this->container->get() instead.
Comment #98
tedbow@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()
Comment #99
Wim LeersAFAICT that addressed every remark correctly :) Thanks, @klausi & @tedbow!
Comment #101
Wim LeersPatch is still green. Likely failed due to the testbot/l.d.o problems of last week.
Comment #102
Wim LeersPostponing 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.
Comment #103
Wim Leers#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method landed!
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's test coverage.
Comment #104
tedbowJust a re-roll with changes to old tests removed.
Still need @todo
Comment #106
Wim LeersThis 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! Let's get this done.
Comment #107
tedbowOk 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.
Comment #108
dawehnerWhile 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.
Comment #109
Wim LeersThis looks great! :) Back to RTBC, just like in #99 (October 13, 2016). Only minor changes since then. Let's get this in!
Comment #111
tedbow#108 had a CI error on 1 run. Back to green now
Comment #113
Wim LeersComment #115
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDoesn'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.
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
?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.
Docs don't specify what the expected behavior is for IANA relationships.
It's confusing to return getName(), which is not a URL, in a method named
get*Url()
.This looks like a behavior change (addition), and therefore, something we should add test coverage for.
Comment #116
Wim LeersgetRelationshipIanaNameOrUrl()
?\Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet()
.This also made me notice that it's
LinkRelationInterface
butLinkRelationInterface::getRelationshipUrl()
. i.e.relation
vsrelationship
. Should that not be made consistent?Comment #117
dawehnerYeah afaik this issue was kind of start.
Good idea!
This addressed 1. to 4. I'll not be able to work on this particular issue due to personal choice though.
Comment #118
Wim LeersThanks, @dawehner!
This:
delete-form
andrevision
link relationsComment #120
Wim LeersAll 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:
This patch adds all missing ones!
Now it should be green.
Comment #121
Wim Leershttps://tools.ietf.org/html/rfc5988 uses both "relationship" and "relation". 99% of the time it uses "relation" though.
Also:
https://tools.ietf.org/html/rfc5988#section-4 says:
So I think it makes sense to introduce two constants:
KIND_REGISTERED
andKIND_EXTENSION
. Then we can have agetKind()
method. Which means we can do:instead of:
or, instead, introduce
isRegistered()
(and perhaps alsoisExtension()
), which then allows:But
getName()
only makes sense for "registered", andgetUri()
only makes sense for "extension", sogetRegisteredName()
andgetExtensionUri()
make more sense. That'd then become:I bounced back and forth with @effulgentsia for the second half of what I wrote above. He +1'd it.
So this:
core.link_relations.yml
tocore.link_relation_types.yml
relationship
key in the YAML file touri
(per @see https://tools.ietf.org/html/rfc5988#section-4.2)plugin.manager.link_relation
service toplugin.manager.link_relation_type
\Drupal\Core\Http\LinkRelationManager
to\Drupal\Core\Http\LinkRelationTypeManager
\Drupal\Core\Http\LinkRelation(Interface)
to\Drupal\Core\Http\LinkRelationType(Interface)
LinkRelationTypeInterface::isRegistered()
LinkRelationTypeInterface::isExtension()
LinkRelationInterface::getName()
to\Drupal\Core\Http\LinkRelationTypeInterface::getRegisteredName()
LinkRelationInterface::getRelationshipIanaNameOrUrl()
in favor of its successor,\Drupal\Core\Http\LinkRelationTypeInterface::getExtensionName()
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!
Comment #122
Wim Leers#121's interdiff is correct, but the patch is not. A few
relationship
keys were not yet converted tokey
.Comment #124
effulgentsia CreditAttribution: effulgentsia at Acquia commented#122 looks great. Ticking some credit boxes.
Comment #125
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAnd one more.
Comment #126
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis patch removes unused use statements.
Comment #129
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.4.x and cherry picked to 8.3.x.
A follow-up issue for the following would be great though:
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.
Comment #130
Wim LeersYay, 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.
Comment #131
Wim LeersI 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.
Comment #133
hchonovHas 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.
Comment #134
dawehnerConstructors are not APIs ...
Comment #135
hchonovOk, 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...