The entities returned by entity_load() using the Entity API are currently keyed by their machine names, even though the general contract for entity_load() is to return them keyed by ID.
This was the only thing to do back when default entities just didn't have any IDs, but in my opinion should be revised now that that has changed.

(I ran into this problem when trying to create a search index on search indexes and while I admit that this is hardly a real-world use case, it illustrates the problem that one has when dealing with entities on a general level without this issue being fixed.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

fago’s picture

ad #1: The entity API introduces a name key for entities that are exportable, which therefor make use of the name key as uniform identifier while they still have the numeric id for internal usage (-> db, field API).

>The entities returned by entity_load() using the Entity API are currently keyed by their machine names, even though the general contract for entity_load() is to return them keyed by ID.
Yes, the name key is supposed to be the entity's main identifier / ID.

Thus, all the API functions like entity_load() or entity_delete() expect the entities identifier, the name key, while at least entity load supports being called with the numeric ids too. Still, it is going to return an array indexed by name key + passes such an array to the entity load hook.
I do think this makes much sense as the name keys are that what developers usually want and should use.

drunken monkey’s picture

I do think this makes much sense as the name keys are that what developers usually want and should use.

Well, they are if I know that the entities in question use the Entity API (and are exportable). However, when I generically handle entities (which the Entity API is generally designed to support), I expect a numeric ID, as it is documented for entity_load().
Maybe it makes sense in most cases, but it still goes against the contract for entity_load(), and in my opinion such breaches should only be made when necessary. Someone not knowing of your module would have no idea that entity_load() might in some cases return the entities differently.

fago’s picture

Someone not knowing exportable entities is not likely to load it ;) Also entity_load() does not specify that the $id has to be numeric in any way, so I don't see any contract violation here. Anyway, I agree with you that introducing another, machine-readable identifier extends the core entity API a bit, but this really is a necessity for doing exportables.
So I still think it's a good idea to do it that way, also changing this at this point what mean a rather drastic API change for all modules making use of exportable entities.

bojanz’s picture

There needs to be a warning then, telling people that declaring their entity type as exportable (or just providing the "name" key?) breaks entity_load() and everything that uses it (starting from Views).
I get the reasoning, but breaking a core function is breaking a core function...

Damien Tournoud’s picture

Category: feature » bug
Priority: Normal » Critical

This is pretty critical. Even if I understand the design decisions behind it (mainly: simplifying the code that uses entities with a name), there is no excuse to break the API of such a central function as entity_load().

For example, this completely break Views integration for those entities, because the fieldapi field handler expects entity_load() to work properly.

I suggest implementing a separate API for those entities (entity_load_named()?), and making sure that every place in the entity API accepts both an entity ID and an entity name (I see that the entity wrapper thingie expects a "token" in place of a numeric ID when an entity has a name, which breaks things in funny ways).

fago’s picture

Once a name key is specified, the entity API uses this as the entity's id(entifier) everywhere - thus also for entity_load(). Strictly, I don't think this is an API violation as entity_load() nor something else says that the entity identifier has to be numeric. Just the field API requires the existence of a numeric id (-> 'id' key).

However, I agree with you that this is really bending the core entity API/concepts and is not ideal at all. Still, I don't see another way: Changing that now would mean a really drastic API change + it is not really possible for exportable entities as it would basically require the usage of an entity_load wrapper / establishing another alternate loading infrastructure making use of names in the hooks too.

Consequently, I think the only practicable fix to the rare issues caused by this is to just not assume entity_load() returns entities keyed by the numeric entity key 'id'.

Dave Reid’s picture

I have lots of D7 contrib modules in development that work on a generic entity bases and store ID keys in integer fields. So it's not just field API that could possibly be affected.

Crell’s picture

fago: Views is not a "rare issue". This sounds like yet another problem caused by trying to make entities into exportable config objects when they really really should not be.

fago’s picture

>I have lots of D7 contrib modules in development that work on a generic entity bases and store ID keys in integer fields. So it's not just field API that could possibly be affected.

All exportable entities will have the numeric ID too, so the code works as long as it doesn't assume the return form entity_load() is keyed by the numeric id.
Still as to the docs, the existence of the numeric 'id' key is only required for fieldable entities. The core entity API is just not a 100% complete system and very vague on what's required when.

