Problem/Motivation

entity_uri() currently uses $entity->uri for caching purposes, which is neither namespaced nor otherwise protected. In core, this clashes with the file entity, which has its own uri property. This leads to the return value of entity_uri() being wrong for file entities in a rather critical way (string instead of array).

Proposed resolution

Remove the undocumented caching for now to maybe later replace it with something more thought-out. A patch for that is in #15.

Remaining tasks

Review and commit the patch.

User interface changes

None.

API changes

None. (Fixes code to comply to currently documented API.)

Original report by drunken monkey

I don't know why there isn't a bug report for this already, but basically calling entity_uri() for file entities simply doesn't work.

The entity_uri() docs state that the return value will be:

An array containing the 'path' and 'options' keys used to build the uri of the entity, and matching the signature of url(). NULL if the entity has no uri of its own.

However, internally the function uses a $object->uri field for caching (instead of something more distinct and sensible, like $object->entity_uri). Since the file entity already has a $uri field set, this is automatically used, even though it is a string and not an array suitable for an uri callback / entity_uri().

I don't really know how this can be fixed in D7 without either breaking existing behaviour (at least for the undocumented $object->uri field) or removing the entity_uri() caching (thereby possibly causing considerable performance drops), but this is a serious flaw when handling file entities generically.

Files: 
CommentFileSizeAuthor
#15 1057242--entity_uri-15.patch2.65 KBdrunken monkey
PASSED: [[SimpleTest]]: [MySQL] 35,982 pass(es).
[ View ]
#12 1057242--entity_uri-12.patch1.84 KBdrunken monkey
PASSED: [[SimpleTest]]: [MySQL] 32,930 pass(es).
[ View ]
#11 1057242--entity_uri-11-D8.patch1.84 KBdrunken monkey
#11 1057242--entity_uri-11-D7.patch497 bytesdrunken monkey
#3 drupal.file-uri.patch489 bytesdrunken monkey
PASSED: [[SimpleTest]]: [MySQL] 29,395 pass(es).
[ View ]

Comments

What about defining a custom uri callback in http://api.drupal.org/api/drupal/modules--system--system.module/function... ?

See http://api.worldempire.ch/api/privatemsg/privatemsg.module/function/priv... where that is used because the uri for private messages is a bit more complex.

I'm pretty sure that is impossible with the current caching code in entity_uri(), as said. The callback would never be called, since file entities always have an $uri field set.

Version:7.x-dev» 8.x-dev
Status:Active» Needs review
StatusFileSize
new489 bytes
PASSED: [[SimpleTest]]: [MySQL] 29,395 pass(es).
[ View ]

