$entity->entity_type
Seems like a tiny little property to add during entity load that could really be useful for a bunch of modules. Without such a property, it's near impossible to get the entity type of an $entity object that's been passed to a function, without a second $entity_type parameter being passed as well. This is the case with quite a few Drupal core functions.

My question is sincere... Is there a reason to NOT have an $entity->entity_type property?

I've heard the Rules community talk of this need:
http://drupal.org/node/596222#comment-2356516
http://drupal.org/node/915058

And I'm sure as Drupal 7 gains popularity, module builders will think of new and interesting needs for this feature.

My own use case is that I have created some tokens and token types that pull field data from a given $entity. When I define these token types in hook_token_info() I can add a SINGLE string value as their "needs-data" property, to indicate a key in the $data array (passed to hook_tokens()) that contains the object needed for rendering tokens of this type. Well, for my custom token type, this "needs-data" property is 'entity', and I pass an $entity object as it's value. But when I render my tokens, I sometimes need to know the entity type...

As a workaround, I have added the $entity->entity_type property using hook_entity_load :

function mymodule_entity_load($entities, $type) {
  foreach ($entities as $entity) {
    $entity->entity_type = $type;
  }
}

But I don't want to require this module or rewrite this hook implementation every time I need this functionality. So can we get an $entity_type property added to the $entity object on entity_load()? OR Is there some reason not to...?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

manimejia’s picture

Status: Active » Needs review
FileSize
534 bytes

Here's my proposed solution:

function entity_load($entity_type, $ids = FALSE, $conditions = array(), $reset = FALSE) {
  if ($reset) {
    entity_get_controller($entity_type)->resetCache();
  }
  $entity = entity_get_controller($entity_type)->load($ids, $conditions);
  $entity->entity_type = $entity_type;
  return $entity;
}

(see attached patch)

Status: Needs review » Needs work

The last submitted patch, entity-load-entity-type.patch, failed testing.

catch’s picture

While it's not ideal, we already have entity_extract_ids() (which was inherited from Field API) - unless there's something you really can't do with that, I'd move this to D8.

mrsinguyen’s picture

Status: Needs work » Needs review

#1: entity-load-entity-type.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, entity-load-entity-type.patch, failed testing.

Damien Tournoud’s picture

Priority: Normal » Major

I think it makes sense to have that. You need to know the entity type to do anything with it. entity_extract_ids() cannot be used, because it requires the entity_type :)

Lars Toomre’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Bumping to D8 so this fix can be applied to HEAD and then back-ported to D7. Also updating Issue tags.

I do not have a test environment for checking patches at present. However, this patch makes loads of sense to me since it is difficult to optionally manipulate an $entity without knowing its type. I am hoping others can make RTBC with a testing of the patch.

Dave Reid’s picture

Issue tags: +entity API

Hrm, this might be nice, because then we could possibly remove the $type parameters to some of our entity functions like entity_extract_ids() and entity_uri().

hass’s picture

Can be very helpful in Linkchecker, too.

naught101’s picture

Hell yes. Very useful when using things like hook_entity_insert().

tstoeckler’s picture

FileSize
582 bytes

Let's try this patch.

tstoeckler’s picture

Status: Needs work » Needs review

Sending for a date with the bot.

aspilicious’s picture

+1 in fullcalendar we get the entity type from the view and add it to the entity object. It would be nice if it was just already inside the enitity object :)

fago’s picture

Component: base system » entity system

Imo we should introduce an Entity base class containing easy accessors to important stuff like entity_type, ids, label, ...

DamienMcKenna’s picture

@fago: your idea is pushing into a larger architectural change that would greatly extend Drupal's OOP structure - could we separate that out into another issue and just get this small piece in first? =)

naught101’s picture

fago’s picture

ad #15:
See #1184944: Make entities classed objects, introduce CRUD support - the patch there covers this issue already. I'd prefer putting our energy into that instead of coming up with an interim solution just for changing it again a month later.

Patch in #1 wouldn't cover newly created entities, thus be unreliable.

aspilicious’s picture

I agree this method should be reliable for all new made enity types, else my use case wont work.

DamienMcKenna’s picture

Status: Needs review » Needs work

@fago: as a D8 issue, I can understand wanting more effort put into expanding the API; for D7, which is what the original request was for, we're more limited so adding a small attribute would help.

That said, I do see your point of needing to be able to add $entity->entity_type on other scenarios other than just via entity_load() - maybe it should be added to entity_save() too? For example:

