Add entity 'label' key info (e.g. title on node)

I forced both bots to do a new self test, which will test core without your patch. If they fail, they'll shut themselves off.

Seems 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.

There 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.

I 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.

That 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.

I'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?

>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.

+++ includes/common.inc

+ *       @see entity_label().
  *   - bundle keys: An array describing how the Field API can extract the

We don't put an @see statement in the middle of a docblock.
Replace this with a normal "See"

+ * @param $entity
+ *   The entity for which to generate a path.
+ * @return

Put a newline between the @param and @return block

Powered by Dreditor.

Great!

+
+ // 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.

+ // 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.

Fine, 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.

This 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.

This patch looks great to me!

 }
 
+/*
+ * Returns the label of an entity.
+ *
+ * @param $entity_type

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.

hm, imho the new comment reads a bit clumsy. What about that instead:

+ *   - label callback: Optional; A function taking an entity as argument and
+ *     returning the label of the entity, e.g. the title of a node, or the
+ *     subject of a comment. This property can be used when the label is a
+ *     result of complex logic, else just specify the entity key "label" (see
+ *     below).

?

I 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.

Should the user entity label be using a label callback with format_username() so we're not promoting direct use of $account->name?

Yes, 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.

So, 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?

Well, auto_nodetitle would still have to do some logic, but yes, the hard stuff is done with this improvement.

looks ready to me

subscribe

I 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.

is this going to be committed? or pushed to D8?

I'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.

>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.

I'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.

Re-roll.

"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.

As 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.

I'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.

For the record, I agree that "label" is misleading. Since "name" applies to users and terms at least, this would be semantically clearer.

I 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():

function taxonomy_entity_info() {
       'entity keys' => array(
         'id' => 'tid',
         'bundle' => 'vocabulary_machine_name',
+        'label' => 'name',
       ),

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.

I 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().

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.

Exactly ;) 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?

subscribing

Suscribing

Suscribing

subscribing

subscribing

subscribe

What 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...)

I think all that's left here is documentation. Therefore, removing the "API change" tag.

I'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.

OK. Back to #87 status, priority, and tags.