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.
Comment | File | Size | Author |
---|---|---|---|
#15 | 1057242--entity_uri-15.patch | 2.65 KB | drunken monkey |
#12 | 1057242--entity_uri-12.patch | 1.84 KB | drunken monkey |
#11 | 1057242--entity_uri-11-D8.patch | 1.84 KB | drunken monkey |
#11 | 1057242--entity_uri-11-D7.patch | 497 bytes | drunken monkey |
#3 | drupal.file-uri.patch | 489 bytes | drunken monkey |
Comments
Comment #1
BerdirWhat 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.
Comment #2
drunken monkeyI'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.Comment #3
drunken monkeyThis 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 usingentity_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.
Comment #4
Shadlington CreditAttribution: Shadlington commentedSubbing
Comment #5
drunken monkeySadly, 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).Comment #7
Dave ReidJust 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.
Comment #8
drunken monkeyWell, 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.
Comment #9
naught101 CreditAttribution: naught101 commentedWe'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.
Comment #10
BerdirSee #2, a custom uri callback is not possible
Comment #11
drunken monkeyHe suggests to also change the field name, which would make this possible:
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.
Comment #12
drunken monkeyThe test bot also ignores patches with "D8" suffix? Doesn't make sense to me, but well …
Comment #13
drunken monkeyTests pass, wonderful.
Comment #14
Dave ReidI 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.
Comment #15
drunken monkeyMight 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.)Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm fine with this approach.
Comment #17
Lars Toomre CreditAttribution: Lars Toomre commentedFor 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.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commented@Lars Toomre: the return value is documented, both in the docblock and inline:
I'm fine with the patch as it is.
Comment #19
Lars Toomre CreditAttribution: Lars Toomre commented@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.
Comment #20
Dave Reid$x = function_that_returns_nothing();
var_export($x) == 'NULL'.
There's no point in using 'return NULL' in a function that returns nothing.
Comment #21
drunken monkeyI'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.Comment #22
Lars Toomre CreditAttribution: Lars Toomre commentedFirst, 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.
Comment #23
naught101 CreditAttribution: naught101 commentedThere 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.
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commentedWell to be consistent with what core does elsewhere then, drop the lines:
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.
Comment #25
drunken monkeyMy (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.
Comment #26
Dries CreditAttribution: Dries commentedCommitted 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.
Comment #27
drunken monkeyOh, 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.
Comment #28
drunken monkey#15: 1057242--entity_uri-15.patch queued for re-testing.
Comment #29
Dave ReidThis is a good change for D7 and have tested with no regressions.
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm in favor of committing #15 to D7 too.
Comment #31
webchickCould 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).
Comment #32
drunken monkeyOf 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 (orNULL
). The current code thus breaks everything wanting the URI of a file entity in a rather ugly way.Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedNo, 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.Comment #34
webchickOk, 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."
Comment #35
webchickEr. Meant this.
Comment #36
Dave ReidComment #37
aspilicious CreditAttribution: aspilicious commentedhttp://drupal.org/node/1335516
So this is fixed again :)
Comment #39
grendzy CreditAttribution: grendzy commentedCan 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.Comment #40
Dave ReidI created http://drupal.org/node/1370062 specifically for the theme regression.
Comment #43
xjmMooooo
Comment #43.0
xjm+ summary