Updated: Comment #103

Problem/Motivation

Currently, there is no central definition of the path to an entity. Sure there's its route/menu item, but we can't reverse associate from that.

Sure entities now have a uri() method, but you need an entity object for that. Also, the default implementation still uses a uri callback function, for no reason I can fathom.

In rest.module, we need to be able to dynamically generate paths to not individual entities, but entity types. That is, we need to be able to dynamically generate the route path pattern for an entity.

Field API still uses hook_menu-style paths for wiring up the admin pages. That's... going to break as soon as hook_menu-based routes go away.

A number of other issues are circling the same problem space of needing to deal with links to entities in the abstract. Among them:

#1783964: Allow entity types to provide menu items
#1839516: Introduce entity operation providers

Proposed resolution

Bite the bullet. What we really need here is to define a canonical path TEMPLATE for an entity type. When I say template, I mean the uri-template specification, RFC 6570:

http://tools.ietf.org/html/rfc6570

A subset of that specification is what Symfony/Drupal 8 use for route patterns, and is also rapidly becoming the standard for REST services anyway. (I'm using it for a non-Drupal HAL-based project now.)

In particular, we should define multiple such patterns. One for each operation, as identified by a link. Did I say operation, as in relevant to #1839516: Introduce entity operation providers? Why yes I did. :-)

What does "identified by a link" mean? I mean using one of the IANA standardized link relationships (which collects relationships defined by IETF RFCs as well as the W3C):

http://www.iana.org/assignments/link-relations/link-relations.xml

It's possible to define your own, but there's plenty there we can already use. And, we can then use that standardized knowledge all over the place; for entity operations, for rest.module, for link tags in HTML (semantic web, baby!), all kinds of stuff!

The natural way to identify such patterns, IMO, is in the annotation for an entity. It's data we explicitly want in the absence of an actual entity object, which therefore ends up in the annotation.

To wit, nodes might look something like:

links = {
  "canonical" = "/node/{node}",
  "edit-form" = "/node/{node}/edit",
  "create-form" = "/node/add/{entity_subtype}",
  "version-history" = "/node/{node}/revisions"
}

For Drupal-specific logic, we can add additional operations whose link is a namespaced property. For instance:

links = {
  // ...
  "drupal:field-configuration" = "admin/structure/types/manage/{entity_subtype}"
}

(To be strict, we should then define drupal:field-configuration as an alias for a URI that describes what the heck that means, but we may skip that for the moment.)

Benefits:

  • We can, likely, generate the routes for all of those operations dynamically, thus resolving #1783964: Allow entity types to provide menu items. That would also imply a standard name format for entity-based routes, which would be good for DX predictability.
  • rest.module can generate RESTful routes for all entity types it needs that match the path of the HTML version of the entity, which is what we want.
  • rest.module can inject all of those relationships into the _links array of an entity when serialized, meaning we can tell client apps everything they need to know about how to use an entity, using industry-standard definitions. Hypermedia FTW.
  • We can do the same in HTML <link> elements. Hypermedia FTW, again. (What could we do with that information client-side? The mind boggles!)
  • We can greatly simplify the logic in Entity::uri(), As it would just need to grab the canonical link and str_replace() in the entity ID. Or, get this, uri() can take *any* supported relation and generate that link. To wit:
    public function uri($relation = 'canonical') {
      $info = $this->entityInfo();
      $pattern = $info['links'][$relation];
      // Do some str_replace() to turn $pattern into a complete string.
      return $uri;
    }
    

    (Or soething along those lines.) And now you can ask an arbitrary entity what the URI of that entity's edit form is, or its Field UI configuration page. How cool is that??

  • It's not a complete entity operations API, but it can be leveraged by one fairly easily.
  • I *think* (although I'm not certain), that this would allow for entity-specific paths that include other entities, such as /node/{node}/comment/{comment} or /myentity/{entity_subtype}/{myentity}, if any custom entities were so inclined. Because uri() is responsible for generating the actual links, and it can know its context and the meaning of different placeholders, that class would know how to get at the data it needs for that placeholder.

We likely could not support the entire uri-template spec (the rules around query parameters are a bit tricky), but we can support enough of it to get done what we need to get done.

Remaining tasks

Agree on the above. :-) And then someone needs to code it, stat!

User interface changes

Probably none, or minimal.

API changes

hook_enity_bundle_info() becomes thinner, which it needs to since it's still designed for the old routing system.

Entity::uri() becomes a lot simpler.

We probably lose the ability to do per-bundle paths. But, really, with path aliases I don't think that's a problem.

Issues this would help, replace, or obsolete:

#1783964: Allow entity types to provide menu items
#1839516: Introduce entity operation providers
#1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation
#2046289: Deprecate entity uri procedural callbacks (uri_callback)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

+1 about all of this.

joachim’s picture

> I *think* (although I'm not certain), that this would allow for entity-specific paths that include other entities, such as /node/{node}/comment/{comment} or /myentity/{entity_subtype}/{myentity}, if any custom entities were so inclined. Because uri() is responsible for generating the actual links,

I managed this on D7 with my Entity Operations module by passing the menu %map to my entity's loader function. Obviously, that's all obsolete for D8...

If other modules can see that for node's there's "canonical" = "/node/{node}", presumably that means they can easily define new tabs on a node, for things such as translation?

(I felt a bit out of my depth on #1839516: Introduce entity operation providers, but that pales in comparison to this one! :/ )

sun’s picture

Overall, this makes sense to me and looks like a sensible direction.

The only part I'm missing and what I think we have to cater for are multiple URI templates / endpoints per entity [[sub]type], as discussed in #1332058-8: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones

I.e., to some extent, "entity URI contexts", if you will.


Also, the URI callbacks are still actively used by things like Forum module, in order to dynamically override the URI of a taxonomy term page depending on the vocabulary it is contained in. I'd rather not touch them here. Related issues:
#1899816: Various config entity types are missing URI callbacks
#1907208: Some config entities do not respect uri callbacks

Gábor Hojtsy’s picture

The http://drupal.org/project/config_translation module also goes over its own definition hooks to reverse-map paths to config entities, so it can put in translation tabs on standard entity paths. Also, it adds operations to entity listings, where #1839516: Introduce entity operation providers comes into play to put the added entity links/operations into operations dropdowns for example.

plach’s picture

@Crell:

The outlined plan sounds good. In contrib ET we had the need to support different edit links, as the Media module provides an alternative UI to edit files at a URL different from file/%file/edit. Perhaps it would make sense to allow to specify more than one template for each link.

Damien Tournoud’s picture

This looks like a long-overdue refactoring.

Just one thing: custom link relation types have to be absolute URLs according to RFC 5988 (which established the IANA Link relation types registry):

Note that extension relation types are REQUIRED to be absolute URIs
in Link headers, and MUST be quoted if they contain a semicolon (";")
or comma (",") (as these characters are used as delimiters in the
header itself).

As a consequence, this is *not* valid:

For Drupal-specific logic, we can add additional operations whose link is a namespaced property. For instance:

links = {
  // ...
  "drupal:field-configuration" = "admin/structure/types/manage/{entity_subtype}"
}

Also, there is a big difference between actions relating to an entity and actions relating to an entity type. I don't see the point of having a drupal:field-configuration link on a particular entity. But a particular entity can have a relation to the entity type, and from there a relation to the field configuration.

Crell’s picture

DamZ: Actually from section 4.2:

When extension relation types are compared, they MUST be compared as
strings (after converting to URIs if serialised in a different
format, such as a Curie [W3C.CR-curie-20090116]) in a case-
insensitive fashion, character-by-character. Because of this, all-
lowercase URIs SHOULD be used for extension relations.

Curies are here:
http://www.w3.org/TR/curie/

If we want to be fully semantically compliant for custom relationships, that's what I'd recommend. Although even with just the standard link relationships we can get a lot of mileage.

Tim Plunkett also mentioned the entity vs. entity-type difference as a potential concern. We can perhaps hold off on entity-type for now, as IMO the entity-based links are more important. (They relate to data semantics, whereas the entity-type ones relate mostly to automating admin page generation.) I think something along similar lines, though, would be helpful for that as well.

plach: Hypermedia would allow for multiple URIs per link, although multiple link templates may get tricky. Not sure there.

joachim: I want to really try to avoid modules treating these link templates as anything other than opaque strings. I suppose we couldn't stop a module from adding more links by using another link, but post-definition they should be treated as opaque strings with a placeholder you regex into.

sun: I'm not entirely sure I follow. Any entity we want to be publicly referenceable, even if it doesn't have an HTML page, needs a canonical URI template. So "admin entities" (like DamZ describes in that linked comment) would still need a URI. We can have a default of /entity/{entity_type}/{entity_id} like we do now, which is fine. The only question there would be whether or not to generate an HTML route for that URI, or just let rest module do it for non-HTML routes.

As for uri callbacks, stubbing out to a function is unquestionably Wrong(tm) in this case. If a particular entity wants to subclass and customize its uri() method it certainly can do so, but a method that is just a dumb wrapper around a procedural callback sets off all sorts of code smell alarms.

Gabor: There are various link relationships defined for collections of various sorts. We can investigate which of those to use, too.

joachim’s picture

> joachim: I want to really try to avoid modules treating these link templates as anything other than opaque strings. I suppose we couldn't stop a module from adding more links by using another link

So how do we accomplish the use case of modules adding functionality to entities as new tabs -- such as translation, for instance? Or things like... signup, media manager, devel, diff, etc etc

Damien Tournoud’s picture

@Crell, well, that's just a serialization format. If you are in a context that allow CURIEs (like HAL), you can serialize them this way. It doesn't change the mere fact that they need to be absolute URIs first and foremost.

Crell’s picture

Actually, now that I think about it, "drupal:something-or-other" *is* a legal URI. It's an Opaque URI, specifically, as opposed to a Hierarchical URI, but it's still completely legal as a URI per RFC 3986. There's no requirement that a URI be a URL.

Damien Tournoud’s picture

@Crell: Technically, you are right. But then you end-up with a custom scheme that doesn't have a backing standard, which basically bring back to square one: you are not compliant with an existing standard. It also breaks the best practice of providing documentation at the URI of the link relation type.

A.k.a.: it's a bad idea.

pounard’s picture

I might be wrong here, but it seems you are both discussing URI format within annotations. They wouldn't live anywere else than in Drupal and their goal is to be converted at runtime to any format we want, weither it's REST, or admin URL or "normal" HTTP URL, so I'm not sure the whole debate about custom breaking the standard is right here, the goal is to generate the standard URLs in the end. But I might misinterpret the debate here, so please carry on.

EDIT: Actually the comments in #3, #4 and #5 raise more important concerns IMHO, about the end business usage we're gonna do with those links definition.

linclark’s picture

Issue tags: -WSCCI

Regarding the discussion of CURIEs between Crell and DamZ, I would suggest that we use the full absolute URI for the moment. Ian Hickson has done a good job of identifying some problems with prefix indirection (e.g. CURIEs). In Drupal 7's RDF, they aren't implemented properly... if two prefix definitions collide, they are dropped so the CURIEs no longer expand.

However, whether we use CURIEs or not doesn't change the scheme that you are using. It is a namespace prefix, it isn't a scheme. The prefix expands to the full namespace URI that it is aliased to, and that contains the scheme. Similarly, the problem of documentation exists whether we use CURIEs or not... it's just dependent on us adding the documentation at the appropriate URIs.

However, I don't know that this discussion even has real bearing on the issue at hand, or whether it was a tangent. I'd suggest that we hold on further discussion of the CURIE point until it actually matters.

linclark’s picture

Re-tagging

linclark’s picture

Issue tags: +WSCCI

Crell mentioned an idea yesterday to have modules register their link relation aliases. For example, foo module could register foo-link-relation. In the annotation, this would be represented as "foo-link-relation" = "foo/{entity_subtype}".

We could easily add management of this kind of aliasing to the LinkManager. For example, we could have LinkManager::registerRelation($alias, $uri). This could be used in serialization modules like HAL to expand the link relation to its URI.

fago’s picture

Overall +1 on the proposal!

I might be wrong here, but it seems you are both discussing URI format within annotations. They wouldn't live anywere else than in Drupal and their goal is to be converted at runtime to any format we want, weither it's REST, or admin URL or "normal" HTTP URL, so I'm not sure the whole debate about custom breaking the standard is right here, the goal is to generate the standard URLs in the end. But I might misinterpret the debate here, so please carry on.

I guess I miss the point as well. I'd define them using regular Drupal paths like "node/{node}", while we would make them absolute before outputting them. Even if we output them as template, we would obviously have to take care of adapting the paths anyway?

I *think* (although I'm not certain), that this would allow for entity-specific paths that include other entities, such as /node/{node}/comment/{comment} or /myentity/{entity_subtype}/{myentity}, if any custom entities were so inclined. Because uri() is responsible for generating the actual links, and it can know its context and the meaning of different placeholders, that class would know how to get at the data it needs for that placeholder.

I'm not sure about this. I think the only context uri() should get $entity, but we could support expanding it by any somehow derived entity also. Either via a helper method or via the name of an entity-reference field.

@entity-type vs entity:
Yeah, I think the links need to be separated as well. How about "create" for adding an entity, where would that belong to?

One thing I'm missing as well is catering for multiple URI templates for entities as mentioned by sun in #3. We definitly want one default URI template, but the API should allow for optional further templates. Consider you are building custom UIs (e.g. with modules like page manager/panels) that provide alternative paths for viewing/editing an entity, maybe for a certain user role or in a certain (organic) group or other context. So while there can be a primary (default) URL template, you end up with having URL templates for an entity. Then, depending on the currently used URL template, links should be generated using the same URL template context while falling back to the default URL template.
That way it'd be possible to keep a custom context - like an og group active, e.g. when you view an entity in the OG group and use "edit", you'd get to the OG group specifc edit screen.

linclark’s picture

I guess I miss the point as well. I'd define them using regular Drupal paths like "node/{node}", while we would make them absolute before outputting them. Even if we output them as template, we would obviously have to take care of adapting the paths anyway?

I think the discussion was around the link relations, not the templates which would populate the values of the link relations.

In Crell's example, drupal:field-configuration would expand to something like http://drupal.org/rest/relations/field-configuration. It would not be a path on the site but instead would be a link relation that can be used to traverse a REST interface. Any consumer that knows Drupal's link relations would be able to write generic code to traverse these relations. It is a similar to the concept of a vocabulary (like Schema.org) in RDF.

Crell’s picture

Given the availability of path aliases and other path futzing code, I think having a simple canonical URI template for an entity type (rather than letting it vary on who knows what in bundles or time of day or whatever via a callback) is a much better idea. It's considerably simpler and easier, and anything involving panels et al should be handleable via aliases, no?

Crell’s picture

Status: Active » Needs review
FileSize
7.85 KB

OK, here's a patch.

What it does:

  1. Adds a link annotation to EntityNG entities, including sane default.
  2. Adds support for getting a link for *any* defined relationship.
  3. Documents how to use it. :-)
  4. Adds a link relationship for canonical and edit-form for nodes only.
  5. Updates node_view() so that it generates link header elements for *all* defined links. That means edit-form now shows up in the header.
  6. Adds a method to allow a caller to get a list of all relationships supported by the entity.
  7. Adds a __call() passthrough to EntityBCDecorator so that the new method is callable.

There should be no visual impact from this, because most of the visual links are built off of the routes, not $entity->uri(). However, you can see the change by creating a node and viewing the source.

Known issues/still to do:

  1. The URI templates use a leading /, per standard. However, url() adds a / so we end up with a // in the path. I didn't fix that yet because url() is about to change entirely in #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence, and I figure we can tweak it after that to trim leading / in paths. (This is likely to break a test somewhere.)
  2. Define @links for all other entity types in core.
  3. Maybe add more rels, but I'd rather keep it simple for now.

Benefits:

  1. We can now for any entity generate all of the links off of it that we reasonably could want, and use them wherever.
  2. We can (in follow-ups!!!) dynamically generate routes for entity types, using the provided link relationships as the path patterns.
  3. REST module can trivially generate its own routes for entities that use "whatever the entity says its path is", rather than breaking off and having its own paths, which is technically wrong per Hypermedia standards.
  4. REST module can also provide the full set of relationships on all entities, which means when you add a new relationship to an entity type the Hypermedia representation of it magically gets richer.
  5. By using IANA standard relationships, it makes it possible for any arbitrary hypermedia-aware client to understand what to do with the links.
  6. Individual entity types can add additional relationships as needed.
  7. This is a tool now available for Entity Operations et al, if they can leverage it. It doesn't yet do anything there, but is another tool available.
  8. Aside from the // problem (which we can address, I just didn't have time to do so here), this is entirely backward compatible with existing code.

Ease of use:
After I wrote the above, I went back and added a version-history relationship for nodes. It just magically worked. The entire second commit is one new line and adding a comma to the previous line. In the words of Ross Perot, "it's just that simple!"

I am going to be out of town and not Internet-connected for a week and a half starting this Saturday, so if someone wants to continue to run with this (please do so!), I have pushed the code to a branch in the WSCCI sandbox called 1970360-entity-uri. I recommend keeping the implementation simple and basic like this for now so that we can easily iterate on it later. That includes punting for now on any non-standard link types, curies, etc.

Hat tip to Tim Plunkett for walking me through how to actually do things with Entity API. :-)

Crell’s picture

FileSize
7.61 KB

And speaking of Tim Plunkett, he found a few quick gaffs that I've corrected here.

Status: Needs review » Needs work

The last submitted patch, 1970360-entity-uri.patch, failed testing.

fago’s picture

It's considerably simpler and easier, and anything involving panels et al should be handleable via aliases, no?

I'm talking about supporting having the same operation at two or more different paths at the same time. Consider using organic groups and providing group-specific UI for viewing, editing and deleting an entity. From group A view you should get link relations to edit and delete the entity in group A. In group B view you should get link relations to edit and delete the entity in group B.

Considering entity-operations etc. should build upon those URLs I think we'll need something like that.

Talking code-wize I think

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -352,8 +359,8 @@ public function label($langcode = NULL) {
-  public function uri() {
-    return $this->decorated->uri();
+  public function uri($rel = 'canonical') {
+    return $this->decorated->uri($rel);

should become something like

public function uri($rel = 'canonical', $pattern = 'default')

By default, you just have a single default URI pattern. But you could have a 'group A' and 'group B' URI pattern overriding certain links if you need. (Best terminology for "pattern", "url context" or whatever to be determined).

The patch looks like a good start to me. I think we need to check whether the current user has access to that paths and only output the links if though?

Crell’s picture

To answer the second point first, uri() doesn't do any access checking now. I don't think that's its responsibility, nor should it be. That's something that, I think, the generator and/or url() (or its descendents in the generator) take care of, no? Definitely it's not the business of the entity object to enforce, since it doesn't know about this weird thing called "users" (nor should it).

I'm extremely skeptical about this multi-axis relationship concept, still. The implications of that are far reaching and extremely ugly. If you were to offer a node at canonical = /group/{group}/post/{node}, for example, then you'd have to register a second route at that pattern, two. Now we have multiple routes defined for the same object, which breaks every conceivable rule about how to use URIs both according to IETF and Googlebot. There is no scenario in which that works out well for us. The entire point of a URI is that it is globally unique.

Path processors (formerly path aliases plus hook_url_inbound_alter/hook_url_outbound_alter) are the correct solution here. They could mutate the link ingoing and outgoing, while leaving all internal logic consistent at a single URI, as it should be.

Where possible entity operations should be defined explicitly as patterns, not via string concat. What we could consider, though, is something like drupal:operation = /node/{node}/op/{operation}, which would then support any unique operation machine name. We'd need to enumerate those somewhere, but it's a possibility. I really think that should be a follow-up, though. Incremental steps, please! :-)

fago’s picture

To answer the second point first, uri() doesn't do any access checking now. I don't think that's its responsibility, nor should it be.

Sure, it's not the responsibility of the uri generation functions. Still I think we should apply access checks to determine whether the link is available to a given user before we output the link.

Now we have multiple routes defined for the same object, which breaks every conceivable rule about
how to use URIs both according to IETF and Googlebot.

Multiple working URLs for the same object are not a problem, it's just the canonical URL that needs to be canonical. For the purpose of having URIs it would be reasonable to go with the 'default' pattern only - sure, but for generating the actively used URLs having multiple patterns is what we end up with in practice.

If you were to offer a node at canonical = /group/{group}/post/{node}, for example, then you'd have to register a second route at that pattern, two.

Exactly. Considering modules like page manager that's not a big deal at all.

Path processors (formerly path aliases plus hook_url_inbound_alter/hook_url_outbound_alter) are the correct solution here.

Well, that's not a good solution when you actually want to render the entity differently than usual - as it would be required when you want to render it in a special way per group context?

Crell’s picture

We're talking here about the intrinsic URIs and realationships of the object. If you're building something with page manager at a URI other than /node/{node}, then what you're showing is fundamentally NOT a node... it's a Page, which is (nod to UserAdvocate) a "unit of business intent". It is not a unit of content. The only place those two concepts overlap is the layout of the HTML-representation of the content resource, at its own native URI. That is semantically an edge case. It's a very common edge case, but an edge case. (Just like HTML is the most common response to an HTTP request, but it's conceptually in no way special.)

effulgentsia’s picture

Hm. Having read through this, it seems like there's 3 things still unresolved:

1) Is it always clear which one URI is canonical, and which are other relations? For example, in #1332058-8: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones, what's the canonical URI of a commerce order: user/{user}/order/{order} or admin/commerce/order/{order}? What's the relation of the other one? That comment also mentions checkout/{order}, but that seems like a straightforward payment relation to me.

2) How do we want to represent an alternate set of relations? For example, per issue summary, suppose we have:

links = {
  "canonical" = "/node/{node}",
  "edit-form" = "/node/{node}/edit",
  "version-history" = "/node/{node}/revisions"
}

But suppose og.module wants to add a set of additional routes: /group/{group}/node/{node}, /group/{group}/node/{node}/edit, and /group/{group}/node/{node}/revisions. Obviously, og.module could add these as routes directly without adding them to hook_entity_info(), but is there a compelling reason for it to also add them to hook_entity_info(), and if so, then if we don't do it with a separate axis per #22, then how do we do it? I interpret #25 as saying that alternate, contextualized UIs like this don't belong as part of what's registered in hook_entity_info(): @fago: can you clarify why you disagree with that?

 
3) The proposal in this issue leaves no room for a canonical URI pattern to vary by entity bundle. But per #3, if you have a taxonomy vocabulary configured as a forum vocabulary, then isn't /forum/{term} the logical canonical URI template for terms in that vocabulary? Or are we comfortable saying that it's not, and that we'll make up some relation that expresses the semantics of what a forum page is?