This is bugging me increasingly. Can we please get this in? Attached is a patch that solves this with a special case in entity_uri() – not perfect, but better than forcing everyone using entity_uri() with unknown entities to do the special-casing themselves. (It's in at least three of my projects already.)

I guess this is D8 now and has to be backported? Doesn't really matter, though, the patch applies perfectly in both versions.

Subbing

Sadly, after some consideration, I don't think this can be backported to D7 anymore. It would constitute an API change, even if changing from something being broken to it working like specified. At least, all my modules which are bugged by this would have to be changed to remove the workaround – and we can't know who else has already installed a workaround in one of their modules.

However, this should definitely be fixed in D8. In my opinion, this should either be done by using a better name for the field used for caching, or by moving the caching to a static array (keyed by entity type and ID – would mean that we need to call entity_extract_ids() each time, though).

Just encountered this WTF in #1190968: array_diff_key() warnings in redirect caused by invalid entity_uri() result. I think this is still eligible for backport. This function should work correctly for all core entities.

Well, before thinking about backporting, we have to first get this into D8, and therefore get anyone to care about it.
Please review the patch and test, whether it resolves the problem in D7.

We've hit this at relation too: #1260134: Rendering(?) of relations with files fails

Patch feels hackish. Changing the uri field name and implementing a callback would seem more sensible. Any thing to standardise rouge core entities seems like the best idea to me.

See #2, a custom uri callback is not possible

Priority:Normal» Major
StatusFileSize
new497 bytes
new1.84 KB

See #2, a custom uri callback is not possible

He suggests to also change the field name, which would make this possible:

Changing the uri field name and implementing a callback would seem more sensible.

However, at least in D7 this is out of the question, being a much too huge API change (unless I'm mistaken). It is hackish (btw, re-roll attached), of course, but at least it's not broken. Hackish is all we can hope for, I think.
And in D8, the correct fix would be not to use a non-namespaced three-character magic property (that even core itself breaks) as a cache. Something like the attached, I'd say.

Upping this to „major“, maybe then there'll finally be any reaction here. Also, I'm pretty sure that core breaking it's own API rather badly qualifies as major.

StatusFileSize
new1.84 KB
PASSED: [[SimpleTest]]: [MySQL] 32,930 pass(es).
[ View ]

The test bot also ignores patches with "D8" suffix? Doesn't make sense to me, but well …

Issue tags:+needs backport to D7

Tests pass, wonderful.

I actually think that we need to take the same position as entity_label() and other entity functions - and do not "statically" cache this information in the entity itself until we can come up with a standardized pattern for doing so.

StatusFileSize
new2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 35,982 pass(es).
[ View ]

Might be a good idea, too. entity_uri() probably is rarely called for the same entity more than once on a page (OK, I have a use case myself, but still …) and the callbacks usually aren't very complex, either.

The discussion about a standardized pattern (A $entity->_entity_information array with function names mapped to their return values?) would surely need to be done on a broader basis, but removing it for now if it just creates bugs is definitely also an option. Patch for that attached.

At this point, I'd be happy to get anything in that doesn't make every freaking code in every contrib module that wants to correctly call entity_uri() 6 lines longer. (Besides creating silly bugs in code by people who haven't noticed this bug yet.)

Title:$file->uri doesn't match the contract for uri callbacks, thus messing up entity_uri()entity_uri() should not use $entity->uri (was $file->uri doesn't match the contract for uri callbacks)
Status:Needs review» Reviewed & tested by the community

I'm fine with this approach.

For clarity in patch in #15, shouldn't there be a return value of NULL if $uri_callback does not exist? Having no return value leaves it unclear what value to check for when testing for a successful operation.

@Lars Toomre: the return value is documented, both in the docblock and inline:

* @return
*   An array containing the 'path' and 'options' keys used to build the uri of
*   the entity, and matching the signature of url(). NULL if the entity has no
*   uri of its own.

I'm fine with the patch as it is.

@Damien - I saw that it is documented. My point was why not explicitly return NULL rather than returning nothing. It makes the code clearly match the documentation. I personally hate when some code paths return a value and others do not from the same function.

Why depend on no return means NULL from a DX perspective? This is a small point, but I thought it would help the clarity of the revised function.

$x = function_that_returns_nothing();
var_export($x) == 'NULL'.

There's no point in using 'return NULL' in a function that returns nothing.

I'm also pretty sure this (relying on PHP returning NULL in the absence of an explicit return statement) is actually done elsewhere in core, too.

First, as I said in #19, this is a small point.

However, when there are multiple code paths in a function that in some cases returns a value, I believe that it is poor coding style to leave other code paths with no explicit return value(s). I was suggesting that bwe complete the missing code path like 'return NULL' in the else statement earlier in function.

@DaveReid - Clearly, if there is only one exit from a function, there is no need to explicitly return NULL if no return value is expected.

There are a few cases where NULL is explicitly returned in core, but not many. There are many where it's returned implicitly. Suggest posting a separate issue for making that standard (or not), and keeping this one free of sidetracks.

Well to be consistent with what core does elsewhere then, drop the lines:

  else {
    return NULL;
  }

and depend on the default NULL value return except if uri_callback executed. My point is let's be consistent: either return NULL for each needed path or do not return it at all.

My (Our?) point is that this is a major bug in one of core's API function, and everyone else seems to be OK with the code. You've made your point, now please don't go on and on about it. If webchick/Dries is of your opinion, it will have to be changed anyways. Otherwise, there is absolutely no use in making ten more comments about it.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. I'll leave this to webchick to backport to 7.x; could use another pair of eyes before adding it to a stable branch.

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)
Issue tags:-needs backport to D7

Oh, great!

OK, so do we also just remove the caching here for D7? In any cases, the patches should all apply to both versions equally.

Status:Patch (to be ported)» Needs review

#15: 1057242--entity_uri-15.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

This is a good change for D7 and have tested with no regressions.

I'm in favor of committing #15 to D7 too.

Could someone help out by posting an issue summary of what this changes? At a glance it seems to turn a string into an array, and I'm not sure why that woudln't break things (though it doesn't seem to affect any tests).

Could someone help out by posting an issue summary of what this changes? At a glance it seems to turn a string into an array, and I'm not sure why that woudln't break things (though it doesn't seem to affect any tests).

Of course.
It does turn the return value from a string to an array, yes – the return value of entity_uri(), which is documented to always return an array (or NULL). The current code thus breaks everything wanting the URI of a file entity in a rather ugly way.

No, this patch doesn't change any API. It just removes the caching of the URI in $entity->uri, which breaks as soon as an entity defines a column named "uri", as core does with the File entity.

Title:entity_uri() should not use $entity->uri (was $file->uri doesn't match the contract for uri callbacks)Change notice for entity_uri() should not use $entity->uri (was $file->uri doesn't match the contract for uri callbacks)
Category:bug» task
Priority:Major» Critical
Status:Reviewed & tested by the community» Fixed

Ok, I see. The docs do indeed indicate that, yes.

Committed and pushed to 7.x. Thanks!

This still feels like it could use a change notice though, "just in case."

Status:Fixed» Active

Er. Meant this.

Title:Change notice for entity_uri() should not use $entity->uri (was $file->uri doesn't match the contract for uri callbacks)entity_uri() should not use $entity->uri (was $file->uri doesn't match the contract for uri callbacks)
Issue tags:+Needs change record

Status:Active» Fixed

http://drupal.org/node/1335516

So this is fixed again :)

Status:Fixed» Closed (fixed)

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

Priority:Critical» Normal
Status:Closed (fixed)» Active

Can the change notice be updated? D7.9 breaks themes that use $uri in node.tpl.php. Dave Reid says it's because template_preprocess_node copies each property of node into $variables, so $uri isn't defined in templates anymore.

Status:Active» Fixed
Issue tags:-Needs change record

I created http://drupal.org/node/1370062 specifically for the theme regression.

Status:Fixed» Closed (fixed)
Issue tags:+Needs change record

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

Issue tags:-Needs change record

Mooooo

Issue summary:View changes

+ summary