Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Sometimes instead of entity_load(), I'd like to query the tables directly in order to get the ID and label (e.g. for select lists).
This patch adds 'label' key under 'object keys'. Also updated docs with the current implementation of node_entity_info() -- docs seemed to be outdated,
Comment | File | Size | Author |
---|---|---|---|
#83 | drupal.entity-label.83.patch | 12.12 KB | sun |
#82 | entity_label.patch | 12.46 KB | drunken monkey |
#71 | 629484-entity-label-71.patch | 11.45 KB | amitaibu |
#69 | 629484-entity-label-69.patch | 11.61 KB | amitaibu |
#63 | 629484-entity-label-63.patch | 11.08 KB | amitaibu |
Comments
Comment #4
amitaibuhmm, is that a test bot issue? "Failed to install Drupal on MySQL 5.0 ISAM"
Comment #7
amitaibuFor some reason *many* core tests on my local fail, so re-rolled and let bot try.
Comment #11
amitaibuI think there's a problem with the bot here - can someone please check this?
Comment #14
deekayen CreditAttribution: deekayen commentedI forced both bots to do a new self test, which will test core without your patch. If they fail, they'll shut themselves off.
Comment #15
deekayen CreditAttribution: deekayen commentedSeems to be your patch that's preventing core from installing. Both bots passed the self test and have been checking other patches with passing results.
Comment #16
amitaibu/* me not gonna blame bot again for stupid things I did ;) 10x deekayen...
Comment #17
amitaibuOk, let's see how #571654: Revert node titles as fields turns out before doing anything with this issue.
Comment #18
amitaibu#571654: Revert node titles as fields has been rolled back, so back to needs review.
Comment #21
amitaibure-rolled.
Comment #22
amitaibu#21: 629484-entity-label-21.patch queued for re-testing.
Comment #23
chx CreditAttribution: chx commentedThere is no use case, example, documentation, test, anything. Not to mention that you might need more than a key (might -- I am not sure because I cant be sure what do you want here) it might need an optional callback.
Comment #24
amitaibuPatch adds 'labal callback' property and docs.
I have seen no tests for uri callback, so I'm not sure where to put it -- each entity should test it's own callback?
Comment #25
amitaibuFix typo.
Comment #26
amitaibuHere's a code snippet of a use case in OG7:
Comment #27
catchI can see the use case but can we please not call it label_field, that's going to create massive confusion with field API. This should just be added to the 'object_keys' array.
Also I have no idea what will happen with this if we try to move label-ish things to fields in D8, but since that didn't happen and title as field showed how complex it is, that shouldn't necessarily stop this going in but worth bearing in mind.
Comment #28
amitaibu10x catch -- Moved label under object_keys. Let's see what bot thinks of it...
Comment #29
amitaibuviolets are blue and tests are green :)
Comment #30
fagoThat one makes a lot of sense.
@object-keys-label:
It's documented to be a "field in the base column", but that's not what the "object-keys" are for. It should just work like the other object keys - thus it should specify the key the entity object is using for the label property.
Then entity_label() could just return $entity->$label_key - that's it. Don't know whether we need the possibility to specify a 'label callback' instead, probably just having the key is fine. Also having the key in place would be useful for the RDF stuff to map those key to rdfs:label.
Comment #31
amitaibu@fago,
> It should just work like the other object keys - thus it should specify the key the entity object is using for the label property.
One of the reasons for the patch, is that we don't need to have the entity loaded to get the label. We should be able to query it directly, for example when we want to show a selectlist with 100 node titles. This is the reason we specify the DB column, and not the entity key.
Aslo, Maybe we should use the same idea of entity_uri #699440: Add bundle support to entity_uri callback to remove performance overhead of forum_url_outbound_alter() (regression from D6) -- and unify those functions to entity_property($property, $entity_type, $entity).
Comment #32
amitaibu- removed label callback from each entity, and moves logic into
entity_label
-- if there's a callback it will be fired, otherwise use the label key.- A bit better docs.
Comment #33
fagoI'm not sure about this:
>+ * This property can be also used when a module wants to query the
+ * database directly in order to get the entity label, without loading the
+ * entire entity object. Note that it is possible that an entity label is
+ * a result of more complex logic, thus can't be queried. In such a case
+ * the "label callback" should be used. @see entity_label().
Well, it also won't work if the entity doesn't use a db backend. But still specifying the label key + no callback might make sense?
Also, to make use of that but still falling back to loading the full entity, quite some special casing is needed anyway. Next entity_uri() needs the full entity to generate the uri anyway, so the typical use-case to generate a link needs the entity object. So shouldn't we just use entity_load_multiple() anyway and remove that db-specific special thing?
Comment #34
amitaibu> Well, it also won't work if the entity doesn't use a db backend
Right, didn't think of it.
> But still specifying the label key + no callback might make sense?
Yes, actually in most cases you won't specify a callback function, and can just use the label key.
> So shouldn't we just use entity_load_multiple() anyway and remove that db-specific special thing?
My concern is that if we want to show a selectlist with 100 node titles do we really want to node_load() all 100 nodes.
Comment #35
fago>My concern is that if we want to show a selectlist with 100 node titles do we really want to node_load() all 100 nodes.
I see. I just wonder, if one has to fall back on loading entities + using entity_label() on the entity anyway, whether the performance impact of using entity_load() to load all entities at once vs a direct query is worth the troubles.
Comment #36
amitaibuOk, also after talking with fago and DamZ, I'll change the patch to only work will $entity->label, and entity_callback - and avoid advocating direct query of the DB.
DamZ also suggested for my use case to use an autocomplete field when there are more then X items in the list.
Comment #37
amitaibuPatch changes API docs, and no longer refers to query of DB. Also the file entity label key was added.
Comment #38
aspilicious CreditAttribution: aspilicious commentedWe don't put an @see statement in the middle of a docblock.
Replace this with a normal "See"
Put a newline between the @param and @return block
Powered by Dreditor.
Comment #39
amitaibuFixed aspilicious' comments.
Comment #40
fagoGreat!
+
+ // The label property marks the label location (e.g. title on a node object)
+ // we don't need to treat it as a reserved field name.
+ unset($info['entity keys']['label']);
Why not? A field of this name would conflict with the existing label key, not?
I think you forgot to add the label key for vocabularies.
Comment #41
amitaibu[Edit: GURU meditation error?! :)]
Comment #42
amitaibu> + unset($info['entity keys']['label']);
>
> Why not? A field of this name would conflict with the existing label key, not?
That's the issue that caused the tests to fail at first. Upon installation the "label" field couldn't be created -I've added a bit to docs, to make it clearer.
> I think you forgot to add the label key for vocabularies.
thanks, added.
Comment #43
fago+ // installation the original label field will not be able to be created.
Which original label field? *confused* We have no label as field, no? Even it won't be supported by a simple entity key, as the field API doesn't use a simple string property anyway. So I re-rolled the patch without that hunk, I'm curious what breaks.
Comment #44
fagoFine, no need for that hunk! ;)
ok, we still need tests. Unfortunately there is no "entity" testing file/module yet. I'd just put the test into the system module's system.test and add a small entity_test.module to the simpletest/tests modules. Testing two new entity types, one using the key and one using the callback should be fine.
Ideally we'd convert the existing "entity_cache_test_dependency.module" to the entity_test.module, such that this module can server for any kind of entity API related tests now.
Comment #45
amitaibu> Fine, no need for that hunk! ;)
Cool, something must have changed, since the first time I've rolled the patch -- thanks for checking it!
Comment #46
amitaibuPatch is untested -- my home laptop doesn't seem to run simpletets properly.
In this patch:
- Pass to the label callback the entity type as-well.
- Add test. I've added them in field.test as there are already helper functions there that deal with entity creation.
Comment #48
amitaibuLet's see if bots likes this one better -- if it won't work, I'll continue on Sunday on my office computer.
Comment #50
amitaibuFixed failing tests.
Comment #51
amitaibuDiscussed with catch on IRC that in order to keep this patch simple, the tests should remain in field_entity. A follow up patch can move the tests to an entity_test module.
Comment #52
amitaibuRe-roll to apply to head. No code change.
Comment #54
amitaibu*sigh*, wrong patch...
Comment #55
amitaibucorrect status.
Comment #56
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis feature is needed by groups, and is a nice addition to the entity API.
The patch looks sound on visual inspection (except the fixes the FieldAttachStorageTestCase, but this set of tests is bound to go away with the Listing API), adds a few tests and doesn't introduce regressions.
Me want.
Comment #57
amitaibu> except the fixes the FieldAttachStorageTestCase, but this set of tests is bound to go away with the Listing API)
Listing API is in -- re-rolled
Comment #58
amitaibu#57: 629484-entity-label-56.patch queued for re-testing.
Comment #60
amitaibure-roll
Comment #61
amitaibuIt was already RTBC and tests are green, so I think it's safe to re-mark as RTBC.
Comment #62
fagoThis patch looks great to me!
It should be /**. Also the added docs to hook_entity_info() 'label callback' don't tell me that there is the label key I can use instead. Thus if I don't coincidently stumble over that, I don't know whether I should implement it.
Comment #63
amitaibuRe-rolled to address #62 comments. Thanks fago.
Comment #64
fagohm, imho the new comment reads a bit clumsy. What about that instead:
?
Comment #65
amitaibu@fago,
I explicitly specify that label is under "entity keys" , so it won't be confused with the entity label property.
Comment #66
fagoI see, makes sense. Then back to RTBC!
Having a label for an entity is rather critical for doing anything on top of entities. Thus I guess this is rather important for some field types too. Then the patch looks fine and comes with tests.
Comment #67
amitaibu@Dries,
Webchick asked me to give a summary of why this issue is important, I'll use OG's case:
OG is no longer node-centric. This means that a node can be a group, but also a user or a taxonomy term. In fact every entity that is fieldable can be group.
OG can't be responsible, nor does it know how to extract the label of any entity -- it's up to the entity to "explain" Drupal how it's label is formed - similar to entity_uri().
The simple case is just returning the key of the label (e.g. "title" on node entity). If complex logic is needed an optional label callback can be set.
Comment #68
Dave ReidShould the user entity label be using a label callback with format_username() so we're not promoting direct use of $account->name?
Comment #69
amitaibuGood idea Dave, re-rolled and added user_label() callback.
Comment #70
Dave ReidYes, that's a definite improvement. Is there a reason we're having to add $entity_type as a parameter for the label callback? It doesn't fit with the pattern currently used by entity_uri which just takes the entity object as the sole parameter. Maybe entity_uri is the messed-up function and should have two parameters. Just looking for which one is right.
Comment #71
amitaibuMakes sense, the callback function is already on specific entity type.
Comment #72
moshe weitzman CreditAttribution: moshe weitzman commentedSo, does this mostly deprecate Automatic Nodetitles module (http://drupal.org/project/auto_nodetitle)? If one wants to custom node title for a given content type, would that be an entity_alter now?
Comment #73
Dave ReidWell, auto_nodetitle would still have to do some logic, but yes, the hard stuff is done with this improvement.
Comment #74
moshe weitzman CreditAttribution: moshe weitzman commentedlooks ready to me
Comment #75
arhak CreditAttribution: arhak commentedsubscribe
Comment #76
chx CreditAttribution: chx commentedI am in strong support of this patch because you can't create a URL out of an entity without having a title / name / label for the URL.
Edit: the path already comes from entity_uri.
Comment #77
rcross CreditAttribution: rcross commentedis this going to be committed? or pushed to D8?
Comment #78
amitaibuYikes, @rcross, don't give the committers bad ideas ;) OG is now relying on it, but I seriously doubt it will be the only one that needs to get the label of an entity...
Comment #79
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm strongly in favor of this too.
One use case I have in mind is building custom translatable terms for internationalization. Currently $term->name is hardcoded all over the place, so you cannot do that properly.
Comment #80
fago>I'm strongly in favor of this too.
Me too, having a possibility to get the label of an entity is critical for any module building upon entities.
Comment #81
drunken monkeyI'm very much in favor of this, too, working with entities generically is a lot more difficult if you can't find out their names. At the moment I've hardcoded a test for $name, $title or $label as an entity field, but that's hardly a solution.
Comment #82
drunken monkeyRe-roll.
Comment #83
sun"label" and "label callback" slightly clash with the existing entity type "label". I wonder why we cannot simply use "title" instead of "label" - why do we always re-invent the wheel? Just because it's "subject" for comments, but "title" for 99.9999% of all other entities? Sorry, but I don't get that.
Re-rolled with various documentation fixes, proper test group, etc.
Also removed needless user_label() wrapper, which was plain function call overhead.
Comment #84
fagoAs you know I'm no native speaker, but afaik "title" fits well for documents (nodes). But with not all entities being documents, label makes more sense to me.
Also I can't think of another entity type beside nodes that uses 'title'? I'd say most of them use 'name', but personally I prefer 'label' as it makes clear that we are talking about a human readable description and not a machine readable name as e.g. vocabs have.
Comment #85
drunken monkeyI'm with fago, e.g. for users a "title" would be something like "PhD" rather than the name.
That an entity info now can contain "label", "label callback" and "entity keys[label]", with the first having got nothing to do with the others is a bit unfortunate, but I think the documentation is clear enough on this.
Comment #86
mossy2100 CreditAttribution: mossy2100 commentedFor the record, I agree that "label" is misleading. Since "name" applies to users and terms at least, this would be semantically clearer.
Comment #87
webchickI talked with Amitaibu about this some in CPH, along with DamZ. While I'm (as always) heavily resistant to any sort of API change at this stage in the release cycle, it does definitely seem like this is something that's a missing link in the entity API. Now that a few contributed modules are starting to make use of entities (OG, Drupal Commerce, etc.) it's becoming more clear that such functionality is needed. Apparently Dries has already given verbal sign-off on this as well.
On the naming, I'm a bit unsure. "Title" has a very specific meaning within the context of nodes. "Name" has a very specific meaning within the context of users. "Label", while not perfect, is at least a new word (in terms of core) that doesn't have any pre-existing meaning that people need to "un-learn", and applies fine across all entities I can think of (except, I guess, an entity with an identifying field called "Label" :P) without ambiguity on what's being talked about. So I think I'm fine with "label", at least for D7.
Committed to HEAD. Thanks!!
Randy, the API change here is that modules that declare entities must now declare a new 'label' index in the 'entity keys' array, which points to their primary identifying field (subject for comments, title for nodes, etc.)
For example, in taxonomy_entity_info():
Since Entity API is a new API in D7, I'm unsure whether this needs to go out over the devel list. I leave that to your call.
Comment #88
effulgentsia CreditAttribution: effulgentsia commentedI just saw this issue and the new entity_label() function and 'label callback' seems cool, but as in #823428: Impossible to alter node URLs efficiently, I'm confused whether this is intended for altering in hook_entity_info_alter(). Should node_admin_nodes() be changed to call entity_label() instead of hard-coding that it's the 'title' column? I'm guessing fago would say no, just like node.module builds many queries that hard-code 'node' as the table instead of assuming that 'base table' is alterable. Can we have some more docs that clarify how this is intended to be used?
This is potentially very helpful in a media.module use-case (#910396: Implement the new entity label setting), where we use filename as the label by default, but want contrib modules to be able to add a title field to media entities, so I'm assuming such a contrib module could hook_entity_info_alter() to a 'label callback' that can return the value of a field, but then such a contrib module could only expect this to work in places that actually call entity_label().
Comment #89
fagoExactly ;) The existence of an info hook does not mean altering it magically works. It is information about the entity type, which the providing module defines. However if the providing module wants to allow altering, it could just stick to the information put in the hook and document that somewhere. E.g. implement a helper function/method media_label() that just makes use of entity_label() and document that the label is alterable via hook_entity_info in the function docs.
I don't think we need to document each info property whether its alterable or not, usually they are not. Documenting *generically* alterable properties + document entity type specific supported alterations like the media label should be fine.
Does that make sense?
Comment #90
flokli CreditAttribution: flokli commentedsubscribing
Comment #91
tbeauchemin CreditAttribution: tbeauchemin commentedSuscribing
Comment #92
baduong CreditAttribution: baduong commentedSuscribing
Comment #93
kenianbei CreditAttribution: kenianbei commentedsubscribing
Comment #94
kim.peppersubscribing
Comment #95
squ1rr3l CreditAttribution: squ1rr3l commentedsubscribe
Comment #96
flokli CreditAttribution: flokli commentedWhat about this?
Is the only thing left here documentation work or is there still a possible API change? (when looking at the nearing release of D7 RC1...)
When looking into CVS, I see that there was code commited, but is this code enough? (Bug has still the "API change" tag...)
Comment #97
effulgentsia CreditAttribution: effulgentsia commentedI think all that's left here is documentation. Therefore, removing the "API change" tag.
Comment #98
fagoI'm unsure whether we should document this at all? Else, we'd have to go and document for each key of drupal's info array keys whether it's alterable or not.
If it's unclear though whether something is alterable - and it is, then I think this needs to be mentioned in the docs.
For that issue that would mean, if no one documented it to be alterable for a specific entity type, assume it's not.
Comment #99
effulgentsia CreditAttribution: effulgentsia commentedOK. Back to #87 status, priority, and tags.