Those are the 3 outstanding questions that stood out to me, but if there's something else outstanding that I missed, please summarize/clarify it.

effulgentsia’s picture

the Media module provides an alternative UI to edit files at a URL different from file/%file/edit

Yeah, that's just plain wrong, and I think might be already fixed in Media 2.x, and if it isn't, it should be.

Dave Reid’s picture

Yeah, that's just plain wrong, and I think might be already fixed in Media 2.x, and if it isn't, it should be.

Irrelevant to the current topic since in D7 there's no way provided by core to easily output a form in a modal context. Luckily we have that with D8 now.

Crell’s picture

OK, let me try a different angle to explain why I'm against alternate "sets" of relationships.

A URI uniquely identifies a resource. Thus, conversely, a resource must have a single URI. In HTTP/REST, URI and resource should be understood as practically synonymous.

A data object (eg, entity) does not necessarily map 1:1 to a resource. In fact, such a naive mapping can cause problems if over-applied. That said, having data objects available at resources is not a bad thing, as long as it's understood that not all resources correspond to a data object. (That is, object->resource is sort of OK, but resource->object is not.)

That said... Having the same data object appear nearly identically as two different resources is problematic. For one, it's confusing for users. For another, it's confusing to Google. Google's dislike of multiple resources (pages) with nearly identical content is well-known and is a great way to kill your Google rankings.