function entity_save($entity_type, &$entity) {
  $info = entity_get_info($entity_type);
  if (!isset($entity->entity_type)) {
    $entity->entity_type = $entity_type;
  }
  ...
fago’s picture

I don't think it makes sense to add anything like this to D7, nor do I think it's properly doable as most entity-type providing modules do not even have proper ENTITY_create() functions but just create stdClass objects on the fly.

The whole Drupal API is built with functions having $entity_type, $entity parameters, whereas the first would be duplicated then. Thus, new code would go without the $entity_type parameter, and we would end up with lots of inconsistent code.

xjm’s picture

Tracking. I think this is very important. Simply having this property in the entity object could, potentially, reduce overhead in many, many cases.

Letharion’s picture

I've been working on integrating the Rules and Relation modules, and gotten stuck on this specific problem, not knowing what entities I get. While it could be fixed in the entity-module, it seems like it really does belong in core.

dixon_’s picture

subscribing.

joachim’s picture

function entity_save($entity_type, &$entity) {
  $info = entity_get_info($entity_type);
  if (!isset($entity->entity_type)) {
    $entity->entity_type = $entity_type;
  }
  ...

Why not simply expect the caller to set $entity->type themselves, and drop the type parameter from entity_save()? Seems a bit redundant.

xjm’s picture

Aye, we have $entity_type all over the place as a parameter, and it is redundant, since really this is logically a property of the entity. Everywhere we have these two arguments, we could replace them with the one.

However, such an API change is extensive and not at all backportable. I think we should focus first on a very small, simple change that could be used in D7, under the "backport small changes to stable releases" principle. I'm not saying we should discount the bigger picture until the small change is either in D7 or rejected for D7--but I think we should separate the two cases, and make iterative improvements.

For this issue, we should:

  1. Keep function signatures the same.
  2. Continue to support entities that do not contain the type parameter. Backwards compatibility is essential.
  3. Provide $entity->type where possible so future development, both for D7 and D8, can start taking advantage of it.

The overall removal of the redundant parameter should maybe be opened as a separate issue, a task that is not and will not be considered for backport, and is therefore free to improve as much as possible without the restrictions above.

(Edited for clarity.)

colan’s picture

bojanz’s picture

Not having a way to know the entity type of an entity is a real DX WTF.

13rac1’s picture

Yes! sub.

craigjones’s picture

subscribing

naught101’s picture

Huh. This would mean that we could use a lot of CRUD functions with a array of entities of different types as the only argument - eg. entity_dele_multiple or something. Probably not that beneficial I suppose.

As far as I understand it, entities don't have to have a bundle, but could we also have a bundle property anyway, that can just be empty for non-bundled entities?

I think I agree with fago on the d7 part of this - even if we get it in to d 7.xx, module maintainers can't assume that users will be keeping their core install up to date, so this is pretty much useless, as everything would need to check for a $entity->entity_type, and if that's not there, just assume it's something else, and probably break.

hass’s picture

Modules can define what core version is required to run. No issue. Not having the entity type causes more issues.

joachim’s picture

Agreed. On D6, plenty of contrib modules needed >= 6.2 because of the changes to the menu API, and I'm sure I've recently seen a contrib module state on its project page a minimum release for D7.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1 KB

Rerolled for D8 because of #1018602: Move entity system to a module. I think that just getting this into D7 on entity_load would solve 80% of contrib's problems.

Status: Needs review » Needs work

The last submitted patch, drupal-1042822-33.patch, failed testing.

xjm’s picture

I confirmed that the same exceptions occur when the node_save() test is run locally with the patch applied:

Creating default object from empty value Run-time notice entity.module 260 entity_load_unchanged()
Undefined property: stdClass::$title Notice node_test.module 134 node_test_node_presave()

xjm’s picture

Status: Needs work » Needs review
FileSize
634 bytes
1.02 KB

Simple fix.

xjm’s picture

I agree that we should consider this simple fix, as @tim.plunkett suggests in #33, and address questions like #19 and #24 as followups.

xjm’s picture

Assigned: Unassigned » xjm
Status: Needs review » Needs work
Issue tags: +Needs tests

chx and @tim.plunkett and I chatted about this in IRC, and we think it would probably be better to move this upstream a little into DrupalDefaultEntityController::load(). The class constructor has:

 $this->entityType = $entityType;

So we just need to add a foreach() in the load method setting $entity->entity_type = $this->entityType;.

We should be able to add tests by adding assertions for each type in entity_crud_hook_test:
Edit: see #1312802: Entity tests weren't moved out of simpletest.info.

I'll reroll.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.09 KB
3.93 KB

Tests to add assertions that $entity->entity_type is set properly for each core entity type when loaded. These should fail without the patch. Confirmed they pass locally with the most recent patch above.

The non-suffixed version is rolled with the patch from #1312802: Entity tests weren't moved out of simpletest.info so that the entity crud tests will actually run.

Status: Needs review » Needs work

The last submitted patch, 1042822-39-tests-with-1312802.patch, failed testing.

xjm’s picture

Status: Needs review » Needs work
FileSize
3.99 KB
4.82 KB

Failed as expected. The attached versions include the fix described in #38. Again, the suffixed patch is the "real" one, while the first includes #1312802: Entity tests weren't moved out of simpletest.info so that tests run.

I changed the tests slightly from #39 so that the assertion text is actually meaningful.

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

+++ b/modules/entity/entity.controller.incundefined
@@ -227,6 +227,11 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+    // Store the entity type in each entity.
+    foreach ($entities as $key => $entity) {
+      $entity->entity_type = $this->entityType;
+    }
+
     return $entities;
   }

Okay, so I half-wrote this by accident. I originally intended:
$entities[$key]->entity_type = $this->entityType;

However, since $entity is an existing object, it works anyway. (Reference: http://drupal4hu.com/node/224)

I'll reroll to remove the unnecessary definition of $key.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
4.81 KB
sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks cool to me.

As a separate follow-up issue for D8, I think it would make sense to see this in $entity->type - but which of course requires to free that property name from arbitrary other usages.

fago’s picture

Status: Reviewed & tested by the community » Needs work

As said in #20 :
The whole Drupal API is built with functions having $entity_type, $entity parameters, whereas the first would be duplicated then. Thus, new code would go without the $entity_type parameter, and we would end up with lots of inconsistent code.

I dislike extending our APIs now in a way it deprecates lots of current code. Yes, the $entity_type, $entity "double-arguments" are ugly, but this is all over D7 and it's far too late to clean this up now.

Apart from that, it doesn't really work that way. Extending the default controller buys us nothing. This change applies to those default controller handled entities only but does not extend the entity API interface/contract - what isn't possible without breaking APIs.
Thus, no one can assume this property is there for every entity, so you cannot rely upon it -> it's useless! And even worse, if lazy developers rely upon it, this code is going to break for entities using another controller or newly created entities.

-> I'm objecting to adding this unreliable and thus useless entity property at this point.
For D8 this is covered by #1184944: Make entities classed objects, introduce CRUD support.

tim.plunkett’s picture

What about going back to #36? That would work for all controllers.

This is too painful in D7, we need something reliable and backportable.

sun’s picture

yched’s picture

#36 means the $entity->type property gets added after the controller's load() method runs. That includes a fair bit of code where the property cannot be relied on (including field_attach_load(), hook_entity_load() and everything below). I.e : sometimes there, sometimes not, not very reliable.

xjm’s picture

Assigned: xjm » Unassigned
fago’s picture

What about going back to #36? That would work for all controllers.

As yched points out it would still not cover the loading phase, but moreover it would not cover newly created entities - so it remains unreliable.

Still, it would extend our APIs now in a way it deprecates lots of current code + comes with subtle API changes, e.g. you'd have to make sure you serialize that property and you don't have conflicting property name (e.g. OG makes use of a property "entity type").

xjm’s picture

My reading of @fago's comments is not actually that the proposed patches need work, but that from his perspective this issue is won't fix for D7 and a dupe in D8.

I appreciate @fago's concerns as described in #51 (though #46 is a bit harsh). However, if this is won't fix for D7, that leaves contrib that handles multiple entity types stuck doing tragic things like:

function mymodule_entity_load($entities, $type) {
  foreach ($entities as $entity) {
    $entity->entity_type = $type;
  }
}

Can anyone suggest a different workaround or backportable fix for Drupal 7 contrib?

joachim’s picture

Brute force method: add an entity_type column to all entity base tables. Then you know for sure it's going to get loaded any time you load an entity.

But seriously. If every contrib module has to do a mymodule_entity_load to dose up the entity, why can't core do it for everybody?

fago’s picture

I appreciate @fago's concerns as described in #51 (though #46 is a bit harsh).

Sry if I sounded harsh, that wasn't my intention.

However, if this is won't fix for D7, that leaves contrib that handles multiple entity types stuck doing tragic things like:

No, you don't need to - I've written quite some modules handling multiple entity types in d7 without such hacks. It's not nice to have two function parameter instead of one, but it's certainly nothing impossible.

xjm’s picture

No, you don't need to - I've written quite some modules handling multiple entity types in d7 without such hacks. It's not nice to have two function parameter instead of one, but it's certainly nothing impossible.

Alright, then please suggest the workaround here. :)

fago’s picture

Usually, no workaround needed:

function mymodule_work_with_entity($entity_type, $entity) {
  // your code.
}

In case you *really* have to wrap it into a single variable, you could just do something like:

$variable = array($entity_type, $entity);
// and ..
list($entity_type, $entity) = $variable;

...or you could also use the entity api module's wrappers:

$wrapper = entity_metadata_wrapper($entity_type, $entity);
$entity_type = $wrapper->type();
$entity = $wrapper->value();

and once #1184944: Make entities classed objects, introduce CRUD support goes into D8, you can finally do

$entity_type = $entity->entityType();
joachim’s picture

Both are those are not terribly nice workarounds.

I guess this is a question of principle: in order to save core from adding a potentially hacky way to add the entity type property, should we force ugly code on the entirety of contrib?[*]

xjm’s picture

The difficulty arises when you don't have the entity type. I don't care about having to use two arguments (well... I do a little, but that's just aesthetics). The concern is when you are in a context where the type is not available.

tstoeckler’s picture

@xjm: That qualifies as a bug report for D7, see for instance: #1096446: entity_label() is not passing along the $entity_type parameter

naught101’s picture

Version: 8.x-dev » 7.x-dev
Category: feature » bug

tstoeckler: if that's the case, then this is that issue, considering that xjm basically just repeated the original post.

tstoeckler’s picture

Well, the use-case in the original report doesn't really count, because you could just as well require the entity_type together with the entity to be passed to token_replace().

The thread then diverged to discuss this matter on a general level, whether it would make to add this to all entites or not. Since @fago made a very strong argument against that, which no one really could defeat so far, I think that is off the table. The question remains whether are cases in core, where the entity_type is falsely not passed along with the entity (as it was e.g. in #1096446: entity_label() is not passing along the $entity_type parameter). If such use-cases are found, they probably deserve their own issue, though.

So can we close this (and collectively cross our fingers for #1184944: Make entities classed objects, introduce CRUD support) ??

xjm’s picture

Issue tags: -Needs backport to D7

#1184944: Make entities classed objects, introduce CRUD support makes this unnecessary in D8, so this is properly a D7 issue. However, I disagree with some of the reasoning about not including this in D7. Rather than waiting for someone to stumble on the next situation where they don't have the type available--or those of this in this issue trying to remember where exactly we encountered it six months ago :P -- why not simply add this property so it's available? It's a very small addition, and people will have to change their entity code when porting to D8 anyway. I also don't think the point about deprecating API is really relevant; we have lots of deprecated stuff in our API, and that's not really a compelling reason not to make it better.

The one problem I can see with adding it is the point @fago mentions in #51, where contributed modules are already doing this:

/**
  * Entity property info setter callback to set the "entity" property for groups and memberships.
  *
  * As the property is of type entity, the value will be passed as a wrapped
  * entity.
  */
 function og_entity_setter($object, $property_name, $wrapper) {
   $object->entity_type = $wrapper->type();
   $object->etid = $wrapper->getIdentifier();
}

I told myself I was unfollowing this issue, but now it's showing up on the majors and they're over threshold. :P

xjm’s picture

I suspect #1301522: field_ui_default_value_widget() does not pass along the entity type when it creates the default value form may have been one of the ways I originally encountered the entity type being unavailable, so crossposting that.

Above statement was entirely false. ;)

catch’s picture

Category: bug » feature
Priority: Major » Normal

Following the discussion here this looks like a feature request to me. I'd be happy to see issues such as #1301522: field_ui_default_value_widget() does not pass along the entity type when it creates the default value form bumped to major if that's the real problem.

Also not sure why this was moved to 7.x, entities are not yet classed objects, and patches get applied to 8.x first per http://drupal.org/node/767608 but not going to change any more issues statuses than I already have.

yched’s picture

Note that #1301522: field_ui_default_value_widget() does not pass along the entity type when it creates the default value form is precisely not a case for this feature request, since it deals with providing a missing $entity_type param in a case where there is no $entity anyway (the "default value" input on the edit form of a field instance - you can get an entity type since you're editing a specific instance, but you have no actual entity).
Thus $entity->entity_type would be of no help here.

xjm’s picture

@yched: Yeah, I realized that after I worked on that issue some more. Editing my comment to indicate it's wrong. :)

The reason this was moved to D7 is that fago has indicated it seems to be a duplicate of #1184944: Make entities classed objects, introduce CRUD support for D8. I disagreed with that four months ago, but that was four months ago and the CRUD patch looks to be getting closer. However, since that won't be backported, the question is:

  1. Whether it's safe to add in D7 at this point (might be good to get webchick's executive yea or nay so if appropriate we just close this).
  2. If it can be added, how to do it in a way that's at least somewhat reliable but does not collide with existing code.
xjm’s picture

Tagging for posterity.

tstoeckler’s picture

Adding the property is not a problem in terms of backwards compatibility.
If we could do it reliably, there would be little harm actually, although fago argues it would be a DX WTF to have the $entity_type "duplicated" in all the functions/hooks that pass it anyway. I.e.:

hook_some_field_or_entity_hook($entity_type, $entity) {
  $entity_type == $entity->entityType; // WTF???
}

Whether that would actually put people put off is arguable though.
The problem is that there is no way to add this property reliably, as fago also points out. We cannot add it to the Interface (i.e. the API declaration) as that would be an API change, and would break existing entity controllers. So we are left adding it to the default controller (DrupalDefaultEntityController). But that doesn't help you if you are dealing with arbitrary entities, as an arbitrary entity might have an arbitrary entity controller, which might or not be DrupalDefaultEntityController, and might or might not provide $entity_type. So you still cannot access $entity->entityType unconditionally.

Won't fix anyone :) ??

merlinofchaos’s picture

We can't rely on entity.module or anything else.

Core can, however, force the issue all by itself:

/**
 * Implements hook_entity_load()
 */
function system_entity_load($entities, $entity_type) { 
  // For every entity, ensure that the 'entity_type' key is set so that we can always figure out
  // what type of entity it is. This effectively reserves the key and we should document this in
  // the document that describes entity reserved keys.
  foreach ($entities as $entity) { 
    $entity->entity_type = $entity_type; 
  }
}
webchick’s picture

That seems like a reasonable approach.

fago’s picture

Please see #46 and #49 - that's not going to be reliable and would basically deprecate all entity-generic code for d7.

Also, I've never suggested relying upon entity api module as *the* solution, see #56

merlinofchaos’s picture

I disagree with your definition of useless. There are times when the entity_type is simply *not available*. *that* is useless.

Saying that making it available 98% of the time instead of 50% of the time is saying that the concept is useless, and that's illogical.

fago’s picture

I'd even argue that introducing a property that is not reliable is even worse than useless, it's going to be the cause of bugs if people rely upon it nevertheless. And that's going to happen once we add it...

If the entity-type of an entity is not available, then that's the bug which needs to be fixed.

fago’s picture

*double post*

merlinofchaos’s picture

Because the problem is solved in D8, IMO in D7 all that is needed is a bandaid.

If the module that implements hook_entity_load has a really low weight so that is likely to come first or very close to first, very few things are going to be in a state where the property is not available. We document the property, the fact that it is a bandaid, and move on. It will be fixed properly in D8 where entities are more mature, but in D7 we are not in a position to make the necessary changes to go that last 1 or 2 percent.

webchick’s picture

I'd like to hear from more of the people in this issue who are asking for this feature as to whether it'd still be useful to you even if it's not available in earlier hooks (field_attach_load(), hook_entity_load(), etc.). If so, then I don't have a problem applying a band-aid in D7 so it's available for the remainder of the hooks.

hass’s picture

I'm for the band-aid. It only helps me. I will conditionally skip the 1-2% as unsupported. No harm for linkchecker.

tim.plunkett’s picture

I would much prefer having it there 98% of the time. Bandaid++

13rac1’s picture

Bandaid+++ 98% is better than 0%.

xjm’s picture

I think so long as we document clearly that one should have a fallback if $entity->entity_type is not set, and that it will be deprecated in D8, something like this is worthwhile. Yes, it's a bit of an ugly hack. But that's the best we can do for now, and I think the pros outweigh the cons. D8 will be better.

Related: If you have been frustrated by this or other issues related to entity DX in D7, review #1184944: Make entities classed objects, introduce CRUD support. It's a big first step toward making the entity API robust in D8, and several initiatives depend on it. :) That's the proper fix.

sun’s picture

Issue tags: -entity API +Entity system

Standardizing on "entity" tag, which will be renamed to "Entity system".

hass’s picture

Status: Needs work » Reviewed & tested by the community

@webchick: Bumping for band-aid commit. Reverting status back to RTBC per sun and other reliable testers. Patch is in #44

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

The patches in #44 are for D8 from before the /core switch, and this issue is currently against D7.

hass’s picture

It looks like both patches are attached to #44. D8 is suffixed with "-d8" - the other not, but I could be wrong.

tim.plunkett’s picture

@hass, both patches are against /modules/entity which does not exist in D7.

Even if it were a patch that could be applied, it would need a retest first before RTBC.

mxh’s picture

has it been already commited to D7? D7.12 still does not provide me the entity type on current object.
This band-aid should be commited to D7, I don't know a reason why it SHOULD NOT be commited.

alanom’s picture

So is this happening or not?

If there are worries about $entity->entity_type clashing with entity_type properties that existing contrib modules have legitimately defined, why not just do the standard trick of namespacing with the unique module name?

function system_entity_load($entities, $entity_type) {
  foreach ($entities as $entity) {
    $entity->system_entity_type = $entity_type;
  }
}

It's not pretty but it works, and for a bandaid, that's what matters.

I don't see any reason why this issue should have been left to just wither and died...

yuriy.babenko’s picture

+1 to the bandaid. I've spent the last week or so working on custom proprietary modules that require entity type from random places, and not having it in $entity has been a consistant pain. Ended up doing the same hook_entity_load() workaround.

AaronBauman’s picture

bump

drecute’s picture

I have a question. Is this a fix for this: http://drupal.stackexchange.com/questions/27747/how-do-i-get-entity-type... ?

For example, in hook_page_alter, I always have to find a way to get the current entity and entity type. I've never been lucky nonetheless.

codycraven’s picture

Bumping for bandaid fix, trying to act on the current page entity and am unable to know the type using menu_get_item().

ottawadeveloper’s picture

I think this is an important fix to put in 7.x.

The entity system appears to be designed to allow a generic method of accessing information in Drupal. It's hard to write anything that just treats entities as entities unless you also know and save the entity type. For example, if you wanted to export an entity for demo content or inclusion in a Feature, you need to save the entity type manually so you know what to invoke upon load. You can't just pass an entity object to a function to be saved, you also need to pass the type to save it as. If I wanted to write a theme function that shows a title of the entity, I need to know the type to pass it to entity_label() which means I need to pass it to my theme function.

In regards to the comments about it being a DX WTF to have this duplicated, I think it's an issue that's already causing DX WTFs. The difference is that the current issue is a DX WTF that's making it difficult to work around certain use cases in the entity system, where as the duplication is simply an oddity to have your head shaken over and moved on from. It doesn't negatively impact what a developer can do, like the current situation does.

mxh’s picture

This feature would be very useful for the Computed Field module since it has generic entity support.

xtfer’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

Rerolled the patch in #44 for Drupal 7. No other changes.

To recap, this merely adds the 'entity_type' key to existing entity's.

hass’s picture

After reading all again, whats the difference to #69?

xtfer’s picture

Three things:
- This occurs inside DrupalDefaultEntityController, not in a hook.
- It's a patch.
- It has tests.

:)

pjcdawkins’s picture

I like the patch in #94 and it works for me.

jitse’s picture

Status: Needs review » Reviewed & tested by the community

Patch at #94 does what it's supposed to do. Works fine here.
"Namespacing issues" mentioned before should be easily fixed as developers who extend entity don't have to anymore.

mxh’s picture

Because the problem is solved in D8, IMO in D7 all that is needed is a bandaid.

Just a short offtopic question: How can I check the type the right way in D8?

dawehner’s picture

$entity->entityType() is the way to go in Drupal 8.

fago’s picture

Status: Reviewed & tested by the community » Closed (won't fix)
  • This won't work in all cases, see #49 and #51.
  • This would be a very fundamental API addition, rendering most of the existing d7 entity code obsolete. It's way too late for that in D7.
  • The patch could break existing entity types, e.g. og memberhsip entities.

Thus, marking it as won't fix.

xtfer’s picture

Well, I'm not sure that I agree that your points (1) and (2) are a reason not to commit it, but I do agree with point (3).

alanom’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

Point one: As people have said repeatedly over the course of the two years this issue has existed, "It's not a perfect fix in 100% of cases" is not a good reason to not apply a band-aid that fixes 98% of cases. See #75 and subsequent posts, this issue has been discussed in depth already.

Point two: It doesn't render any code harmfully obsolete. The $entity_type arguments that get passed around simply become a redundant but harmless convenience that saves typing $entity_type = $entity->unique_entity_type_key; on line one of a hook implementation. Something going from an essential necessity to a redundant but harmless optional convenience isn't an obstacle, and it doesn't outweigh the genuine problem of the gymnastics and hacking that developers are currently forced to do when working with all types of entity.

This has also already been discussed - with many posters saying they value the ability to access the data over code aesthetics. Many posters have described cases where the entity type can't be accessed without hacks.

Regarding it being too late (ignoring the fact this issue has been open for 2 years... this issue was opened the same month Drupal 7 was formally released), have a look at the "sites made with drupal" carousel on the drupal.org home page. There are heaps of serious Drupal agencies and consultants still building sites in Drupal 6! Drupal 7 will be in widespread active use with continuing contrib development work for at least another couple of years. Realistically, D7 adoption probably hasn't even peaked yet. It absolutely makes sense to focus on D8 for core development, but it doesn't make sense to block a tiny, simple patch that aids equivalence and module backporting / upgrading between D7 and D8, at least until D7 is deprecated when D8 has been released and D9 arrives.

Point three: It's a genuine issue that some modules have already defined an entity_type key. But it's easily solved.

(To clarify the problem: this concern about breaking the OG Membership contrib entity is simply that it, and maybe others too, already uses the $entity->entity_type namespace. This module(s) uses the entity_type key to store the entity type it refers to, e.g. "user" if it's a membership for users. So, it stores case specific data different to what we're talking about here. It would need both keys side by side)

As I mentioned in #87 in May, Drupal has for years had a simple fix for namespace conflicts in a variety of contexts: prefix with the unique module name. If it's the system / core modules that define this entity type key, you can avoid namespace problems by simply calling it system_entity_type. Or core_entity_type. Or drupal_entity_type, or entity_entity_type, or self_entity_type, or whatever is the best fit.

If you don't like this approach for aesthetic reasons because this kind of namespacing isn't often done within an entity's keys (because it isn't often needed), remember that it's the same basic idea as prefixing all fields defined by the core field module with field_. Fields apply to all (fieldable) entities and have potential namespace conflicts, so, we solve that problem by prefixing their keys with something unique to the module defining the potentially problematic key (field_). A system defined entity type key applies to all entities and has potential namespace conflicts, so, we solve that problem by prefixing the key with something unique to the module defining the potentially problematic key. If / when a contrib module had a reason to extend other entities, we'd expect it to avoid namespacing conflicts as necessary in the same way. Fits existing patterns.

(even using D8-like entityType as the key would most likely work fine as a way to avoid namespace conflicts since camel case is seldom used in D7)

Any other concerns? Please share them. This issue has been going for two years and the vast majority of people posting have been in favour of a simple "band-aid" that makes developing functionality that works with all Drupal 7 entities more straightforward and less dependent on hacks. I'm struggling to find any other dissent anywhere in this 100-comment thread, other than specific practical concerns that have been discussed and addressed.

DamienMcKenna’s picture

Status: Closed (won't fix) » Reviewed & tested by the community

This should have been added two years ago rather than bikeshed for two years then thrown in the scrap heap. Can someone please just commit this, we can follow up separately with OG?

alanom’s picture

If we namespace it as I suggested, there isn't even a problem with OG. We can commit it, document it, and it's done.

bonobo’s picture

Status: Closed (won't fix) » Reviewed & tested by the community

I'd love to see even the bandaid in place for this, but IMO breaking OG is too high a price.

There are currently over 10K reported installs of the 7.x branch of OG. Has anyone fired up a test site with a patch that incorporates the namespace changes suggested by alanomaly?

dawehner’s picture

For me the actual problem is not OG itself, because hey, fixing the code in OG itself is doable, but people who upgrade core/OG and have some custom code, don't expect their stuff to break, so if we do that, we need some prefixing.

alanom’s picture

Absolutely agree with what dawehner says. There's nothing wrong with the OG code, and it doesn't make sense to expect it and all other contrib/custom module with similar features to change overnight. Particularly since OG's one of those modules with a lively ecosystem of modules around it.

All we have to do is play by the same rules as contrib modules, field module, etc. When introducing anything that could potentially cause namespace conflicts, avoid namespace conflicts with prefixing.

dawehner’s picture

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

Just to get the ball rolling.

This just updates the patch to use self_entity_type, as it feels like the best of the suggested once, feel free to provide other patches.

hass’s picture

Status: Needs review » Reviewed & tested by the community

Can someone explain what exacly "breaks" in OG? Overriding an entity type with the same value cannot be a problem and if OG would have prefixed it' own workaround properly there is no problem. That og has not used a prefix itself sound like a bug to me. A bug that can be fixed on OG side.

hass’s picture

Status: Reviewed & tested by the community » Needs review

Crosspost

bojanz’s picture

@hass
Og uses a column called "entity_type" for the membership entity.
There is no rule saying database columns should be prefixed by module name (which would be quite silly), so there's no bug there.

hass’s picture

OG could run an update hook and rename it or just the key in the object :-)

hass’s picture

webchick’s picture

$entity->self_entity_type = $this->entityType;

Can the name of the property not just be "entityType" to match? That's pretty close to the D8 version, as well.

mxh’s picture

Can the name of the property not just be "entityType" to match? That's pretty close to the D8 version, as well.

+1. I can't follow the meaning of the 'self' prefix.

dawehner’s picture

This might be an idea as well.

Entity API is using that as protected property, so this would cause problems :(

mxh’s picture

I'm also running out of ideas.
Here are some of my suggestions from the last few hours for possible attribute names, sorry if it's not useful, anyway:

<?php
// Surely less often used than entity_type, but also possible to break existing code.
$entity->entity_type_name;

// I think this one barely breaks other ones.
$entity->entity_attached_info['entity_type'];

// Additional _ prefix, can be seen as helper attributes.
$entity->_entity_type;
$entity->_entity_type_name;

// As mentioned on drupal.org/node/1261744:
// "An entity type is a base class".
$entity->base_class; // Or similar like that, maybe another prefix would help.
$entity->entity_base_class;

// Additional help function would be useful,
// it just would return the type attribute.
public function entity_type_get_name($entity) {
return $entity->_entity_type_name;
};
?>

fago is right with #101: It's too late for D7. It is a fundamental design problem you cannot solve quickly in a good way.
However, I think we need the band-aid, even if it's a dirty one. Otherwise, there always will be users complaining about that lost information.

Lars Toomre’s picture

What about an attribute name like 'entityTypeD7'? The naming makes it clear that it is a temporary D7 attribute that will be changed in subsequent Drupal versions.

fago’s picture

Status: Needs review » Closed (won't fix)

"It's not a perfect fix in 100% of cases" is not a good reason to not apply a band-aid that fixes 98% of cases.

It's not the point that it is not perfect, it's the point that it won't work in all cases. Drupal history has shown several times that APIs that are available most times - but not always - lead to the introduction of bugs.

Drupal 7 will be in widespread active use with continuing contrib development work for at least another couple of years. Realistically, D7 adoption probably hasn't even peaked yet.

Yes, but probably most of the contrib code has been written, books and dozens of tutorials have been written that show how you write your own entity types. Outdating all docs and confusing developers is not a good idea.

This has also already been discussed - with many posters saying they value the ability to access the data over code aesthetics. Many posters have described cases where the entity type can't be accessed without hacks.

That's just wrong. I've written dozens of entity type generic code and it's possible to pass on $entity_type *always* if you can pass on $entity. Yes it's not nice, but it's not a hack.

On the contrary - this band-aid fix would be a hack making the d7 entity API a real mess. Consider a new developer getting into d7 code: He will see lots of functions and APIs that pass on $entity_type, then there will be code that uses $entity->entity_type or whatever. But what's right now?

So no, this is not a good idea.

joachim’s picture

> I've written dozens of entity type generic code and it's possible to pass on $entity_type *always* if you can pass on $entity.

Not the entity_uri callbacks though. Which arguably we could fix to pass the type as a 2nd param.

For this issue though I'd say we patch OG. It's still in beta, no?

hass’s picture

If OG is not final, we can go with the original patch. It could be one of the reasons why it's still beta/rc aka "not stable". Most are waiting to see this patch committed to follow up.

alanom’s picture

Because people are still confused about the OG thing: OG has an entity type that uses the entity_type key to store the type of a different type of entity, that this membership entity refers to.

I gave an example earlier: you can have an entity with entity type og_membership (or whatever they call it) that handles memberships for users. It itself isn't a user entity, but it's entity_type key will be 'user' because that's the entity type this og_membership entity deals with.

Conflict:

OG defines $entity->entity_type as 'user'
System defines $entity->entity_type as 'og_membership'

This is a race condition where either system or OG will get information that's not right. If OG wins, code looking for user entities will crash out because it tries to treat the OG entity as a user, which it isn't. If System wins, OG will crash out because it will think this is a membership handler that handles the memberships of other membership handlers, which isn't right and probably doesn't even make sense (I don't work with OG but that's how it looks from a glance at the api).

No conflict:

OG defines $entity->entity_type as 'user'
System defines $entity->self_entity_type (or whatever) as 'og_membership'

That's the (very simple) problem.

And don't forget that there's no reason to assume that OG is the only module out there that defined entity_type keys. Other, lesser-known modules may have done the same, and any number of client-specific custom modules might do the same. There was no reason at the time not to do so, so it's kinda unfair to potentially cause all these sites to crash and burn next time they update when they did nothing wrong.

Moving on, two questions that seem key:

1) What's the deal with the Entity API's entityType key? Can it be used as a less hacky workaround? If developers who need this can just install Entity API and get it, seems fine to me, we can leave core alone.

2) re the question of "Are there / are there not legitimate cases where entity_type can't be passed" - Lots of people have commented that there are, but their comments are vague. Can we please expand on Joachim's comment #121 and establish a list of types of cases where this occurs? Seems like vital baseline info that should have been established early on. Callbacks are one. What else? If there's a pattern, we could do something specific to that pattern.

Nice to see some actual movement and discussion on this.

webchick’s picture

Status: Closed (won't fix) » Needs review

fago, there's a clear call for this feature in core, and you're the only one who seems opposed. Please do not unilaterally mark things "won't fix".

bojanz’s picture

I would be fine with won't fix-ing this as well.
We've survived without the bandaid so far, I myself never had a need for it that couldn't be solved otherwise.

joachim’s picture

> 1) What's the deal with the Entity API's entityType key? Can it be used as a less hacky workaround? If developers who need this can just install Entity API and get it, seems fine to me, we can leave core alone.

If you depend on Entity API module, and make your own custom entity use the Entity class (or a subclass) instead of being an StdClass, then you get $entity->entityType() and $entity->entityInfo().

It's certainly enough if you make your own custom entities; not for core, and not sure how many contrib modules define entities without using entity API.

merlinofchaos’s picture

It's less about modules defining entities without using entity module, and more about contrib modules that interact with entities they did not define and do not use entity module. It's that last bit that's important.

merlinofchaos’s picture

Addendum: People who always use entity module will never actually see this as a problem. We could simply say the answer is "Use entity module, then" but that's not always necessarily attractive. It's a common answer to entity problems, though.

fago’s picture

It's less about modules defining entities without using entity module, and more about contrib modules that interact with entities they did not define and do not use entity module. It's that last bit that's important.

I agree, but for the reasons pointed out several times (see #120) it's too late for that. entity.module isn't a big help here either.

@webchick:

fago, there's a clear call for this feature in core, and you're the only one who seems opposed. Please do not unilaterally mark things "won't fix".

It's not my intention to shut anything down, but we've discussed this issue to length during the last years yet no one came up with solutions for the problems summarized in #120. Thus, imo this "band-aid" would harm the state of the overall subsystem and make things worse - what as subsystem maintainer I try to prevent.

alanom’s picture

Okay, so Entity API doesn't solve the problem.

Questions outstanding about the need for this fix:

q1: Under what circumstances will it not be possible to pass $entity_type? So far, we've got callbacks. Previous comments suggest there are more. Clarifying when precisely this key will be used will help us address the other concerns

Questions outstanding about obstacles to this fix:

q2: Is there a risk of bugs creeping in if this defined key isn't available before entity load?

My suspicion is, not really. Any code looking at this before load will always hit a key is undefined error. The developer will then look at the documentation and see what they're trying to do can't work.

We can answer this question conclusively based on the answer to q1: if this key is only ever needed in cases where the entity has been loaded (e.g.callbacks), there's no problem. If there's a case where this is needed before entity load, we should focus discussion on that case(s)

q3: Will this outdate books and tutorials on how to write and work with entity types?

I think this one depends on how it's done. If we suddenly demand that contrib and core define self_entity_type keys whenever an entity is defined, then yes, that's an unreasonable change to the rules of the game. But I don't see why we'd need to, since we're talking about defining it automatically for all entities anyway. Defining entities will be the same as it was, and books written in Jan 2011 will still apply (a rare thing in web development!)

q4: Will new developers be confused seeing some code using $entity and $entity_type, and some using $entity and $entity->self_entity_type ?

Again, depends how we do it. If we ditch the convention of passing $entity and $entity_type, so code written in 2013 suddenly passes one argument and uses $entity->self_entity_type, I agree with Fago - that would be an ugly, confusing mess.

But we don't need to do that. I'd suggest the convention be:
- Where possible, pass $entity and $entity_type
- Where not possible, use $entity_type = $entity->self_entity_type; as early as possible, then pass $entity_type as before.

This way, code will be pretty consistent. Everything that can be done today will be as before, and the one new thing, $entity_type = $entity->self_entity_type;, will only exist in places where currently, nothing works without hacking (the q1 cases)

mxh’s picture

I think fago's statements which already explained why this band-aid is a bad idea to contribute into core are overall not to be neglected. To go further, whatever attribute we will asign on an entity load to the stdClass entity instance, this could lead to serious trouble on existing sites.

I agree with bojanz that this feature is not essential, it's comfortable. The risk is too high to contribute a new attribute to all exisiting entities for D7 installations (OG is not the problem, I see a big problem in selfwritten, unofficial and custom code).

For those who need this attribute they should just write an own module which is being used on their sites using hook_entity_load().

fago, I've got a question just of interest: Would you agree with a function for core like this one:

<?php
/*
  Return the type of a given entity.

  @param $entity
    an entity like node, user, taxonomy term etc.

  @return
     A string containing the entity type's name.
  */
function entity_type_get_name($entity) {
}
?>
fago’s picture

ad #130:

q1: Under what circumstances will it not be possible to pass $entity_type? So far, we've got callbacks. Previous comments suggest there are more. Clarifying when precisely this key will be used will help us address the other concerns

Any callback receiving $entity and not $entity_type is a bug, which needs to be fixed by passing $entity_type also. Issue fixed.

I'd be happy to see other situations also where this would be a real problem. But as posted in #56 (2nd code snippet) there is always a way to pass on both $entity and $entity_type even in one variable. Yes it's ugly, but it shows this rare situations can be solved without this "band-aid".
Thus, I postulate that such an situation where $entity_type cannot be passed does not exist - thus given that - this "band-aid" is not needed, it's "nice-to-have".

My suspicion is, not really. Any code looking at this before load will always hit a key is undefined error. The developer will then look at the documentation and see what they're trying to do can't work.

That's not the way the bugs will appear. Developers will work with $entity in the situation where the band-aid entity type property is not available, thus they will pass $entity on to further functions, possibly further sub-systems. Those other functions and sub-system do not know about the special-context of the $entity - thus will work with the usual assumptions. As soon as the assume the "band-aid" entity type is there, the bug has been created. We've gone through that we half-baked entities already.

Then as mentioned, it's not only the pre-entity-load hook loading phase but also newly, manually created entities. They would not have the "band-aid" property either. Those entities are passed on to various APIs also, e.g. during entity creation. So yes, there is a high-risk of this introducing bugs. Does a "nice-to-have" band-aid warrant that?

But I don't see why we'd need to, since we're talking about defining it automatically for all entities anyway. Defining entities will be the same as it was, and books written in Jan 2011 will still apply (a rare thing in web development!)

But we don't need to do that. I'd suggest the convention be:
- Where possible, pass $entity and $entity_type
- Where not possible, use $entity_type = $entity->self_entity_type; as early as possible, then pass $entity_type as before.

If we are not supposed to use it, we don't need the "band-aid". Then, as said above there are always ways to pass on $entity_type. Those ways are not always nice, but way nicer than messing with the whole sub-system.

Then even if you are not supposed to use the "band-aid", people will wonder why the heck $entity_type is passed around when it's there anyway (dpm() will show it..). Let's don't unnecessarily confuse people, but live with the entity API we have created in d7. It's not complete, it's not nice, but at least it's consistently not nice. Let's keep at least that.

ad #131:

function entity_type_get_name($entity) {

You mean something like entity_get_type() ? I think everybody would be happy with that function, but for that to work you'll need to have $entity_type inside of $entity - what we don't have and brings us back to this issue.

joachim’s picture

> Any callback receiving $entity and not $entity_type is a bug, which needs to be fixed by passing $entity_type also. Issue fixed.

http://api.drupal.org/api/drupal/includes!common.inc/function/entity_uri/7 doesn't pass entity type to the URI callback.

> You mean something like entity_get_type() ? I think everybody would be happy with that function, but for that to work you'll need to have $entity_type inside of $entity - what we don't have and brings us back to this issue.

The advantage of having that function is that we can use $entity->weird_property_name_that_wont_clash and developers don't need to worry about it.

mxh’s picture

Thanks fago for your reply, just wanted to know if such a function would be ok.

The advantage of having that function is that we can use $entity->weird_property_name_that_wont_clash and developers don't need to worry about it.

This would make it harder for developers to implement information hiding on entities. Whatever property you'll attach, it will definetly create confusion and possibly break existing sites. I think working with properties here is not the right way at all.

It simply seems that there will be no proper solution for this issue in core.

Current patch would just break D7's entity design, which is defined for years now - and so many many modules and APIs are designed for it. It's a fundemental design problem in D7 which forces us to live with it. But it's ok, because you are able to handle it - It's not comfortable, but resolvable.

guedressel’s picture

Hello!
Since my current project required the entity type detection I implemented the function proposed in comment #131:

/*
  Return the type of a given entity.

  @param $entity
    an entity like node, user, taxonomy term etc.

  @return
     A string containing the entity type's name.
  */
function entity_type_get_name($entity) {

    if (empty($entity) || ! is_object($entity))
        return FALSE;

    static $drupal_static_fast;
    if (!isset($drupal_static_fast)) {
        $drupal_static_fast = &drupal_static(__FUNCTION__);
    }

    $entity_class = get_class($entity);
    $entity_bundle = $entity->type;
    if ( ! isset($drupal_static_fast[$entity_class]) ||  ! isset($drupal_static_fast[$entity_class][$entity_bundle]) ) {

        $detected_type = FALSE;

        $reflectionClass = new \ReflectionClass($entity_class);
        foreach ($reflectionClass->getProperties() as $property) {

            if ($property->getName() == 'entityType') {
                $property->setAccessible(TRUE);
                $detected_type = $property->getValue($entity);
                $property->setAccessible(FALSE);

            }

        }

        if ( ! isset($drupal_static_fast[$entity_class]))
            $drupal_static_fast[$entity_class] = array();

        $drupal_static_fast[$entity_class][$entity_bundle] = $detected_type;

    }


    return $drupal_static_fast[$entity_class][$entity_bundle];
}

Looking forward to have Drupal core not protect the entityType Property or [better] provide a getter method for it.

Cheers

Xano’s picture

Are you familiar with \Drupal\Core\Entity\EntityInterface::entityType()?

Edit: Sorry, didn't realize this was for D7.

guedressel’s picture

No - I'm not. But it could solve this issue.
I'm looking forward for Drupal 8!

mxh’s picture

guedressel, thanks for sharing your code. But I doubt #135 could work for D7, since entities are handled as StdClass objects (guessing the line $entity_class = get_class($entity); makes no sense then) and don't have the entityType property. Is your code written for a D8 installation?

guedressel’s picture

hauptm, my code was written for D7 and it works flawless in our environment.
But actually you are right - some times:
It depends how you load the entity instance. If you use entity_load you'll get a Entity instance. If you use something else - like node_load - you could possibly end up with an stdClass instance.

Hence my solution is maybe not that universal usable :-(

cravecode’s picture

It seems like this issue isn't going to be addressed.
I routinely deal with this problem when dealing with fields in a generic manner (i.e.: using field_get_items and field_view_value). I've taken a similar approach as the OP. I'm posting the OP's workaround as a module (Entity Base Type) so I can add it as a dependency in our other modules. Perhaps others could benefit from it as well.

It's not an ideal solution but something is better than nothing in this case. The property is called "base_type" to avoid conflicting with OG.

tl;dr
OP's workaround as a module called Entity Base Type.

nevergone’s picture

And now?

hanoii’s picture

Incredibly after all this time, I also think this is worth adding. I got here looking for a better fix to #3088758: Notice: Undefined property: stdClass::$bid in FieldCollectionItemEntity->fetchHostDetails(). This could have helped enormously.