Problem/Motivation

Currently, the call to uri() will return NULL if there is no uri callback specified for the bundle or entity. This is the case for the entity_test in D8.

However, if we want to use the @id key in JSON-LD to reference entities, we need a URI.

Proposed resolution

Default to a standard callback if the uri callback isn't defined on the bundle or entity.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Add vocabularies to the list (see taxonomy_entity_info).

scor’s picture

+1, that makes sense to me. The default pattern could be entity/[entity-type]/[entity-id], for example entity/bla/1. Should we consider using UUID in these URIs and skip the entity type in the URI? (#1726734: Replace serial entity IDs with UUIDs in URLs, or even globally?).

I'm assuming the route for this default path would be registered once by the Entity property API maybe, so that each new entity implementation does not have to define it?

For entity types which define their own uri() like node, it means the path entity/node/1 would work and be a duplicate of node/1. Is that a problem? I guess it's fine since entity/node/1 should not be used or nor published anywhere as long as we always use the uri() method to retrieve the URI. It is implied that entity type with a custom uri() should also register their route for it.

rszrama’s picture

I'm not a huge fan of [entity-type] as an argument in the URL, but I suppose it's trivial enough for an entity type to define its own URI and never have to use this pattern. I suppose you add the entity type to ensure we don't have ID collisions between different entity types using the generic pattern, but then why not just make the default pattern [entity-type]/[entity-id]? You can't have entity type naming collisions, and even if such a path was already in use on the front end, the fact that an API request accept header specifies JSON-LD should be enough to differentiate it from any browser based request.

If that's insufficient, I'd think it'd be better as you suggest to have a single entity collection in core based on UUIDs. The entity resource could be filtered by entity type with a simple query parameter, and I presume you'd never have a collision at entity/[entity-uuid]. It just may be awkward for the default pattern to use UUIDs if every other entity URI pattern used entity IDs.

Anonymous’s picture

It is important to note this URI isn't specific to JSON-LD. It is the URI set in the entity system, which I believe is sometimes used to construct other paths which can be used for browser based requests.

That said, I doubt that we would have collisions if we use the [entity-type]/[entity-id] pattern, and would be fine with either that or entity/[entity-uuid].

moshe weitzman’s picture

Is it a problem to offer two equivalent routes, one with a UUID and one with a local ID? Seems reasonable to me.

scor’s picture

@moshe: could be a problem wrt SEO and duplicate content, so one of these should be chosen and set to be the canonical URI of the entity via a rel=canonical in a link element (related: #989032: Insert the canonical tag into all content, not just into nodes).

Anonymous’s picture

Status: Active » Needs review
FileSize
2.62 KB

Just posting a patch so we have something to work from. This adds a whole new test for the URI handling because I couldn't figure out an existing test where it would fit.

fago’s picture

Generally, this sounds like a good idea. But, if an entity has an URI, I'd assume I can link to it - what won't fly if there is no HTML representation of them available... :/

So, instead couldn't we just default to such a pattern for the json-ld representation only? Still, RDF probably has the same needs - so maybe we better add a helper function for that?

@uuid-vs-id: I agree we want do id by default, as we do elsewhere. But adding both - uuid and id paths - should be fine, the canonical url tag resolves the issue for HTML and for REST services it's fine also as long as you always use the *same* URI for referring to the entity.

Anonymous’s picture

There might be other (non-RDF) serialization modules which need the URI, and we shouldn't mint different URIs based on serialization. Because of this, I don't think that we want to define the helper function in RDF module. We could potentially add a helper function to Entity.

Is the uri() function used by code to infer routes that should return HTML? Or does it just imply to the human coder that they should be able to use it as an HTML link?

fago’s picture

Is the uri() function used by code to infer routes that should return HTML? Or does it just imply to the human coder that they should be able to use it as an HTML link?

It's mostly the human coder thinking that. But the same way it should be possible to only link to the entity if it has an URI, e.g. as the helpers from #1637342: Add entity_url() and entity_l() wrapper functions to simplify using EntityInterface::uri().

Anonymous’s picture

There's a proposal in #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones, which I callout in #10, which might address fago's concern.

Crell’s picture

I'm fine with a default path for all entities of /entity/{entity_type}/{entity-id}. That gives us a standard to work from, which can be overridden for a particular entity if appropriate (eg, nodes, terms, users, all do so already.) I'm not sure if we should use UUIDs there or serial IDs. UUIDs are useful, but serial IDs would be consistent. 'course, non-content entities probably will have machine names, not UUIDs or serial IDs, so...

andypost’s picture

yched’s picture

Just to be sure - are we talking about giving URIs to entities across all entity types ? Including config entities (i.e "a view", "a field", "a breakpoint"...) ?

Anonymous’s picture

If we want to expose those entities in JSON-LD, then it makes sense to give them URIs. If a request to that URI used "Accept: text/html", they would either get a 415 Unsupported Media Type, or they would get human readable documentation of the config.

I'm fine with not exposing config entities, but others have use cases for it, such as MASt module.

Grayside’s picture

Regards to Serial vs. UUIDs, URI should consistently use one, can use querystring for lookups by other elements.

moshe weitzman’s picture

The most recent patch looks fine to me. Do we need to actually implement uri callback anywhere or is this good to go?

moshe weitzman’s picture

Issue tags: +WSCCI

Adding tag

chx’s picture

One point for id: /entity/1234-adgf-gftrutyrnfgh is as ugly as it gets.

One point for uuid: <a href="/entity/1234-adgf-gftrutyrnfgh"> works across environments even if the id changes.

Both is moot when aliases are used but as this is about a default, I presumed they are not. Between the two, I think my preference goes to id, the second one should be a reference field.

chx’s picture

Also? entity/$entity_type/$id the lookup time is O(1) but entity/$uuid is a black horse -- you would need to go over every entity type and query 'em. If you go entity/$entity_type anyways then doing entity/$entity_type/$uuid seems awkward and pointless.

entity/$entity_type/$id it is.

andypost’s picture

+1 to entity/entity_type/id
having uuid as key seems ugly but could work

jibran’s picture

smiletrl’s picture

patch #7 looks good, but I'm thinking can we force entity to have a URI callback at the beginning? Maybe URI isn't used for that entity-self, but for JsonLD, it could be useful. I also saw this info at WSCCI sprint

•Every entity (even those that do not have an HTML URI) need to have a universally accessible canonical URI.

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUriTest.php
@@ -0,0 +1,46 @@
+class EntityUriTest extends WebTestBase {

I think that DrupalUnitTestBase is enough here

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUriTest.php
@@ -0,0 +1,46 @@
+    $user1 = $this->drupalCreateUser();

Is it needed at all? Anonym could be used

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
3.04 KB

Yeah, DrupalUnitBaseTest wasn't around when the first patch was posted. I've converted the test.

I also updated the URI structure to match the pattern everyone seems to agree on, entity/$entity_type/$id

tim.plunkett’s picture

Functionally, its great. Just some nitpicks on the coding standards.

This seems to change the expected return value of EntityInterface::uri(), implying that NULL should never be returned. If so, we should update that.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUriTest.phpundefined
@@ -0,0 +1,58 @@
+  /**
+   * Overrides \Drupal\simpletest\DrupalUnitTestBase::setup().
+   */

For some reason, the coding standard is to not give setUp a docblock.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUriTest.phpundefined
@@ -0,0 +1,58 @@
+  function setUp() {
+      parent::setUp();
+
+      $this->installSchema('system', 'variable');
+      $this->installSchema('system', 'url_alias');
+      $this->installSchema('field', 'field_config');
+      $this->installSchema('field', 'field_config_instance');
+
+      $this->enableModules(array('entity_test'));

The indentation is off here, and setUp should be protected

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUriTest.phpundefined
@@ -0,0 +1,58 @@
+   * Test that an entity without a URI callback uses the default URI.

Tests that...

sun’s picture

Anonymous’s picture

Thanks for the review and catching the interface change, Tim :) Fixed all.

chx’s picture

well, that's great, but what happens if you actually visit that URL? is this question out of scope?

Anonymous’s picture

If REST module is enabled, GET requests will be served a serialized entity in the data format that was requested if it is enabled. Otherwise, they get HTTP Error 415 Unsupported media type.

Crell’s picture

Silly question: In this case, why do we even need uri callbacks? Just let a specific entity type subclass and override the method. Done.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good.

@chx: if you visit that URL and rest.module is not enabled you get a 404, because there are no routes registered for the path. If you enable rest.module you will still get a 404. If you then enable the specific entity resource and visit it with your browser you will get a 415 for entity_test and some other 5xx error for entities that do not implement EntityNG yet.

@Crell: offtopic for this issue, whether we want uri callbacks or not is a separate issue that should be opened if you feel like it.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Dave Reid’s picture

Bumped #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones to a major task and Contributed project blocker as adding URIs to all entity types prevents contrib modules from knowing which entity types are 'public' and meant to be visible to end users, and which ones are not and shouldn't get things like meta tags, redirects, etc.

Status: Fixed » Closed (fixed)

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

dww’s picture

Status: Closed (fixed) » Needs work

A) This conflicts with and breaks the bug fix we're trying to do at #1991464: user_uri() should not return an invalid path (user/0) for the anonymous user object (which might be duplicate with #1275902: Allow entity URI callbacks to indicate that the entity has no URI, and make the User module use that for anonymous users, which is really a bug). user/0 is a 404. Why should we be forced to return broken links? Or, is the theory that user/0 should return something about the anonymous user now, just since we support a bunch of other serialization formats? WTF? ;)

B) Shouldn't there be a change notice for this if we end up keeping it?

fago’s picture

Title: Give all entities their own URI » Change notice: Give all entities their own URI
Status: Needs work » Active

I agree this needs a change notice.

Berdir’s picture

Title: Change notice: Give all entities their own URI » Give all entities their own URI
Issue summary: View changes
Status: Active » Closed (fixed)

I'm not sure if anything from here is actually left, it all changed 3 times anyway, so I don't think we need to keep this open.