The solution to that is the canonical relationship. Per RFC 6596: "The canonical link relation specifies the preferred IRI from resources with duplicative content." Also, "More formally, the canonical link relation specifies the preferred IRI from a set of resources that return the context IRI's content in duplicated form."

That is, a canonical link means "yes I know I'm almost the same thing as this other resource; that one over there is the for-reals one, and I'm the copy". The RFC specifically says to not have chained canonical relationships or to specify multiple canonical relationships per resource. (Specifying yourself as canonical is OK, though.)

So, having two or more canonical URIs for duplicative resources is explicitly against the RFC. That does not mean that exposing a given node at two different URIs (vis, as two different resources) is necessarily wrong... what it means is that if you do so, it is in fact wrong to not have one and only one of them designated as canonical.

Put in a more applied way...

If by default we expose a node at /node/$nid, and OG module wants to also duplicate that node at /group/$gid/post/$nid, we could debate whether or not that is wise (IMO, it's not), and how it should do it (e.g., an extra route defined by OG on its own) but there is no debate to be had that it is actively bad to do so and not designate one of those URIs as the single canonical URI. To do otherwise is to tell Google to blacklist you.

Additionally, all of that is only relevant to HTML pages. HTML pages are an edge case as far as HTTP/REST/Drupal 8's architecture is concerned. From an HTML page representation of a node, a bot should be able to determine what URI it should request with application/hal+json to get the HAL version of that node. That URI is opaque, and there is absolutely no reason to duplicate that whatsoever. HOw does the bot know where to go? It looks at the canonical link in the header and sends a request for application/hal+json to whatever that URI is. "Pretty URLs" have absolutely no meaning in that context.

In practice, the only use cases of canonical for us (not all resource need a canonical link) would be:
1) REST module, for which a single link is precisely what we want, always.
2) Resolving duplicate content pages to a single resource URI so that Google doesn't blacklist us.