xatoo’s picture

I've come across this issue several times, especially when trying to index entities which have a named key defined with the search_api. Could we please reach some form of consensus on this issue? Whether or not it is naive to assume numeric keys or unconventional to return them that way; if both parties are refusing to move their opinion, things are not going to work out and people will be forced to hack in code to translate arrays keyed by name to a variant which is keyed by a numeric id.

drunken monkey’s picture

As mentioned above, I'm heavily in favour of removing this Entity API pecularity and always returning entities keyed by numeric ID.

My reasoning (apart from "Don't needlessly break core APIs or conventions!") is this: When someone wants the machine name for certain entities they load, in nearly all cases they'll know the type of entity they are loading. I can think of only very few cases (or only one, namely for dealing with features) where you would want to generically handle only exportable entities. And in all other cases, I can just use the field directly.
When dealing with entities generically, on the other hand, you usually (I'd say) will want to deal with all entities, not just exportable ones. Therefore you want the numerical ID (at least when writing to the DB or such – it might not matter in other cases, of course) – and since manually getting the name of the ID field for each entity involved is much more complicated, almost all developers will use the array keys for that, unaware that some contrib module doesn't return the numeric IDs. Even though it is not directly specified in the entity_load() documentation, this is at least a convention in core, and few developers will have reason to doubt that the documentation always means numeric IDs (even if this isn't the case, strictly speaking).

I have just checked in the Search API: in ten modules using exportable entities (search indexes, servers, facets and ranges) there is not a single line that depends on the fact that entity_load() will return the entities keyed by machine name for those types. Even if this isn't representative, I strongly doubt that this is used very often.
On the other hand, my module will currently break if you want to create an index for any exportable entity, and changing this would probably be quite some work. And this is even though I know the Entity API quite well – others might not have even heard about it yet, and still use entity_load().

fago’s picture

When someone wants the machine name for certain entities they load, in nearly all cases they'll know the type of entity they are loading. I can think of only very few cases (or only one, namely for dealing with features) where you would want to generically handle only exportable entities. And in all other cases, I can just use the field directly.

Just knowing the entity type does not suffice. Use cases are simple: load two or more entities by name, access them after loading. It's the same for hook implementations: Alter a specific entity by name. Without the array being keyed by name, have fun searching for your entity.

Beside that: Changing that right now, would mean
* a rather drastic API change (changed (+unusable) loading, view hooks for all exportable entities)
* not mentioning an API change that would need have to be enrolled to all modules implementing them (dependency nightmare)
* break all code that tries to entity_load() by name, e.g. entity wrappers
* break exports of modules referencing to exportables as they suddenly would have to care theirselves about using names as identifier. This is just silly, why would one make the identifier of an exportable a numeric id?

While the fix for the occurred issues is easy: You really need to get the numeric id? Run entity_extract_ids(). Done.

Thus, given all that headache we would ran into with changing that now, I don't see a realistic option to do that.

ad #11:
Regardless of what the entity API module does:

Does the (core) entity API specify all entity identifiers are numeric? No.
So can one rely on numeric entity identifiers? No.

bojanz’s picture

If it's a major API change, then it's a major API change that needs to happen.
Better to break the 10 (or 20 or 30) modules that use exportable entities than break all of contrib land.

Does the (core) entity API specify all entity identifiers are numeric: No.
So can one rely on numeric entity identifiers? No.

You are looking for loopholes. The fact that you found one doesn't change anything. One fuzzy comment can't be stronger than every other entity(-related) implementation out there (which assumes numeric ids).

Please fago, reconsider.

fago’s picture

>Better to break the 10 (or 20 or 30) modules that use exportable entities than break all of contrib land.
Better break the 2,3 modules that assume something wrong, than breaking half of contrib.

Entities != fieldable entities. I'm sry, yes most people have not got that yet, but that's how it is.

drunken monkey’s picture

Just knowing the entity type does not suffice. Use cases are simple: load two or more entities by name, access them after loading. It's the same for hook implementations: Alter a specific entity by name. Without the array being keyed by name, have fun searching for your entity.

As said, just provide a different function for loading entities keyed by name. Modules can just use that if they really want them keyed by name.
The hook example makes sense, that's true. However, you could just provide a simple function (I'd even volunteer to code it, if you want) that converts an array indexed by ID to one keyed by name. Would lengthen such hooks by exactly one line, even if there'd be a slight overhead.
Both these examples, however, work equally well reversed: What if you only know the entity's ID? Have fun searching for it. (When you don't know the entity type beforehand, of course – otherwise it's clear you'd say that you shouldn't reference exportable entities by numeric ID.) There is then no way to get them keyed by ID.

As for it being nowhere specified: hook_entity_info() specifies that the "entity keys[id]" key contains the entity's ID. entity_load() specifies that the returned array is indexed by ID. And even if none of those specify directly that all those IDs have to be numeric: even for exportable entities the field in "entity keys[id]" is the numeric ID. Consequently, this has to be the one the return array of entity_load() is indexed by, following the function's contract, no matter where you try to find loopholes. Your "name" key is a contrib extension, and therefore isn't what core functions mean with "entity id".

Also, this means that exportable entities can't be fieldable? Also quite a restriction, although maybe it's necessary in any case.

Better break the 2,3 modules that assume something wrong, than breaking half of contrib.

Come on, how could a change in your module possibly break "half of contrib"? An inconsistency (and, as I stated above, even breach of contract) in a (quite central) core function seems much more dangerous to me.

fago’s picture

>Both these examples, however, work equally well reversed.
They don't. No one cares about an numeric id of an exportable.

>As for it being nowhere specified....
Entity info specifies that fieldable entities need that id key (which is specified to be numeric), nothing else. No one says how one has to identify entities in general.

So despite I think using names is valid and assuming all entity-identifiers have to be numeric is wrong - point is, I'm not going invest hours and hours, if not weeks of work in doing all that changes and break lots of existing code just because 2,3 issues came up that can be fixed by adding a single function call.

xatoo’s picture

> They don't. No one cares about an numeric id of an exportable.
I can't fully agree with that. If someone wants to do some generic thing with entities they don't care whether it is exportable or not. Chances are, they don't even care whether they get a numeric id or a machine name as long as it is consistent for all entities. (And not assuming anything is in my view a form of consistency. ;) )

At the moment, although entity_load() doesn't explicitly specifies its result to be keyed by numeric id, I can image that the vast majority of people will asume it to be. Especially since it's true in most cases. So, for one, the documentation needs to be clarified as soon as there is consensus on what to return.

> Without the array being keyed by name, have fun searching for your entity.
> What if you only know the entity's ID? Have fun searching for it.
Although it is clear that it is easier for a lot modules to be able to asume one thing or another I don't think they should. Enforcing entities to be keyed by numeric or named id kind of undermines the idea of entities. Entities pulled out e.g. some web service might even not have a numeric ID. Therefore, imho, I believe these two statements to be equally wrong. You shouldn't care whether the key is a name or a number. It's an identifier and you can load entities with it.

sun’s picture

Not too familiar with the discussion here, but I like the idea of a separate API function for non-numeric things.

However, truth is also that if a Twitter module in D7 would use entities for tweets, then those tweet entities could not use numeric integer IDs.

Also note that, if third-party modules storing attached data for particular entities would simply reference the entity ID as string, then it is not possible to perform a SQL JOIN if the original entity ID is stored as integer. MySQL apparently allows that, but PostGreSQL does not. Of course, it's use-case specific whether you'll ever want to join on your data, but in terms of this discussion, I guess it's highly relevant.

Dave Reid’s picture

There's no reason a twitter entity couldn't use an auto-increment ID as well as a string tweet ID, just like we would do with UUIDs - we'd use them in combination with serial IDs.

fago’s picture

Entities pulled out e.g. some web service might even not have a numeric ID. Therefore, imho, I believe these two statements to be equally wrong. You shouldn't care whether the key is a name or a number. It's an identifier and you can load entities with it.

Exactly, that's how the (core) entity API works. The entity module adds the name option, but this is no enforcement either. You don't have a numeric ID for everything and enforcing that would be wrong.
For d8 we should add a way to get a unique identifier for every entity though, regardless of what data type the identifier will be.

Also note that, if third-party modules storing attached data for particular entities would simply reference the entity ID as string, then it is not possible to perform a SQL JOIN if the original entity ID is stored as integer. MySQL apparently allows that, but PostGreSQL does not. Of course, it's use-case specific whether you'll ever want to join on your data, but in terms of this discussion, I guess it's highly relevant.

hm, that's indeed a problem. I think adding a numeric id makes sense as soon as the entity goes to the DB level, what enables efficient joins. Still, for a join of a 3rd party module it might be preferable to join by name...

fago’s picture

ok, I've re-read all the stuff that's documented and tried to look at it from a broader view. And I agree that the current behaviour is not optimal as all that different ids are really confusing. So let's improve that while trying to keep the API changes to the unavoidable minimum.

a) I remembered that entity_load() for exportables can already handle both, id + name keys. By still supporting loading by name key too, we can save us lots of breakage (and work).

