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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

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.

drunken monkey’s picture

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.

drunken monkey’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
FileSize
489 bytes

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.

Shadlington’s picture

Subbing

drunken monkey’s picture

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).

Dave Reid’s picture

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.

drunken monkey’s picture

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.

naught101’s picture

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.

Berdir’s picture

See #2, a custom uri callback is not possible

drunken monkey’s picture

Priority: Normal » Major
FileSize
497 bytes
1.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.

drunken monkey’s picture

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

drunken monkey’s picture

Issue tags: +Needs backport to D7

Tests pass, wonderful.

Dave Reid’s picture

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.

drunken monkey’s picture

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.)

Damien Tournoud’s picture

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.

Lars Toomre’s picture

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.

Damien Tournoud’s picture

@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.

Lars Toomre’s picture

@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.

Dave Reid’s picture

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

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

drunken monkey’s picture

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.

Lars Toomre’s picture

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.

naught101’s picture

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.

Lars Toomre’s picture

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.

drunken monkey’s picture

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.

Dries’s picture

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.

drunken monkey’s picture

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.

drunken monkey’s picture

Status: Patch (to be ported) » Needs review

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

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

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

Damien Tournoud’s picture

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

webchick’s picture

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).

drunken monkey’s picture

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.

Damien Tournoud’s picture

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.

webchick’s picture

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."

webchick’s picture

Status: Fixed » Active

Er. Meant this.

Dave Reid’s picture

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
aspilicious’s picture

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.

grendzy’s picture

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.

Dave Reid’s picture

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.

xjm’s picture

Issue tags: -Needs change record

Mooooo

xjm’s picture

Issue summary: View changes

+ summary