In both cases, having a single canonical URI for that entity/resource is not a bug... it is in fact the feature, the whole feature, and nothing but the feature.

If commerce module wants to expose an order entity at 3 different URIs, I would first question why it's doing so and submit that it is a bad idea in the first place. Baring that, I would refer to the above argument and say that "the relationship between those links" is "two of them MUST refer to the 3rd as their canonical link, otherwise You're Doing It Wrong(tm)".

(Note that URL aliases are already 1:1, so aliasing /node/5 to /group/newbies/post/first-post does not change the above logic in any way; it only applies if we want the same node resource available at two post-aliasing URIs, in which case see the above argument.)

As for per-bundle links... Were it not for forum module I'd say "no, we don't want that kind of flexibility because it introduces more complexity than we can justify." Excessive flexibility breaks automation, and we need automation. That's the trend of Drupal 8 and the Entity API in general towards a more rigid, predictable structure at the cost of "do whatever" flexibility, because "do whatever" flexibility bites us in the arse. :-) Forum module is the edge case. Frankly, Forum module's use of taxonomy was always a hack, and given the new capabilities of Drupal 8 it should rightly be using a non-taxonomy hierarchical config entity of some sort instead of taxonomy anyway. We should not shape our new APIs around a 10 year old kludge that should be removed in the first place.