b) As we noted, the docs are clear that as soon as an entity is fieldable, it must have a numeric primary id key. As it is documented as primary id, let's use it for entity_load and in particular for keying the entity load results.
As said, I think not having that numeric id is perfectly valid for non-fieldable entities, but changing the id behaviour once an entity is made fieldable is just weird. So let's always use the numeric id for keying the entity-load results. That said, people still shouldn't rely on numeric ids for non-fieldable entities if no 'id' key is specified.

c) We need an easy way to transpose the entity array to be keyed by name, perhaps something like

 $entities = entity_key_array_by_property($entities, 'the_name_key');

>Not too familiar with the discussion here, but I like the idea of a separate API function for non-numeric things.
What would you have in mind?

d) Loading hooks
hook_entity_load() should for sure be always keyed like the entity_load() result, thus then by numeric id. However we might want to choose to key entity-type specific hooks (e.g. hook_profile2_type_load(), ..) still by name key as this makes more sense for those entities + safes us even more headache.
Thoughts on that?

I'd very much appreciate input and help on this, in particular of all the people that complained so loudly.

xatoo’s picture

That said, people still shouldn't rely on numeric ids for non-fieldable entities if no 'id' key is specified.

That is nice in theory but in practice this principle is violated like a drunken prom date. For now it might be just Fields, however you can expect other modules to be added to contrib which rely on entities having a numeric ID. Just look at the relations module.
Sun makes a valid point why not to use text fields to refer to entities, so that's no option. Forcing entities to have a numeric ID is not an option either since people might not be in charge of the entity datastructure.

