Improving usability - Name/machine name for relations instead of using the label

Thomas,

I resist adding default fields to relations when they are fieldable -- but I understand you need fields for display in the relation admin UI.

I wonder if a combination of relation_type and/or options for when to use label and/or relation_type can help accomplish your needs.

Attached patch simply adds the relation_type in parenthesis to the relation label in the Relation Entity Collector block form.

Please let me know if this helps and if there are other places we might be able to use the existing relation_type in the UI to help distinguish relation types.

I also ran into this issue which required a fix to display the "user friendly" node type name since normal users were being given access to the Entity Collector and not just admins. I created this patch which will display the proper "user friendly" node type along with the node title so there is no guessing involved.

I am basically calling the core function node_type_get_type($bundle) and then accessing it's ->name property.

I forgot to include my changes to the validate() function with this feature. I have a few fixes/features that I'm trying to break out as several patches instead of one big one. Patch has been updated with everything it should need now.

+++ b/relation_entity_collector/relation_entity_collector.module
@@ -149,7 +149,9 @@ function relation_entity_collector($form, &$form_state) {
+          $node_type = node_type_get_type($entity_bundle);
+          $node_type_name = (!is_null($node_type)) ? $node_type->name : $entity_bundle;
+          $options["$entity_type:$entity_id:$entity_bundle"] = $node_type_name . ": " . entity_label($entity_type, $entity);

I am maybe missing patch context here, but what if the entity is not a node?

My patch won't ever pass the tests defined in the relation_entity_collector.test file because I had to change the key being used.

 function testEntityCollector() {
    $node1key = 'node:' . $this->node1->nid;
    $node3key = 'node:' . $this->node3->nid;

The above needs to be changed so that the .":$entity_bundle" is appended after nid, but I don't know what the test 'node' entity bundle name would be to correct it. Anyone know what it should be?


@joachim if the node_type is not found, the original $entity_bundle name is used. I wanted to do more for other types, but I cannot seem to get non-node entities to show up on the dropdown on my site anymore (even after reverting to the original source). The gist of what I am doing is that by changing the key to be $options["$entity_type:$entity_id:$entity_bundle"] frees up the label text to be anything we wish. I am able to change node entity type names to be what they should, but since I cannot test other types, someone else will need to help properly patch it.

> if the node_type is not found, the original $entity_bundle name is used. I wanted to do more for other types

You should be working generically, and not doing anything node-specific.

You can get bundle labels by doing:

$entity_info = entity_get_info($entity_type);
$bundle_label = $entity_info['bundles'][BUNDLE_TYPE]['label'];

Thanks! I am by no measure a Drupal expert, so knowing the generic way to do things without the means to easily test it is no simple task. I will incorporate the changes you suggest and issue another patch once I figure out the answer to comment #9 about the test case modification.

Incorporating @joachim's code to generalize it for any entity type and adjusting the defined test case to reflect both the key change and the label change.

Uncle Code Monkey,

Thank you so much. This is helpful, and I think should probably be committed to Relation, but @tsvenson's original request was for clarifying/customizing the relation type (first drop-down) in the Entity Collector block, not the endpoint label (second drop-down).

So, with your change (and a few edits as seen in the attached patch + interdiff), maybe you can continue to develop a solution that adds Name and Description fields for use in the first drop-down (among possible other places)?

-Steve

I guess we should put a testbot on my patch.

Some notes on my changes to your patch (the interdiff):

+++ b/relation.module
@@ -989,6 +989,23 @@ function relation_get_type_label($relation, $reverse = FALSE) {
 /**
+ * Return the label for a given entity type and bundle.
+ *
+ * @param string $entity_type
+ *   The entity type.
+ *
+ * @param string $entity_bundle
+ *   The entity bundle.
+ *
+ * @return string
+ *   The label of the given entity type and bundle.
+ */
+function _relation_get_bundle_label($entity_type, $entity_bundle) {
+  $entity_info = entity_get_info($entity_type);
+  return $entity_info['bundles'][$entity_bundle]['label'];
+}
+

Moved this helper function to relation module, as we may want to use it elsewhere in Relation (I am surprised D7 core doesn't have such a function).

+++ b/relation_entity_collector/relation_entity_collector.module
@@ -157,8 +149,8 @@ function relation_entity_collector($form, &$form_state) {
         if ($valid) {
-          $bundle_label = _relation_entity_collector_get_bundle_label($entity_type, $entity_bundle) . ': ';
-          $options["$entity_type:$entity_id:$entity_bundle"] = $bundle_label . entity_label($entity_type, $entity);
+          $bundle_label = _relation_get_bundle_label($entity_type, $entity_bundle);
+          $options["$entity_type:$entity_bundle:$entity_id"] = $bundle_label . ': ' . entity_label($entity_type, $entity);
         }
       }

Keying by type:bundle:id seems to make more sense to me.

+++ b/relation_entity_collector/relation_entity_collector.module
@@ -302,8 +294,8 @@ function relation_entity_collector_validate($form, &$form_state) {
-      // Here we get (entity_type, entity_id, entity_bundle).
-      $break = explode(':', $entity_key);
+      // Get entity info from key ('{entity_type}:{entity_bundle}:{entity_id}').
+      $entity_info = explode(':', $entity_key);
       // Add the label for later display. #options is check_plain'd but we need
       // to do that ourselves.
       $entity_label = check_plain($form['entity_key']['#options'][$entity_key]);
@@ -312,12 +304,12 @@ function relation_entity_collector_validate($form, &$form_state) {

@@ -312,12 +304,12 @@ function relation_entity_collector_validate($form, &$form_state) {
       $next_index = count($_SESSION['relation_entity_keys']);
       // If validation succeeds we will add this in the submit handler.
       $form_state['pick'] = array(
-        'entity_type' => $break[0],
-        'entity_id' => $break[1],
-        'entity_bundle' => $break[2],
-        'r_index' => $next_index,
-        'entity_label' => $entity_label,
-        'entity_key' => $entity_key,
+        'r_index'       => $next_index,
+        'entity_key'    => $entity_key,
+        'entity_label'  => $entity_label,
+        'entity_type'   => $entity_info[0],
+        'entity_bundle' => $entity_info[1],

Break was a silly var name. ;) Changed order of entity properties to satisfy my eyes.

+++ b/relation_entity_collector/tests/relation_entity_collector.test
@@ -27,8 +27,8 @@ class RelationEntityCollectorTestCase extends RelationTestCase {
   function testEntityCollector() {
-    $node1key = 'node:' . $this->node1->nid . ':article';
-    $node3key = 'node:' . $this->node3->nid . ':page';
+    $node1key = 'node:article:' . $this->node1->nid;

Keys make more sense as type:bundle:id here, too.

+function _relation_get_bundle_label($entity_type, $entity_bundle) {
+  $entity_info = entity_get_info($entity_type);
+  return $entity_info['bundles'][$entity_bundle]['label'];
+}

I really don't think we need a helper for this; it's just two lines of code. Lots of places where you're working with the entity type you've got hold of the entity info too, in which case it's only 1 line of code.

@joachim: Agreed.

From the interdiff:

+++ b/relation_entity_collector/tests/relation_entity_collector.test
@@ -27,8 +27,8 @@ class RelationEntityCollectorTestCase extends RelationTestCase {
@@ -48,13 +48,13 @@ class RelationEntityCollectorTestCase extends RelationTestCase {

@@ -48,13 +48,13 @@ class RelationEntityCollectorTestCase extends RelationTestCase {
       ->execute());
     $path = 'relation/' . $result[0];
     $link = l($relation_type, $path);
-    // Rebuild the message. Use the known node labels to make sure the message
-    // contains those.
-    $node1_label = entity_label('node', $this->node1);
-    $node3_label = entity_label('node', $this->node3);
+    // Rebuild the message using the known bundle and entity labels to make sure
+    // the message contains those.
+    $node1_label = _relation_get_bundle_label('node', 'article') . ': ' . entity_label('node', $this->node1);
+    $node3_label = _relation_get_bundle_label('node', 'page') . ': ' . entity_label('node', $this->node3);
     $items = array(
-      _relation_entity_collector_get_bundle_label('node', 'article') . ': ' . $node1_label,
-      _relation_entity_collector_get_bundle_label('node', 'page') . ': ' . $node3_label,
+      $node1_label,
+      $node3_label,
     );
     $item_list = array(
       '#theme' => 'item_list',

Apparently this is where the test is failing, but I can't tell why.

I wasn't the only one to make the mistake about what the original issue was for (Relation Type dropdown vs. Entity to Pick dropdown) as other issues which meant Entity Pick dropdown point back to this one. Anyway, I don't have the resources to devote to the Relation Type issue, nor do I see why one's business case would have several types with the same exact label, yet different machine names and allowed entities. Perhaps the issue creator could comment with a use-case so that the community can come up with a good solution to his dilemma.

@steveoliver - I don't think your patch changes worked because your test case changes to the $node1_label broke the last line $this->assertRaw(l($node1_label, $node1_uri['path'], $node1_uri['options']), 'Node1 link found');which expected $node1_label to be just the previously declared entity_label('node', $this->node1); Same thing applies to the $node3_label variable. Keeping all your current changes and making those last two lines:

$this->assertRaw(l(entity_label('node', $this->node1), $node1_uri['path'], $node1_uri['options']), 'Node1 link found');
$this->assertRaw(l(entity_label('node', $this->node3), $node3_uri['path'], $node3_uri['options']), 'Node3 link found');

should probably do the trick.

I tried to change as little as possible with my patch, which is why I wrote it as I did, which also meant that other code that expected the entity option keys to be type:id still worked with type:id:bundle because that code would just ignore the extra :bundle. /shrug Possibly just over-thinking it.

I wrote my bundle label function because I would rather waste time creating an easy to use function than work with one or two lines of esoteric code that is difficult to remember or construct. Being the Drupal-newb that I am, I had to be told about those two lines of code rather than discovering a bundle label function on my own. I will be glad for Drupal 8 when many of these functions and esoteric lines of code can be replaced by class objects and their properties/methods. /old-fashioned soapbox

@Uncle,

All fair points.

To address @tsvenson's original request on it's own:

The use case seems a bit confusing to me too, but the argument for specificity at the relation type as opposed to the relation instance level seems valid and worth considering.

The attached patch adds Name and Description fields, with a db update to update existing relation types with new name and description field values derived from label.

Name and Description aren't implemented anywhere and don't show up in the fields after you've saved a new relation, but I updated the language and UI as best I could to address @tsvenson's feature request.

Gotta sign off, but just wanted to post this -- I think it'd probably be more help than hurt and could help any other UI work, which is hope for anyone attempting to efficiently use the Entity Collector Block. ;)

Relation++ :)

splitting tags, seems logical.

wait, sorry. that didn't make sense.

Re: the two things we're doing here (wondering if we should spin off another issue, but not doing it for now):

1. Add name and description fields.
2. Clarify entity drop-down

I'd actually like to see both happen.

@tsvenson,

Have a look at #21 and lemme know if you think this might help. Next steps would be to get it passing tests and implement Name and Description in helpful locations.

@Uncle,

Attached patch with your input from #20 should pass now. I wonder if key 'type:bundle:id' (as opposed to yours 'type:id:bundle') will fly... I think we're pretty close.

Re: the patch and #25 and discussion about the entity drop-down, please see separate issue #1710234: entity collector selector only shows bundle, not entity type and is thus ambiguous.

@tsvenson,

No problem -- just hadn't heard from you on this thread since the beginning.

When you do get time, apply the patch and make any notes on the UI for the Add Relation Type screen, then maybe point out where you'd like to see Name and Description used throughout the Relation UI?

Will be out this weekend, but back online next week.

Cheers,

-Steve

@tsvenson,

This is the UI change generated by the patch in #21:

Proposal:

• Add a "Forward predicate field", and rename the "Reverse label" to "Reverse predicate".
• Leave the label as-is. This is standard terminology across entity types, and would retain the nice UI described by tsvenson.
• Optionally add the description field as per steveoliver's patch, but use a textarea, to keep it inline with the content type settings, as in #31.
• Replace instances of the label being used in code with the new forward-predicate (e.g. in the natural language formatter).
• Create an update function that copies the label to the new forward predicate.

Another option might be to just remove the reverse label, and fix #1479346: Create a display formatter to display relation_endpoints with tokens, and allow relation predicates to be defined using fields and tokens. Not sure where that would happen though...

Stupid updates.

#1624456: Expose Label(s) to views. is postponed, waiting on an outcome for this issue.

@tsvenson: what about the proposal without the "another option" part? Would that suit?