That said, currently in HEAD if you enable forum module you by default get a "General Discussion" forum, with ID 1. It shows up at /forum/1. However, because it's also a taxonomy term it ALSO shows up at /taxonomy/term/1, the same content just different formatting. That is almost exactly the use case for canonical, to note that yes these two resources (URIs) are duplicative, we know, resolve it by saying that one is the official, "canonical" one.

So rather than causing problems for forum module, I'd argue that this approach (one set of links only) fixes an SEO bug in forum module. :-) It would fix it by pointing to the /taxonomy/term/1 URI, but as I said, we should not be designing new APIs around decade-old kludges. We should be fixing decade-old kludges.

fago’s picture

I can agree with most of #29, still I think supporting multiple URL templates make sense.

We're talking here about the intrinsic URIs and realationships of the object.

If it would be only that, I'd totally agree with you. But I think the topic off this issue is a bit broader: We are talking about the practically used URL templates also.

In practice we have the need to expose the same data objects on multiple URLs. As you explained, that's not a problem as that is what the canonical URL link is there for. So why should we build the API without accounting for this if a) people have this needs and b) there is standard solution we can implement?

If commerce module wants to expose an order entity at 3 different URIs, I would first question why it's doing so and submit that it is a bad idea in the first place. Baring that, I would refer to the above argument and say that "the relationship between those links" is "two of them MUST refer to the 3rd as their canonical link, otherwise You're Doing It Wrong(tm)".

Exactly. So this is why I'd enable commerce to have an "admin" URL template besides the default template. When we know the relationship between the admin URL and the usual URL, it's easy to output the right canonical link.

I interpret #25 as saying that alternate, contextualized UIs like this don't belong as part of what's registered in hook_entity_info(): @fago: can you clarify why you disagree with that?

I don't care much where it's defined, but what I'm caring about is that the API supports multiple URL templates so that we don't end up having to build around that.
Let's say you build the node-edit form on a non default location at group "/group/{group}/node/{node}/edit". Then, e.g. any redirects issued from the form need a way to resolve the URL locations for the operations, let's take "delete". With just one default URL pattern, we'd end up redirecting to node/{node}/delete, where as we'd need /group/{group}/node/{node}/delete.

So, having two or more canonical URIs for duplicative resources is explicitly against the RFC. That does not mean that exposing a given node at two different URIs (vis, as two different resources) is necessarily wrong... what it means is that if you do so, it is in fact wrong to not have one and only one of them designated as canonical.

No one proposed that. The idea is to support multiple URL templates, whereas the default template can determine the outputted URI relationships.

E.g. we could have something like

"links" = {
  "default" = {
    "view" = "/node/{node}",
    "edit-form" = "/node/{node}/edit",
    "version-history" = "/node/{node}/revisions"
  },
  "group" = {
    "view" = "/group/{group}/node/{node}",
    "edit-form" = "/group/{group}/node/{node}/edit",
  }
}

Given that, we could easily support an optional $template parameter (default|group|..) in entity_uri() for generating non-default URLs and output canonical link relations on non-default URLs?

effulgentsia’s picture

In the example at the end of #30, if you're on /group/{group}/node/{node}/edit, what should the href of the <link rel="canonical"> tag be? /node/{node}/edit or /node/{node}? Just trying to understand how the two proposed axes would fit together while sticking to the standards explained in #29.

As for per-bundle links... Were it not for forum module I'd say "no, we don't want that kind of flexibility because it introduces more complexity than we can justify." Excessive flexibility breaks automation, and we need automation.

Does it really cause unreasonable complexity and interfere with automation? We already support the idea that creation access can vary by bundle and that data structure (which fields exist) can vary by bundle. In #1380720: Rename entity "bundle" to "subtype" in the UI text and help, we basically said that bundles are effectively subtypes. Why would allowing bundles/subtypes to override/add URI templates defined by the base type create problems?

Crell’s picture

Alex: Because we need to know the pattern for an entity type absent the presence of an entity object. Eg, REST module needs that information to say "oh, you're enabling REST support for product entities, so I'll make a route for you at $entity_info['links']['canonical'] that does that. kthxbye!"