hook_entity_load() should for sure be always keyed like the entity_load() result, thus then by numeric id. However we might want to choose to key entity-type specific hooks (e.g. hook_profile2_type_load(), ..) still by name key as this makes more sense for those entities + safes us even more headache.

Indeed. People using hook_profile2_type_load() know what they are loading and thus what to expect. People using hook_entity_load might not. But how to act if no numeric ID is specified? Should modules just ignore entities which have no numeric ID when they need a numeric ID to work with?

I'm relatively new to this matter and might thus not have the necessary insight to evaluate ideas, but why not the following:
If we keep a table with entity_type, entity_identifier, and a serial. We could give any loaded entity which has no numeric ID a numeric ID on the fly, the first time it is loaded. That way modules can do all things with named-only entities they can do with numeric identifiable ones, even add fields and relations to them. Loading an entity by its numeric ID? Entity_load knows the entity has no numeric ID of its own so it checks the reference table to see if it can get the name and then loads the entity by its name.

drunken monkey’s picture

a) I remembered that entity_load() for exportables can already handle both, id + name keys. By still supporting loading by name key too, we can save us lots of breakage (and work).

Yes, this was of course already a great step. In my opinion it would even be alright if entities requested by name would be returned keyed by name – if you are using names for loading, it implies you already know that these are entities with machine names. I don't know whether that would make things simpler, though. (It would certainly make the documentation more complicated, though, specifying when which key is used.) It would also be arguable from the documentation, as what you pass to entity_load() as $ids are, of course, also entity IDs.

c) We need an easy way to transpose the entity array to be keyed by name, perhaps something like […]

Maybe also something like entity_load_named() that returns the loaded entities keyed by name (i.e., the current behaviour). I haven't looked at the code yet, but if the difference is practically a single line, we could even abstract this in a entity_load_keyed_by_property() function that lets the user specify the (hopefully unique) property to use for indexing the return array, and usse that in the other two functions.

(Function names are of course provisional.)

hook_entity_load() should for sure be always keyed like the entity_load() result, thus then by numeric id. However we might want to choose to key entity-type specific hooks (e.g. hook_profile2_type_load(), ..) still by name key as this makes more sense for those entities + safes us even more headache.
Thoughts on that?

