I have two types of entities defined:
1) A textbook with a numeric ID (ISBN). Eg: 9780495011668
2) A school with a string ID. Eg: my_university
In panels, I am able to load textbooks by their IDs without trouble, but I can't successfully load schools. I can successfully load schools in views, but I believe that uses a different code path. I'm using the Entity API if that matters, but I don't believe it does.
I believe there is a mistaken assumption in ctools that entity IDs must be numeric. I find no evidence of that requirement in core. There is some discussion that results in the same conclusion here: #1027908: entity_load() should return entities keyed by ID, not name
The following change in plugins/arguments/entity_id.inc fixes this for me. Remove:
if (!is_numeric($arg)) {
return FALSE;
}
from:
/**
* Discover if this argument gives us the entity we crave.
*/
function ctools_argument_entity_id_context($arg = NULL, $conf = NULL, $empty = FALSE) {
$entity_type = explode(':', $conf['name']);
$entity_type = $entity_type[1];
// If unset it wants a generic, unfilled context.
if ($empty) {
return ctools_context_create_empty('entity:' . $entity_type);
}
// We can accept either an entity object or a pure id.
if (is_object($arg)) {
return ctools_context_create('entity:' . $entity_type, $arg);
}
if (!is_numeric($arg)) {
return FALSE;
}
$entity = entity_load($entity_type, array($arg));
if (!$entity) {
return FALSE;
}
return ctools_context_create('entity:' . $entity_type, $entity[$arg]);
}
I'm not sure, but I suspect something also needs to be changed in plugins/contexts/entity.inc (too many is_numeric calls for my liking).
Comment | File | Size | Author |
---|---|---|---|
#29 | entity_context-1229320-29.patch | 829 bytes | annya |
#4 | 1229320.patch | 2.09 KB | jdleonard |
Comments
Comment #1
merlinofchaos CreditAttribution: merlinofchaos commentedWould you be able, willing to create a patch?
It seems likely that this code derived from the node context originally and that did require numeric entity IDs. Since every entity in core uses numeric IDs, it is unsurprising that such a thing was never caught.
Comment #2
jdleonardYes. It's been a while, so let me go read how to do that :)
Could you take a quick look at plugins/contexts/entity.inc and let me know if you think anything needs to be changed there too? I'm fairly new to ctools. Some of that code is confusing me. Thanks!
Comment #3
merlinofchaos CreditAttribution: merlinofchaos commentedYes, it suffers from the same problem:
Since the checks prior to it are is_array and is_object already, anything else is probably legit as an entity id, so just changing that to an else is probably fine.
Comment #4
jdleonardAlright, how'd I do? :)
Comment #5
merlinofchaos CreditAttribution: merlinofchaos commentedUnless I misinterpret, that will fail -- you'll entity load, and then entity load again immediately. $data being an array is still an important gate there.
Comment #6
jdleonardI pretty sure it works as intended. The first if/else block sets the ID and the second block loads the entity. The patch removes the entity_load in the first if/else block. The relevant code with the patch is:
Are there any conditions under which the value of $data passed into ctools_context_create_entity() is an array without $data['id'] set?
Comment #7
merlinofchaos CreditAttribution: merlinofchaos commentedIf $data is an object, meaning an already loaded entity was passed in, you then perform a second entity_load. That is definitely not the right behavior.
Comment #8
jdleonardRight you are. Here's another attempt. I've also done a little renaming to make it easier to follow.
Comment #9
merlinofchaos CreditAttribution: merlinofchaos commentedNo need for the isset here; !empty() covers it. Also spacing problem:
if
should have a space.I don't see any validation for an enitity actually being loaded. It's pretty easy to get invalid entity IDs over time, due to deletions or just bad data, so we need to ensure that a failed entity load returns an empty context.
Comment #10
jdleonardThere is validation that the entity loads. entity_load() returns an array keyed on entity ID. We're passing a single ID in to entity_load so if the returned array is non-empty, it must contain the desired entity.
Removed unneeded isset(), fixed spacing, and moved return statement out one level to ensure failed entity load results in returning empty context.
Thanks for your help!
Comment #11
jdleonardBumping for review :)
Comment #12
merlinofchaos CreditAttribution: merlinofchaos commentedSorry! Been basically out of commission the last 2 weeks. Super busy. :/
Comment #13
merlinofchaos CreditAttribution: merlinofchaos commentedLook sokay!
Comment #14
Dave ReidSide note that entity IDs cannot and should not be non-numeric - this is an assumption coming from core that Entity API provides ways to violate. See #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID as to why this is not allowed.
Comment #15
Dave ReidThis needs to be reverted or re-add the numeric checking. To clarify: the core patch #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID once committed the entity_load() function will not work for entities with non-numeric IDs.
Comment #16
merlinofchaos CreditAttribution: merlinofchaos commentedBlargh.
Comment #17
merlinofchaos CreditAttribution: merlinofchaos commentedOk, reverted.
Comment #18
fagoThat's not true. Only fieldable entities require numeric ids, others don't. Also see the docs at hook_entity_info() and #1003788-67: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID.
It's about to fix the default implementation which requires numeric ids - that doesn't mean the whole API does so though. Thus, entity_load() is going to remain working with non-numeric ids.
Comment #19
Dave ReidThe assumption made that non-numeric IDs will work has already caused problems with other contrib modules - see #1343934: Meta Tags & WYSIWYG Incompatibility?. so sorry, I just don't think that supporting machine names as IDs for entities is a valid assumption with Drupal 7.
Comment #20
fagoDave, this is not just about machine names for entities, it's about the restriction in general. Consider integrating Drupal with a REST web service, such that the REST resources are entities. In that case the URL or at least the trailing path of it would have to act as identifier, but needs not be an integer at all. In order to be able to implement use-cases like this it's crucial that the API doesn't pose such restrictions which a remote source might not fulfill.
Comment #21
andrewbelcher CreditAttribution: andrewbelcher commentedWe encountered this problem where we were building a relationship via a machine name. I've attached the patch that fixed it for us. It's essentially the same as the previous patches, though I've only dealt with context creation, not arguments.
The main differences are that we do a check in entity info to make sure that there is a machine name for the entity type and that the following wont work with machine names as entity_load() keys by numeric ids:
As such, I'm using:
I think this is fine, as in this case we either have an array with a single item, or an empty array. Reset will return the item or FALSE respectively, so it has the same result. I wonder if it's worth changing to that across the board?
I wonder if it's worth tweaking the rest of the function so that it's agnostic whether the argument is an id or a name, rather than using
$id
. I've also not done anything to allow it to load via an array/object on machine names.If it looks like this approach is a good one, let me know and I can write a patch to deal with the rest of is as well.
As for the argument, do we want to have a separate
entity_name
argument handler, rather than makingentity_id
cope with machine names?Comment #22
andrewbelcher CreditAttribution: andrewbelcher commentedOops, forgot to attach the patch! Marked for review for testing purposes, though will want more work before being committed.
Comment #23
tim.plunkettThe reason this issue was reverted (#1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID) was never committed.
Profile2 supports non-numeric entity IDs. While added a context via the UI gives you an autocomplete that translates it into a numeric ID, the argument programmatically adds the context with no such allowances.
I haven't revisited the context code, but here's a fix for the argument.
Unlike the above patches, this doesn't need any specific checking, since it just uses reset().
Comment #24
jpstrikesback CreditAttribution: jpstrikesback commentedTested, get's rid of some notices, no side effects including nausea, dizziness, fainting, etc
Comment #24.0
jpstrikesback CreditAttribution: jpstrikesback commentedAdded file names and another place that may need updating
Comment #29
annya CreditAttribution: annya commentedThis patch fix non-numeric issue with arguments. Not sure if we need id-regexp functionality as ctools_argument_entity_id_settings_form has never called(and even if it will be called, it doesn't work).
Comment #30
betz CreditAttribution: betz as a volunteer commentedI am having the same issue.
We are using entities for our backend API, which has non-numerical ID's.
A bit confused about this issue, is there consensus? Will this be allowed?
We are patching Ctools now internally for this in our make file.
It would be nice to know if it's just about submitting a working patch or not to get this pushed upstream.
Tom
Comment #31
Chris Matthews CreditAttribution: Chris Matthews as a volunteer commentedThe 3 year old patch in #29 to entity_id.inc applied cleanly to the latest ctools 7.x-1.x-dev and (if applicable) still needs review.