Replace $term->name with $term->label()

Here is an initial version, am curious if the testbot will like it. I already ran all taxonomy and translation tests, but not the entire suite.

I had to skip all terms that are originating from taxonomy_get_tree(). This function does not return full Term objects, so the Term::label() method can not be used. I left the original $term->name for these.

Ref. #870528: taxonomy_get_parents(), taxonomy_get_children(), and taxonomy_get_tree() do not return a full term objects.

Just ran this test locally and it returns green. The testbot reported a missing table. Trying again.

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest317827cache_bootstrap' doesn't exist' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php:58

#2: 1616972-2-taxonomy-replace_term_name.patch queued for re-testing.

Cool, the test passes now. I'll unassign myself. It would be a good idea if someone who has good knowledge of the Taxonomy internals can have a look at this, to see if there is a solution for the functions that do not return full Term objects.

I'm working on it.

#8: 1616972-3-taxonomy-replace_term_name.patch queued for re-testing.

Converted all tests to $term->label()

Are you sure about using $term->label() as a parameter to taxonomy_term_load_multiple()? I have intentionally skipped all these since I thought it was the intention to use $term->label() only when outputting to the screen, as is done in the issue that is referenced in the summary: #1616962: Replace $node->title with $node->label().

@@ -1384,7 +1384,7 @@ class TaxonomyLoadMultipleUnitTest extends TaxonomyWebTestCase {
 
     // Create a single term and load it by name.
     $term = $this->createTerm($vocabulary);
-    $loaded_terms = taxonomy_term_load_multiple(array(), array('name' => $term->name));
+    $loaded_terms = taxonomy_term_load_multiple(array(), array('name' => $term->label()));
     $this->assertEqual(count($loaded_terms), 1, 'One term was loaded.');
     $loaded_term = reset($loaded_terms);
     $this->assertEqual($term->tid, $loaded_term->tid, 'Term loaded by name successfully.');

Yes the failing test is unrelated to this patch. Re-run!

#11: 1616972-4-taxonomy-replace_term_name.patch queued for re-testing.

#11: 1616972-4-taxonomy-replace_term_name.patch queued for re-testing.

Removed the changes around taxonomy_term_load_multiple(). I found a few more term names in forum and taxonomy module. I think tokens should be language aware and generated with the right language in context.

+++ b/core/modules/forum/forum.moduleundefined
@@ -791,7 +791,7 @@ function forum_forum_load($tid = NULL) {
+  $_forums = taxonomy_get_tree($vid, $tid, 0, TRUE);

Third parameter needs to be NULL.

Patch applies cleanly and does not contain any coding style issues. I did a search for any remaining $term->name that should be replaced but couldn't find any. However, we should only replace $term->name with $term->label() when we are referring to the term, not when we are explicitly working with the term name. More info is available in a comment by fago on the parallell issue: http://drupal.org/node/1616962#comment-6106630

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -692,7 +692,7 @@ function taxonomy_form_term($form, &$form_state, Term $term = NULL, Vocabulary $
-    '#default_value' => $term->name,
+    '#default_value' => $term->label(),

I think we should stick with the $term->name because we're changing the $term->name directly here.

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -861,7 +861,7 @@ function taxonomy_form_term_submit_build_taxonomy_term($form, &$form_state) {
-  $term->name = trim($term->name);
+  $term->name = trim($term->label());

I think we should stick with the $term->name because we're changing the $term->name directly here.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -628,7 +628,7 @@ function template_preprocess_taxonomy_term(&$variables) {
-  $variables['term_name'] = check_plain($term->name);
+  $variables['term_name'] = check_plain($term->label());

The term name in templates should probably stay the term name. If we change this, we should rename the variable to term_label.

+++ b/core/modules/forum/forum.moduleundefined
@@ -1101,7 +1101,7 @@ function template_preprocess_forum_list(&$variables) {
-    $variables['forums'][$id]->name = check_plain($forum->name);
+    $variables['forums'][$id]->name = check_plain($forum->label());

The term name in templates should probably stay the term name. If we change this, we should rename the variable to label.

A tiny change to the name of a t() argument, from %name to %label, since its value is now $term->label().

Created a follow-up on adding a token for $term->label() : #1632720: Create tokens for entity labels.

Rerolled after tests moved to PSR-0.

Just a few things I think might warrant changing:

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -861,7 +861,7 @@ function taxonomy_form_term_submit_build_taxonomy_term($form, &$form_state) {
@@ -899,12 +899,12 @@ function taxonomy_term_confirm_delete($form, &$form_state, $term) {

We might want to type-hint $term as Term, and document the params.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -1052,13 +1052,13 @@ function taxonomy_implode_tags($tags, $vid = NULL) {
-      if (isset($tag->name)) {

This check may not be sufficient, better check if $tag instanceof EntityInterface to make sure it supports the method.

Add the type-hinting to taxonomy_term_confirm_delete() and instanceof EntityInterface in taxonomy_implode_tags() to make sure that Term::label() exists.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1367,7 +1367,7 @@ function taxonomy_field_formatter_prepare_view($entity_type, $entities, $field,
  */
 function taxonomy_term_title(Term $term) {
-  return $term->name;
+  return $term->label();
 }

This function is used as a title callback for two menu router items. This is exactly the use case that we now have for entity_page_label(). Let's remove this function in a follow-up issue and use entity_page_label() instead.

Otherwise, this looks ready for me. Let's get it in.

working on this

fixxed the issues raised by fago

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -627,7 +627,7 @@ function template_preprocess_taxonomy_term(&$variables) {
   $variables['term_url']  = url($uri['path'], $uri['options']);
-  $variables['term_name'] = check_plain($term->label());
+  $variables['term_name'] = check_plain($term->name);

As discussed, this means that the term overview will still display the term name. That's wrong.

IMHO, we should rename the variable to term_label and use that. Same for various forum preprocess functions for both terms and nodes.

Fine with doing that in a follow-up, though.

did not mean to set to needs work.

Followup created in #1642070: Make use of entity labels in templates.

Discussed this Issue with @Webflo and @Gabor

We agree it would probably be pretty bad in memory if we load 100 terms (the default) on the taxonomy_overview_terms() page and also taxonomy_form_term (where it shows you all terms to select as a relation parent)
So we removed it from there.

But for the other places like forum and also hook_options_list() we should load the entities to have the right labels/names.

We should create a follow up for taxonomy_overview_terms() and taxonomy_form_term().

oh shit, the "16.42 KB" and the "2.6 KB" is the correct one... (so nr 3 & 4)

Actually, that's the most problematic one. By default, this would load the whole vocabulary anyway,

correct, but after how many entities to load we talk about performance issues or memory problems? Because we could hope that somebody which has a lot of terms uses autocomplete on their taxonomy term field. But yes this is only an assumption.

I tend to think that's the only way we can proceed for having a proper multilingual support for that. So we'd have the performance implications for that or we find other ways to improve it.

I also don't see another possibility, we could now start with making entities loading faster, but this would stop our progress on multilanguage....

Should we go back and use entity_load everywhere and make a followup for the speed?

Sorry, I have missed the performance issue here. I very much doubt that we can use entity_load() in taxonomy_get_tree(), not until we have some sort of lazy loading for entities, see #1237636: Entity lazy multiple front loading. Once we have an interface/pattern/implementation for that, we might be able to make this really nice and avoid having to load all fields when you just want to display the label() and stuff like that.

No sure what to do here until then. We could leave out these cases, create a follow-up issue and add a @todo to all affected locations. Similar to various places in the corresponding node label issue that adds @todo's for all places that load pseudo $node objects directly with db_query().

unassigning from me

#1616962: Replace $node->title with $node->label() is now in core, so let's get going again with this!

#39: 1616972-39-taxonomy-replace_term_name.patch queued for re-testing.

Re-rolled.

#49: 1616972-49-taxonomy-replace_term_name.patch queued for re-testing.

Re-roll.

#52: taxonomy-term-label-1616972-52.patch queued for re-testing.

will fix this in Munich :)

rerolling, should apply

here some performance analysis:

Admin Taxonomy List

- created 800 terms
- viewing term list (admin/structure/taxonomy/)
- "ab -n 20 -c 1" (with cookie for user1)

with $term->name (old version, no loading of entities)

Time taken for tests: 18.431 seconds
Time per request: 921.571 [ms] (mean)
Total Incl. MemUse (bytes): 8,930,184 bytes

with $term->label() (new version, loading of entities)

Time taken for tests: 22.156 seconds
Time per request: 1107.786 [ms] (mean)
Total Incl. MemUse (bytes): 10,842,576 bytes

Analysis

200ms slower (which is 20% of the overall page load)
Memory +2MB

Create Node with Taxonomy Field

100 Terms

- created 100 terms
- Added TermReference Field to Contenttype Page
- Creating Content (node/add/page)
- "ab -n 20 -c 1" (with cookie for user1)

with $term->name (old version, no loading of entities)

Time taken for tests: 3.689 seconds
Time per request: 184.438 [ms] (mean)

with $term->label() (new version, loading of entities)

Time taken for tests: 4.235 seconds
Time per request: 211.755 [ms] (mean)

Analysis

27ms slower (which is 14% slower of the overall page load)

800 Terms

- created 800 terms
- Added TermReference Field to Contenttype Article
- Creating Content (node/add/article)
- "ab -n 20 -c 1" (with cookie for user1)

with $term->name (old version, no loading of entities)

Time taken for tests: 5.273 seconds
Time per request: 263.629 [ms] (mean)
Total Incl. MemUse (bytes): 5,728,624 bytes

with $term->label() (new version, loading of entities)

Time taken for tests: 9.221 seconds
Time per request: 461.042 [ms] (mean)
Total Incl. MemUse (bytes): 6,855,104 bytes

Analysis

200ms slower (which is 75% of the overall page load)
Memory +1.1MB

Forum

12 Forums

- created 12 terms in forum vocabulary
- viewing forum frontpage (/forum)
- "ab -n 20 -c 1"

with $term->name (old version, no loading of entities)

Time taken for tests: 2.673 seconds
Time per request: 133.656 [ms] (mean)

with $term->label() (new version, loading of entities)

Time taken for tests: 2.718 seconds
Time per request: 135.884 [ms] (mean)

Analysis

2ms slower (which is 1% of the overall page load)

80 Forums

- created 80 terms in forum vocabulary
- viewing forum frontpage (/forum)
- "ab -n 20 -c 1"

with $term->name (old version, no loading of entities)

Time taken for tests: 5.054 seconds
Time per request: 252.681 [ms] (mean)

with $term->label() (new version, loading of entities)

Time taken for tests: 5.501 seconds
Time per request: 275.041 [ms] (mean)

Analysis

25ms slower (which is 9% of the overall page load)

160 Forums

- created 80 terms in forum vocabulary
- viewing forum frontpage (/forum)
- "ab -n 20 -c 1"

with $term->name (old version, no loading of entities)

Time taken for tests: 7.790 seconds
Time per request: 389.487 [ms] (mean)

with $term->label() (new version, loading of entities)

Time taken for tests: 8.784 seconds
Time per request: 439.175 [ms] (mean)

Analysis

50ms slower (which is 12% of the overall page load)

Thanks for the benchmarks... yikes! :)

I asked Schnitzel to look for a place we might be missing an entity_load_multiple() (rather than an entity_load()) as it could account for this happening.

My guess is that those performance differences are because of the taxonomy_get_tree() changes that now do a full entity load if the argument is set to TRUE. Not sure what to do about that.

@berdir

nope, then it should only be 200msec longer.
I checked with xhprof and for "Admin Taxonomy List" and "Create Node with Taxonomy Field" the function taxonomy_get_tree jumps from
30ms to 200ms

while the whole pageload is almost double for "Create Node with Taxonomy Field"

well actually it only jumps 200ms on top, sorry didn't yet had enough hungarian chocolate *g*

so the admin taxonomy list jumps from 921.571 [ms] to 1107.786 [ms]
while Create Node with Taxonomy Field jumps from 263.629 to 461.042 [ms]

so both 200ms, just the overall pagespeed is different

as discussed with webchick the performance regressions are as expected.
When a page loads 800 terms it is 200ms slower, if it loads 100 terms its 27ms slower. And this does not matter if on Forum, NodeAdd or Taxonomy List.

we also discussed if we keep the "$load_entities = FALSE" argument on taxonomy_get_tree(). To not mix too many things in this issue and there can be a case where not loading the entities is interesting (when you only need the TermID), we will not touch this here. So we will make a followup for this.

Attached is a patch which also uses entities for taxonomy term list overview.

Assigning to catch as per discussion with @webchick following @catch's approval for the assignment itself :)

So viewing a node we already load full entities in taxonomy_field_formatter_prepare_view(), there should only be a very small hit for the extra method call there.

The node/add and admin screen regressions are very unfortunate, we already went through this with #870528: taxonomy_get_parents(), taxonomy_get_children(), and taxonomy_get_tree() do not return a full term objects then made the full objects optional for the same reason.

I'd be tempted to commit this with the known regression, but then upgrade #556842: taxonomy_get_tree() memory issues and/or #30993: Scaleable/themeable taxonomy selection widget to try to fix/mitigate the taxonomy_get_tree() issues we're making worse here?

All right, thanks for the review. Let's do that then :)

I was thinking along the suggestions from @catch in #64 as well, fine with me.

Can someone open a follow-up to fix the term/forum templates? Those are right now still using $term->name directly, we need to fix that.

@berdir

There is already a followup from the node->label() discussion:
#1642070: Make use of entity labels in templates

Ok, now that I'm fairly sure that catch won't kill me if I commit this... ;)

Committed and pushed to 8.x. Thanks!

This needs a change notice, so escalating. Also, #556842: taxonomy_get_tree() memory issues seems to already be a critical so I think we're good there. #30993: Scaleable/themeable taxonomy selection widget is still listed as a "major feature request" but I think that's appropriate, given that 800 terms in a drop-down is an edge case, and likely anyone hitting that can install hierarchical_select module or prepopulate or something else to work around it.

Change notice at http://drupal.org/node/1739820

Follow-up bug caused by this issue: #1754070: Test coverage for Editing and re-saving a node with tagged taxonomy terms removes terms from node

Done, off sprint. Thanks all!