In principle, I agree, that would make sense and should help to reduce API breakage to a minimum. (Just has to be documented properly.) I don't know, however, how well this would work in practice, passing different arrays to the two hooks.(On the other hand, those aren't alter hooks, so there shouldn't be too much of a problem, just using the entity_key_array_by_property() function.)

I'd very much appreciate input and help on this, in particular of all the people that complained so loudly.

Of course. ^^"" I'll see what I can do this weekend.

drunken monkey’s picture

Status: Active » Needs review
FileSize
14.88 KB
1.46 KB

OK, I have a first version of this, as discussed in the last comments by fago and me. Entities are returned keyed by id in all cases, even when loading them by names.

Of course, I have only limited insight into the module so I might well have overlooked some areas, but at least the tests all run fine. Don't know about documentation, though, I've also included one @todo in the patch where I don't completely understand what to do in the context.

The first patch only contains some comment typos or errors and can/should be committed independently of this issue. Sorry to mix it in here, but I just stumbled upon those and found it would be a shame to leave them unfixed.
The second patch contains the real changes related to this issue.

fago’s picture

thanks! I've just gone ahead and committed your comment fixes, so we can concentrate on the main patch here. Here are some comments:

entity_key_array_by_property(entity_load(...))

Let's introduce entity_load_by_name() as this will be something often required. Also, let's use this function in the test-cases, as that way the stay better readable.

   protected function getEntitiesKeyedByNumericID($entities) {

We should remove that function and instead use the new helper.

+      // @todo This assumes $entities is keyed by name, not id. Does this still
+      //   apply?

hm, for viewing I'd suggest the following:
* Improve entity_view() to take arbitrary arrays of entities (with any keys).
* Do it as for the rest, invoke entity-level hooks always keyed by numeric id, type-specific hooks by name. Also for keying the rendered-array, I'd go for the names as modules altering it would do that by name.

+ * @param array $entities
+ *   The array of entities to convert.

Afaik we don't use that "array" type in doxygen comments in Drupal yet, so let's stay conform to that convention and don't do so.

I don't know, however, how well this would work in practice, passing different arrays to the two hooks.(On the other hand, those aren't alter hooks, so there shouldn't be too much of a problem, just using the entity_key_array_by_property() function.)

I think we should go for that. It should be fine with the entity_key_array_by_property() function.

Also note that, if third-party modules storing attached data for particular entities would simply reference the entity ID as string, then it is not possible to perform a SQL JOIN if the original entity ID is stored as integer. MySQL apparently allows that, but PostGreSQL does not. Of course, it's use-case specific whether you'll ever want to join on your data, but in terms of this discussion, I guess it's highly relevant.

What about that:
* Make sure any entity stored in DB has a numeric id, as the entity API currently does.
* Always store references using the numeric id. Store the name key additionally, if the relation should use the name.
* Then the 3rd party module needs to care about keeping the numeric ids for named entities up2date.

Or - maybe just using two columns one numeric and one alphanumeric is more feasible?

fago’s picture

Status: Needs review » Needs work
bojanz’s picture

Providing a simple autoincrement (even if it's not the "main" identifier) for solving the join issue sounds good.
Store the alphanumeric key in the base tables for additional lookup (like in the "uuid" idea).

Crell’s picture

fago: Actually noting arrays in docblocks (and interfaces) is now allowed and encouraged by the coding standards, although not fully implemented in core yet. It should be done here, though.

drunken monkey’s picture

Let's introduce entity_load_by_name() as this will be something often required. Also, let's use this function in the test-cases, as that way the stay better readable.

Done.
Or did you mean to also update the tests where I now used $id to reference the right item?

We should remove that function and instead use the new helper.

I didn't remove the function to minimize the API breakage, but I guess it's highly unlikely anyone has used it up to now anyways. So, done.

hm, for viewing I'd suggest the following:
* Improve entity_view() to take arbitrary arrays of entities (with any keys).
* Do it as for the rest, invoke entity-level hooks always keyed by numeric id, type-specific hooks by name. Also for keying the rendered-array, I'd go for the names as modules altering it would do that by name.

Done.
There are no type-specific hooks there, though, as far as I can see.

Afaik we don't use that "array" type in doxygen comments in Drupal yet, so let's stay conform to that convention and don't do so.

The popular and documentation-wise exemplary Search API module does, and I try to follow its maintainer's shining example wherever possible. Also see EntityAPIControllerInterface::create(), where it is used as well. But your choice of course – if that's the only thing still preventing this patch from being committed, I'm more than glad to remove it. ;)

drunken monkey’s picture

Latest patch seems to have a bug still, the "Entity has been rendered" check on line 267 fails. I seem to have made some mistake in the view() method, will debug tomorrow.

fago’s picture

thanks Thomas!

@array-comment: oh fine, then let's do so and use it :)

>Or did you mean to also update the tests where I now used $id to reference the right item?
Yes, that way it is much easier to follow with which entity we are dealing with.

 /**
+ * Wrapper to entity_load() to return the entities keyed by the name key, if it exists.
+ *
+ * @see entity_load()

My little English skills would tell me to remove the comma before the if? The same in @return. Also, @see should be at the bottom of the docblock, below @return (+ spacing).

+ * @param $ids
+ *   An array of entity IDs, or FALSE to load all entities.

I guess we should say $names here then, and also document we fall back to $ids?

-   * Fixed to make attaching fields to entities having a name key work.
+   * Fixed to call type-specific hook with the entities keyed by name, if they
+   * have one.
    */
-  protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
+  protected function attachLoad(&$entities, $revision_id = FALSE) {

Again the if comma. Also, attachLoad() is overridden so I'd prefer to stay with the existing function signature.

@view-ing:
We should make sure the render-array is keyed by name + entity_prepare_view() needs to be moved upwards into the if clause.

Also, we are missing lots of documentation fixes to make clear where $ids and $names are supported/expected. I'll take a stab on that asap.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
15.88 KB

The attached patch should address your comments.

My little English skills would tell me to remove the comma before the if? The same in @return. Also, @see should be at the bottom of the docblock, below @return (+ spacing).

I'm just gonna believe you regarding the former (and similar corrections). However, @see is where it also is in the entity_load() documentation, I just copied that.

EntityAPITestCase::testRendering() still produces a fail on line 260, however. I managed to track down the issue, but was unsure on how to resolve this.
The test creates (but doesn't save) an entity and then attempts to view it. However, since the entity has no ID yet, it will be filtered out in the controller's view() method:

$entities = entity_key_array_by_property($entities, $this->idKey);

This filters out all entities that have the ID field not set.

In my opinion, the behaviour makes sense, which means the test would have to be corrected. Viewing a (fieldable) entity that isn't yet saved and has no ID doesn't really make sense, and when we would call field_attach_prepare_view() with it we would even violate core's Field API specification.
If you say the test is correct, we should probably change the above code to store the differently keyed array in a different variable, and use that only for the Field API calls.

drunken monkey’s picture

Come on – first everyone complains and now there's no-one willing to help testing this, or finding necessary documentation changes? This is quite a large change, so should probably best be controlled by several people, not just fago and me.

bojanz’s picture

I have contrib time on friday (finally), so I can help test it.

Shadlington’s picture

If testing is what you need then I'll help out :)
Any suggestions for how I should go about it?

Framework-y changes like this leave me a little lost as to what to test.

drunken monkey’s picture

If testing is what you need then I'll help out :)
Any suggestions for how I should go about it?

Thanks for the offer, but I don't think this can be tested conventionally. I'd rather have said people should test whether their modules still work, or maybe some things even work now that didn't work previously. Also, reviewing the code might of course be helpful and might unveil some problem or bug.

fago’s picture

>However, @see is where it also is in the entity_load() documentation, I just copied that.

Still, according to the coding standards, it should be at the bottom.

@view-ing:
We need to be able to view entities without id, as this is critical to implement previews. I've improved the logic such that entities without id are keyed with NULL, just as it does core for nodes. Ugly, but that's how it seems to be.

I've worked over the patch and
* fixed the view-ing issue
* improved function naming to use entity_load_multiple_by_name(), so it reflects it loads multiple entities. Thus, I've also added entity_load_single() which also works by name -> so we are well-prepared for renaming entity_load() to entity_load_multiple() in d8. Opinions on that?
* added tons of documentation fixes / improvements

Unrelated, the additional entity_info access of the load-by-name function had triggered an issue which lead to a second entity-defaults-rebuild for exportables upon module install. Thus, I've improved that logic to prevent the unnecessary second rebuild + tightened the tests to prove the rebuild runs just once.

Anyway, I think the attached patch does a good job in minimizing the resulting API changes, which are:

API changes of the attached patch:
* entity_load() now always returns entities keyed by numeric id. -> Use entity_load_multiple_by_name() instead if it bothers.
* hook_entity_load() / hook_entity_prepare_view() are now always invoked with entities keyed by numeric id.
* the provided controller now also keys entities always by numeric id, internally - what might affect modules extending it.
* entity_view() accepts now arbitrarily keyed entities -> a safe change.
* entity_delete_multiple() now accepts both, ids and names -> another safe change.

I've also included patches that implement the API changes for Rules and Profile2 - so others making use of exportable entities can easily see what's necessary to update the modules.

Please re-view carefully and in particular help shaping the docs, so they are actually clear. I'm probably not the best-doc writer here as I've written most of the code, so I'd appreciate feedback in particular of people using the entity API exportables.

Come on – first everyone complains and now there's no-one willing to help testing this, or finding necessary documentation changes? This is quite a large change, so should probably best be controlled by several people, not just fago and me.

Yes, it surely makes no sense to commit this before enough people gave their green light that this fixes their previous concerns. So, any reviews...? :)

fago’s picture

I've just imported the patch of #38 in a separate branch, so we build from that to track further changes.
-> http://drupalcode.org/project/entity.git/tree/refs/heads/ids

fago’s picture

FileSize
29.22 KB

In the previous patch, the ordering of returned entities and the static caching was broken if entities were loaded by name. Fixed that.

For details of the changes, see the Git repository. I'm attaching just the patch against 7.x.-1.x here.

bojanz’s picture

The code looks good. Big step in the right direction.

Was only wondering about this:

+ *     entity hooks like hook_entity_load() are invoked with the entities keyed
+ *     by numeric id, while entity-type specific hooks like
+ *     hook_{entity_type}_load() are invoked with the entities keyed by name.

The entity-type specific hooks are provided by core, right? Isn't it inconsistent to return the entities keyed by name here as well?
Any special reasoning for that?

Nitpicks:

+ *   The renderable array, key by name key if existing or by numeric id else.

should be:

+ *   The renderable array, keyed by name key if existing or by numeric id else.

...and...

+ *   If there is no information how to view an entity, FALSE is returned.

should be:

+ *   If there is no information on how to view an entity, FALSE is returned.

I am very happy to see the issue resolved, big thanks to fago & drunken monkey for working on this.

fago’s picture

thanks for the review.

>The entity-type specific hooks are provided by core, right? Isn't it inconsistent to return the entities keyed by name here as well?

No, with entity-type specific hooks I mean something like hook_profile2_type_load() - which is not provided by core, but the module providing the entity / the entity API. As we are introducing the hook, we are free to key the array as we wish.

Yes, keying it by name there is different to the result, but I think it makes sense as e.g. to access a specific profile type I can directly use something like $types['type_name']->label = 'foo'; instead of having to search for my profile-type in the wrongly keyed array. Additionally, that way we avoid a pretty harsh API change. Makes sense?

bojanz’s picture

Yes, it does. +1 on RTBC then.

drunken monkey’s picture

The entity-type specific hooks are provided by core, right? Isn't it inconsistent to return the entities keyed by name here as well?

For core entities they are, yes. However, core entities don't use the Entity API's entity controller class, so this is irrelevant. They also don't specify name keys, so their IDs are simultaneously there names, making the distinction for core entities unnecessary.
This comment pertains only to entities using the EntityAPIController controller class and specifying a name key – and, as fago wrote, as we provide their hooks, we are free to define them as we wish.

fago’s picture

FileSize
29.44 KB

incorporated suggestions in #41, and added two more comments.

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

Nice. We can convert entity_object_load() to use entity_load_single() in a followup.

fago’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
45.58 KB

As we have already an API changes for modules implementing exportable entities, I think we should separate the current controller into two separate ones: One general one + one caring about exportables.

I think that makes much sense, as far not every entity is exportable. Next, the general controller stays simpler and code is better separated. Attached patch does that split (see Git repo). That way we have two controllers:
* EntityAPIController and
* EntityAPIControllerExportable

Thus, the attached patch adds another API change for exportable entities: The controller needs to be changed.
Among that, I fixed another issue with static caching for named entities and had to remove transactions support for the delete method, as the previous work-a-round for #1007830: Nested transactions throw exceptions when they got out of scope somehow stopped working. We can re-enable it once 7.3 is out.

Please review.

>We can convert entity_object_load() to use entity_load_single() in a followup.
yep

Damien Tournoud’s picture

That approach looks good to me.

I have no comments about the patch, which looks ready to me as is it now.

drunken monkey’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.
I haven't got the time to look at the patch in detail, but for me everything works after a few changes to the Search API and the code looks good. Along with Bojan and Damien, I guess that's enough for setting this to RTBC.

It's just a pain that there's no good way of informing all people using the module of such API changes. I guess there'll be some who are suddenly confronted with fatal errors of whose origins they have no idea.

fago’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
931 bytes
1.63 KB

ok, I've committed it.

That means modules providing exportable-entities face some API changes:

API changes:

  • entity_load() now always returns entities keyed by numeric id. -> Modules may replace calls to entity_load() with entity_load_multiple_by_name() to retrieve entities keyed by name.
  • hook_entity_load() / hook_entity_prepare_view() are now always invoked with entities keyed by numeric id.
  • While generic entity hooks like hook_entity_load() are always keyed by numeric id now, entity-type specific hooks (e.g. hook_profile2_type_load() in case of profile2) are still keyed by name, so that does not change.
  • The provided controller now also internally keys entities always by numeric id - what might affect modules extending the controller.
  • Exportables now got a separated controller EntityAPIControllerExportable. Modules using the 'exportable' feature or a 'name' key have to use this one now.

Apart from that the patch also:

  • adds a new API function entity_load_single()
  • improves entity_view() to accept arbitrarily keyed arrays

I'm going to create a new entity API beta release now, so modules implementing exportable entities can roll new releases too and depend on this beta release.

Also, attached are two patches that show the necessary changes for two modules implementing exportable entities: rules and profile2.

fago’s picture

1.0-beta9 including this is out.
Note that I cannot recommend specifying a versioned dependencies for the update, due to #1013302: Versioned dependencies fail with dev versions and git clones.

zenlan’s picture

Can I please get a clarification...

"entity_load() now always returns entities keyed by numeric id."

The ID does not appear to be numeric necessarily. I have entities that have unique IDs that are strings and was worried that my modules would break after testing the update but it seems that the change is purely from name to ID and that mention of the word 'numeric' is a red herring, an assumption that IDs will always be numeric.

Is this correct or will I encounter some errors that I am not yet aware of?

Thanks in advance. :)

drunken monkey’s picture

Looks like fago made the same mistake he valiantly fought before. ;)
The "normal" IDs are meant (those defined in $info['entity keys']['id']) which, as you say, don't have to be numeric. And I don't see a reason why it shouldn't work with non-numeric IDs – especially if you didn't encounter any problems either.

fago’s picture

from the docs:

id: The name of the property that contains the primary id of the entity. Every entity object passed to the Field API must have this property and its value must be numeric.

Thus, if you want to make your entity fieldable, it has to be numeric. If not, it's perfectly fine if it is not numeric.

However, Entity API module always goes with a numeric 'id', while it introduces an additional 'name' key for uniquely identifying exportables. This patch fixed the controller such that entity_load() always returns the entities keyed by 'id' key, but never by 'name' key.

zenlan’s picture

My entities are pulled from external Solr indexes that are not under my control, I have to take whatever IDs they give me (one index has urls as the IDs!). I have no requirement for these entities to be fieldable but its good to know of this constraint.

Thank you for the info guys!

xatoo’s picture

Status: Fixed » Needs review
FileSize
536 bytes

The save method of the EntityAPIControllerExportable class should return SAVED_NEW or SAVED_UPDATED according to EntityAPIControllerInterface. However, no value is ever returned. Please prepend line 694 of includes/entity.controller.inc with a return statement or apply the following patch.

fago’s picture

Status: Needs review » Fixed

Thanks xatoo, I've committed the follow-up fix.

Status: Fixed » Closed (fixed)

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