It may be possible to break that up and spin up multiple paths, but then we run into the same multiple-paths questions I mentioned before. I am aiming for KISS right now, and the unknowns around per-subtype-path-patterns are a complexity that I do not want holding up this issue. (That's why I also punted on entity-type-level issues and curies for now. KISS.)

I checked last night and the test failures seem to be mostly or entirely related to the __call() in EntityBCDecorator and Twig. How long before we can kill that hack? I could use some help from someone in sorting out how to not break everything with that class. Ping me in IRC if you can help (sometime after 7 pm Central Time when I'm home. :-) )

Crell’s picture

Status: Needs work » Needs review
FileSize
10.55 KB

OK, that __call() was breaking too much, so I eliminated it. I also fixed the // problem temporarily, and added links to comments and terms.

I consider this feature complete for the purposes of this issue. We can iterate as needed with additional links afterward, but this gets us needed functionality for REST module.

Status: Needs review » Needs work

The last submitted patch, 1970360-entity-uri.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
10.39 KB

Some clean up

http://drupal.org/node/1354#drupal
"Sentences should be separated by single spaces."

http://drupal.org/node/1354#lists
lists at same level as the line which precedes the list
and indent multiline just 2 spaces.

http://drupal.org/node/1354#inheritdoc
* Implements \...
to
{@inheritdoc}

http://drupal.org/coding-standards/docs#link
@link needs @endlink
I dont know if "link text" is needed for these urls though.

I think a newline removal at the end of the class was an unintended change.

http://drupal.org/coding-standards/docs#todo
@todo gets indented when multiline.

I was going to just change ie, to i.e., but just went for the "that is" for better clarity.
#1818736: Correct 'e.g.' and 'i.e.' usage and punctuation in core

uri should be capitalized in comments
#1742958-11: All spellings of URI should be uppercase

will still be needs work after this for the fails...

YesCT’s picture

a good separate novice issue would be correcting @parm to @param in core/modules/comment/lib/Drupal/comment/Tests/CommentThreadingTest.php

YesCT’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
@@ -40,6 +40,10 @@
+ *   links = {
+ *     "canonical" = "/comment/{comment}",
+ *     "edit-form" = "/comment/{comment}/edit"

does this change the pattern from comment-NN to comment/NN ?

YesCT’s picture

before patch, Permalink on comment was like:
http://localhost/drupal/comment/1#comment-1
(to jump to the location of that comment on the page)

after patch it is just
http://localhost/drupal/comment/1

joachim’s picture

Could the summary be extended with details / examples of how module B will add to the links for module A's entity?

An example from core would be book module seeking to add paths such as node/NID/outline, or a translation tab on all entities.

Status: Needs review » Needs work

The last submitted patch, 1970360-entity-uri-35.patch, failed testing.

effulgentsia’s picture

fago’s picture

I am aiming for KISS right now, and the unknowns around per-subtype-path-patterns are a complexity that I do not want holding up this issue. (That's why I also punted on entity-type-level issues and curies for now. KISS.)

Yeah, I still think that we should support for multiple URI templates, but I'm ok with postponing this for now and moving on with what we agree. I do not want to hold up valuable progress on that debate.

So let's get to the patch details.. :

In the example at the end of #30, if you're on /group/{group}/node/{node}/edit, what should the href of the
tag be?

The canoncial URL of that would be "node/{node}/edit", so I think it would have to point to that. So that's why I find the following strange:

+ *   links = {
+ *     "canonical" = "/comment/{comment}",
+ *     "edit-form" = "/comment/{comment}/edit"

canonical is the canonical view-page URL, but what about canonical URLs of other pages related to an entity? An entity might have any number of resources, where each could have a canonical URL. I'd see it that way that all specified URLs are the canonical variants.

Imho "self" would be a good link relation for the entity URI, I mean the link relation from $comment to $comment really is about pointing to itself.

fago’s picture

Issue tags: +Needs tests

ok, here a review. Patch looks already quite good, but we should add some tests for this functionality also.

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
@@ -246,6 +246,40 @@ class EntityType extends Plugin {
+   * By default, the following placeholders are supported:
+   * - entityType: The machine name of the entity.

The machine name of the entity type.

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
@@ -246,6 +246,40 @@ class EntityType extends Plugin {
+   * - [entityType]: The entity type itself will also be a valid token for the
+   *   ID of the entity. This is generally preferred over "id" for better

I think this could need an example.

Crell’s picture

Status: Needs work » Needs review
FileSize
10.5 KB

Thanks, YesCT. Added that interdiff to my branch.

fago: Added improved comments from #43, too.

For the comment URI, there was a comment/{comment} path already defined somewhere, so I just ran with that. Comments are funky with their paths. That path is a pass-through for the node page the comment is on, actually, which is seriously weird but wasn't introduced in this patch. I'm actually not sure if it's legal to have a # in the URI for a resource in this way.

Rerolled patch attached. I'd love some help sorting out comments and the remaining fails, as I have DrupalCon presentations I still need to prepare... :-(

Status: Needs review » Needs work

The last submitted patch, 1970360-entity-uri.patch, failed testing.

linclark’s picture

Assigned: Unassigned » linclark

I fixed the CommentThreading test, I'm going to check whether the others still fail.

linclark’s picture

Assigned: linclark » Unassigned
Status: Needs work » Needs review
FileSize
3.26 KB
47.2 KB

Tests and rdf_comment_load assumed that the fragment should be on the comment permalink, but as YesCT points out the comment fragment is no longer included.

Crell, I have this in a branch, I could either push to the sandbox or you can use the interdiff, whichever you prefer.

linclark’s picture

FileSize
13.76 KB

I forgot to make sure that the branch from the WSCCI sandbox had the latest changes, so that will probably fail.

Status: Needs review » Needs work
Issue tags: -Needs tests, -entity cleanup, -WSCCI

The last submitted patch, 1970360-47-entity-uri.patch, failed testing.

linclark’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +entity cleanup, +WSCCI

#48: 1970360-47-entity-uri.patch queued for re-testing.

Crell’s picture

fago, effulgentsia, someone: What else do we need here? This is a blocker for unifying our URIs in REST module.

matt2000’s picture

Status: Needs review » Reviewed & tested by the community

The comment path redirect things is weird, but this patch seems to be a step toward fixing it.

The patch is readable, apply cleanly, and is also needed to move ahead on proper URIs for HAL on REST. Let's do it.

matt2000’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.76 KB
13.44 KB

I went ahead with further development based on this patch, and discovered that allowing {entityType} to stand in for {id} prevents us from passing these patterns directly into Route()s because of the magic object loading that happens for recognized entity types.

So here's a patch that skips adding that extra feature, which is not necessary for any of the other benefits of this issue.

Crell’s picture

That magic loading is likely to be removed: #1943846: Improve ParamConverterManager and developer experience for ParamConverters

Also, wouldn't we *want* that loading? Why wouldn't we?

There's also some odd jitter in the interdiff, as it +/-es several lines without changing them.

fago’s picture

I agree that {node} replacement is nice to have and once the magic in routes will probably go away anyway, I see no reason either to not have it.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.php
@@ -45,7 +45,12 @@
+ *     "canonical" = "/node/{node}",

As mentioned above, I'm unsure about the use of "canonical" here. Is it standardized that way? See #42.

+++ b/core/modules/node/node.module
@@ -2182,10 +2182,15 @@ function node_page_view(EntityInterface $node) {
+    // Set the node path as the canonical URL to prevent duplicate content.
+    drupal_add_html_head_link(array('rel' => $rel, 'href' => url($uri['path'], $uri['options'])), TRUE);

The comment does not really match any more, it now does more than that.

+++ b/core/modules/taxonomy/taxonomy.pages.inc
@@ -32,11 +32,14 @@ function taxonomy_term_page(Term $term) {
+  // Set the non-aliased canonical path as a default shortlink.
   drupal_add_html_head_link(array('rel' => 'shortlink', 'href' => url($uri['path'], array_merge($uri['options'], array('alias' => TRUE)))), TRUE);

Could that be moved to a link relation as well?

Crell’s picture

Discussed this with fago in meatspace. Yes, the canonical usage here is correct. shortlink... I suppose we could, but I'm happy to do that as a follow-up. That could be a little bit trickier because we need to not alias that, unlike all of the others.

fago’s picture

I had another careful look at the patch and I think it's mostly good to go, except one thing: It outputs all the link relations without checking whether the users has access to those pages/resources. Can we load those routes to check access on them?

We'd need that as well for entity operations, see #1839516: Introduce entity operation providers.

Crell’s picture

Ick. We can, but the way menu links do that now is gross. If we're going to do it here, we'd probably be wise to generalize it somehow, but that smells like scope creep. Follow-up, perhaps?

Crell’s picture

Actually, not having the machine name available is a big problem, because that machine name is what is used in routes, and therefore the variable names used in controllers. A controller parameter of $id that happens to have been upcast to a node object is totally not cool.

matt2000’s picture

In the REST module, we have a single EntityResource class for all entities. So we could take an $entity param instead of $id, but not $node, for example.

andypost’s picture

Crell’s picture

Assigned: Unassigned » klausi

Matt: Blargh. Can't we just adapt REST module, which we'll have to do anyway once this patch lands?

Assigning to Klausi for his input. We have to get this patch moving soon.

klausi’s picture

Assigned: klausi » Unassigned
Status: Needs review » Needs work

The second point in #55 still needs to be fixed.

REST module uses a $id parameter in its generic RequestHandler that handles all requests. The EntityResource plugin could provide customized routes that use the canonical URI template from the entity uri() information, but that is likely to contain the {node} placeholder, which will not match the $id parameter in RequestHandler. So we would have to create a copy of that controller class used in the EntityResource routes, which would then use a $node parameter instead. But now we have the problem that we need a separate controller for every entity type, because the variable will be named differently in the URI template, examples: {node}, {user}, {taxonomy_term} etc.

So we need an approach how we can use the routing system in a way that it passes arbitrary named parameters always to the $id parameter in the generic controller. I'm not sure how we could solve that, ideas welcome. And I'm unsure how the RequestHandler should then deal with already upcasted objects - or more precisely: we should rename $id there to something like $resource, as the RequestHandler does not care at all whether the variable has been upcasted already or not, it just passes it to the resource plugin anyway.

Crell’s picture

Magic upcasting is going away anyway: #1943846: Improve ParamConverterManager and developer experience for ParamConverters - So if we need a non-upcast value, we just don't tell it to upcast.

Could the REST controller just accept the Request and pull stuff out of it itself, and ignore the parameter name matching? It could also add additional defaults to indicate how it should behave. (Views is already doing this.)

klausi’s picture

Ah, that suggestion makes sense to just use the request object. I hope we can conveniently pull out parameters where we don't know the name ... anyway, that is a follow up issue for REST module.

So this patch just needs a small fix as mentioned in #55 and should then be ready.

Crell’s picture

Assigned: Unassigned » Crell

I'll fix that up tonight.

Crell’s picture

Status: Needs work » Reviewed & tested by the community

Actually, looking at #55 and #56, I don't think there's anything to do.

- canonical is still correct.
- The comment is accurate, I'd say, because we're adding canonical to HTML pages to avoid duplicate content issues. It could be used for other things, but that's what it's used for there. I don't know what else to put as a comment. :-)
- shortlink still needs special handling either way since it needs to be unaliased.

So unless fago disagrees, this is back to RTBC for the patch in #48. (NOT #53)

fago’s picture

Sounds good to me. Moving shortlink to a link relation needs a follow-up issue then.

alexpott’s picture

Issue tags: -Needs tests, -entity cleanup, -WSCCI

#53: 1970360-entity-uri-matt2000.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +entity cleanup, +WSCCI

The last submitted patch, 1970360-entity-uri-matt2000.patch, failed testing.

alexpott’s picture

+++ b/core/modules/node/node.moduleundefined
@@ -2160,10 +2160,15 @@ function node_page_view(EntityInterface $node) {
+  foreach ($node->uriRelationships() as $rel) {
+    $uri = $node->uri($rel);
+    // Set the node path as the canonical URL to prevent duplicate content.
+    drupal_add_html_head_link(array('rel' => $rel, 'href' => url($uri['path'], $uri['options'])), TRUE);
+  }
+
+  $uri = $node->uri('canonical');
+  // Set the non-aliased canonical path as a default shortlink.
   drupal_add_html_head_link(array('rel' => 'shortlink', 'href' => url($uri['path'], array_merge($uri['options'], array('alias' => TRUE)))), TRUE);

Couldn't this be written like this...

  foreach ($node->uriRelationships() as $rel) {
    $uri = $node->uri($rel);
    // Set the node path as the canonical URL to prevent duplicate content.
    drupal_add_html_head_link(array('rel' => $rel, 'href' => url($uri['path'], $uri['options'])), TRUE);
    
    if ($rel == 'canonical') {
      // Set the non-aliased canonical path as a default shortlink.
      drupal_add_html_head_link(array('rel' => 'shortlink', 'href' => url($uri['path'], array_merge($uri['options'], array('alias' => TRUE)))), TRUE);      
    }
  }

This saves a duplicate call to EntityBCDecorator::uri()

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -32,11 +32,14 @@ function taxonomy_term_page(Term $term) {
+  foreach ($term->uriRelationships() as $rel) {
+    $uri = $term->uri($rel);
+    // Set the node path as the canonical URL to prevent duplicate content.
+    drupal_add_html_head_link(array('rel' => $rel, 'href' => url($uri['path'], $uri['options'])), TRUE);
+  }
 
-  // Set the term path as the canonical URL to prevent duplicate content.
-  drupal_add_html_head_link(array('rel' => 'canonical', 'href' => url($uri['path'], $uri['options'])), TRUE);
-  // Set the non-aliased path as a default shortlink.
+  $uri = $term->uri('canonical');
+  // Set the non-aliased canonical path as a default shortlink.
   drupal_add_html_head_link(array('rel' => 'shortlink', 'href' => url($uri['path'], array_merge($uri['options'], array('alias' => TRUE)))), TRUE);

And this could be similarly improved...

alexpott’s picture

Also looking at the code excerpts above the comment in taxonomy_term_page is referring to nodes :)

Crell’s picture

Alex, you retested the wrong patch. :-) But I'll try to reroll anyway for #71/#72

Crell’s picture

Status: Needs work » Needs review
FileSize
10.9 KB

Rebased, tweaked, rerolled. Commit?

Status: Needs review » Needs work

The last submitted patch, 1970360-entity-uri.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
11.4 KB

User is now NG. Let's see if there's anything that doesn't fix.

Status: Needs review » Needs work

The last submitted patch, 1970360-entity-uri.patch, failed testing.

Crell’s picture

Assigned: Crell » linclark
Status: Needs work » Needs review
FileSize
4.81 KB
15.88 KB

Damn that moving HEAD...

This fixes the comment issues by introducing a permalink() method on Comment entities that is distinct from uri(); Comments lead a dual life, living half the time at /comment/{comment} and half the time at /node/{node}#comment-{cid}. I am not going to sort that out now, but since IETF links don't include fragments I just made permalink() its own method. It makes things pass.

I cannot replicate the password reset errors locally. Bugger.

The RDF fails are way beyond my comprehension. The best I was able to do was add some extra checks to avoid a fatal error, which simpletest handles in about as ungraceful a way as it can possibly manage at the moment. I'm going to have to ask Lin's help here because I don't even know what that test is supposed to be doing much less how to fix it.

Berdir’s picture

@Crell: Not sure what exactly you mean, but there's little that Simpletest could do in case of a fatal error. The fact that the batch API doesn't detect the aborted test run is fixed in #2000558: Regrression: Progress bar creates doesn't stop on errors..

Status: Needs review » Needs work
Issue tags: -Needs tests, -entity cleanup, -WSCCI

The last submitted patch, 1970360-entity-uri.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +entity cleanup, +WSCCI

#78: 1970360-entity-uri.patch queued for re-testing.

linclark’s picture

FileSize
2.99 KB
17.49 KB
+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.phpundefined
@@ -176,7 +176,8 @@ public function testCommentReplyOfRdfaMarkup() {
-    $comment_uri = url('comment/' . $comment->id(), array('fragment' => 'comment-' . $comment->id(), 'absolute' => TRUE));
+    $uri = $comment->uri();
+    $comment_uri = url($uri['path'], $uri['options']);
 

We lost the 'absolute' => TRUE in the options here. That was the reason for the majority of the fails.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.phpundefined
@@ -235,7 +236,12 @@ function _testBasicCommentRdfaMarkup($graph, $comment, $account = array()) {
+      if ($author_uri instanceof EasyRdf_Resource) {

This was added in the most recent patch to help with performance when failing, but the class needs a \ in front of it. As it is, the if fails even when the return is an instance of EasyRdf_Resource.

The other fails were because the changes I made in #48 disappeared from the patch somewhere along the way.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, Lin! Alex verified that it's kosher for me to RTBC this, so RTBC. :-)

dawehner’s picture

FileSize
659 bytes
17.49 KB

Sorry but this looks so horrible.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll :(

curl https://drupal.org/files/1970360-entity-uri_6.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 17906  100 17906    0     0  12309      0  0:00:01  0:00:01 --:--:-- 19253
error: patch failed: core/lib/Drupal/Core/Entity/EntityBCDecorator.php:214
error: core/lib/Drupal/Core/Entity/EntityBCDecorator.php: patch does not apply
dawehner’s picture

Status: Needs work » Needs review
FileSize
18.51 KB
  • Added urlRelationships to the interface and Entity.php
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, dawehner. The bot can object if it wants. I'm ready.

klausi’s picture

+++ b/core/modules/taxonomy/taxonomy.pages.inc
@@ -32,12 +32,16 @@ function taxonomy_term_page(Term $term) {
-  $uri = $term->uri();
-
-  // Set the term path as the canonical URL to prevent duplicate content.
-  drupal_add_html_head_link(array('rel' => 'canonical', 'href' => url($uri['path'], $uri['options'])), TRUE);
-  // Set the non-aliased path as a default shortlink.
-  drupal_add_html_head_link(array('rel' => 'shortlink', 'href' => url($uri['path'], array_merge($uri['options'], array('alias' => TRUE)))), TRUE);
+  foreach ($term->uriRelationships() as $rel) {
+    $uri = $term->uri($rel);
+    // Set the term path as the canonical URL to prevent duplicate content.
+    drupal_add_html_head_link(array('rel' => $rel, 'href' => url($uri['path'], $uri['options'])), TRUE);
+
+    if ($rel == 'canonical') {
+      // Set the non-aliased canonical path as a default shortlink.
+      drupal_add_html_head_link(array('rel' => 'shortlink', 'href' => url($uri['path'], array_merge($uri['options'], array('alias' => TRUE)))), TRUE);
+    }

This logic is broken, but it was already broken before the patch. The shortlink also prints the URL alias of the term, where it should use the non aliased version as the comment says. Created an issue for that: #2017087: Term shortlink link relation is broken

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests, -entity cleanup, -WSCCI

The last submitted patch, entity_uri-1970360-86.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

#86: entity_uri-1970360-86.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, entity_uri-1970360-86.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

#86: entity_uri-1970360-86.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +entity cleanup, +WSCCI

The last submitted patch, entity_uri-1970360-86.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
604 bytes
19.1 KB

Doh! Everytime you chance entityBC decorator you have to change ViewUI as well.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts

Quick, before Alex commits anything else! :-)

alexpott’s picture

Title: Entities should define URI templates and standard links » Change notice: Entities should define URI templates and standard links
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -Needs tests, -Avoid commit conflicts

Committed 342f132 and pushed to 8.x. Thanks!

alexpott’s picture

Issue summary: View changes

Complete a sentence.

dpi’s picture

Are we to assume that values of 'links' array in entity annotations are supposed to match patterns of routes? I see discussion of routes in the summary but I do not see any explicit documentation committed.

If so, I assume this is going to the main entry point into adding menu tasks/tabs to entities?

Crell’s picture

Alex: Yay!

dpi: For the moment, they're only used for the "canonical" link elements. They will be used for defining the routes for REST module on the fly. They COULD be used for other dynamic route definition, but are not yet.

There's lots of things we could do here in follow ups, as noted in the summary.

I've created a follow-up for REST module her: #2019123: Use the same canonical URI paths as for HTML routes

tstoeckler’s picture

Status: Active » Needs work

Unless I'm missing something:

+++ b/core/modules/comment/comment.module
@@ -1568,10 +1568,11 @@ function template_preprocess_comment(&$variables) {
+  $permalink_uri = $comment->permalink();
   $uri['options'] += array('attributes' => array('class' => 'permalink', 'rel' => 'bookmark'));

It seems this should be $permalink_uri['options'] += ...

+++ b/core/modules/comment/comment.module
@@ -1568,10 +1568,11 @@ function template_preprocess_comment(&$variables) {
   $variables['title'] = l($comment->subject->value, $uri['path'], $uri['options']);

@@ -1589,10 +1590,10 @@ function template_preprocess_comment(&$variables) {
+    $variables['parent_title'] = l($comment_parent->subject->value, $permalink_uri_parent['path'], $permalink_uri_parent['options']);

For consistency, I think that 'parent_title' should link to $comment_parent->uri() not $comment_parent->permalink().

Crell’s picture

Title: Change notice: Entities should define URI templates and standard links » Entities should define URI templates and standard links
Priority: Critical » Normal
Status: Needs work » Fixed

tstoeckler: IMO the permalink vs. "real link" (canonical) balance is all kinds of screwy. I don't really understand it either. I was just going for parity with the existing tests. If those tests should change so that we're doing something different that would be a new issue.

Change notice added: https://drupal.org/node/2020491

tstoeckler’s picture

Status: Fixed » Closed (fixed)

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

claudiu.cristea’s picture

claudiu.cristea’s picture

Issue summary: View changes

fix issue link

xjm’s picture

kattekrab’s picture

I've been working on the doc patch for this here
#2047283: Improve documentation for EntityType::$links
Just flagging here in case anyone who worked on the issue might want to chime in...

andypost’s picture

Issue tags: -WSCCI

Related bug #2010202-12: Deprecate comment_uri() comments no longer have permalinks with '#comment-123' fragment

andypost’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture