Posted by fago on June 4, 2012 at 10:36pm
20 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | taxonomy.module |
| Category: | task |
| Priority: | normal |
| Assigned: | catch |
| Status: | closed (fixed) |
| Issue tags: | D8MI, Entity system |
Issue Summary
Similar to #1616962: Replace $node->title with $node->label(), let's make use of the label() method of terms when linking to it.
In conjunction with #1616952: Add a langcode parameter to EntityInterface::label(), this is going to help make the term name appear properly translated. Also it brings in possible alterability of it via changing the label key/property.
Comments
#1
#2
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.
#3
The last submitted patch, 1616972-2-taxonomy-replace_term_name.patch, failed testing.
#4
Just ran this test locally and it returns green. The testbot reported a missing table. Trying again.
#5
#2: 1616972-2-taxonomy-replace_term_name.patch queued for re-testing.
#6
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.
#7
I'm working on it.
#8
#9
The last submitted patch, 1616972-3-taxonomy-replace_term_name.patch, failed testing.
#10
#8: 1616972-3-taxonomy-replace_term_name.patch queued for re-testing.
#11
Converted all tests to $term->label()
#12
The last submitted patch, 1616972-4-taxonomy-replace_term_name.patch, failed testing.
#13
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.');
#14
Yes the failing test is unrelated to this patch. Re-run!
#15
#11: 1616972-4-taxonomy-replace_term_name.patch queued for re-testing.
#16
The last submitted patch, 1616972-4-taxonomy-replace_term_name.patch, failed testing.
#17
#11: 1616972-4-taxonomy-replace_term_name.patch queued for re-testing.
#18
Yep, places where we edit or query for the name should stay with $term->name. $term->label() could be altered to point to something else, what would make this break. The same way, I think we should stay with the name in term-view templates and token replacements for now as both specifically refer to using the name.
Also see http://drupal.org/node/1616962#comment-6106630 for some examples.
#19
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.
#20
The last submitted patch, 1616972-5-taxonomy-replace_term_name.patch, failed testing.
#21
True, but we have language aware getters for that (which won't work for properties yet).
#22
+++ 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.
#23
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.
#24
A tiny change to the name of a t() argument, from %name to %label, since its value is now $term->label().
#25
Created a follow-up on adding a token for $term->label() : #1632720: Create tokens for entity labels.
#26
#27
The last submitted patch, 1616972-24-taxonomy-replace_term_name.patch, failed testing.
#28
Rerolled after tests moved to PSR-0.
#29
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.
#30
Add the type-hinting to taxonomy_term_confirm_delete() and instanceof EntityInterface in taxonomy_implode_tags() to make sure that Term::label() exists.
#31
+++ 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.
#32
+++ b/core/modules/taxonomy/taxonomy.admin.inc@@ -692,7 +692,7 @@ function taxonomy_form_term($form, &$form_state, Term $term = NULL, Vocabulary $
$form['name'] = array(
'#type' => 'textfield',
'#title' => t('Name'),
- '#default_value' => $term->name,
That would break things once the label is not pointing to the name anymore!
+++ b/core/modules/taxonomy/taxonomy.admin.inc@@ -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());
Looks wrong.
+++ b/core/modules/taxonomy/taxonomy.admin.inc@@ -892,19 +892,19 @@ function taxonomy_form_term_delete_submit($form, &$form_state) {
- $form['name'] = array('#type' => 'value', '#value' => $term->name);
+ $form['name'] = array('#type' => 'value', '#value' => $term->label());
What is the name used for? Then, if it's the label it should be called that way.
+++ b/core/modules/taxonomy/taxonomy.module@@ -627,7 +627,7 @@ function template_preprocess_taxonomy_term(&$variables) {
- $variables['term_name'] = check_plain($term->name);
+ $variables['term_name'] = check_plain($term->label());
Again, that variable is supposed to contain the term name. So it would be rather confusing it does not once the label points to something else. That said, I think we should leave templates alone for now. Converting them could be a separate issue, e.g. if the variable holds the label it should be called that way.
#33
working on this
#34
fixxed the issues raised by fago
#35
+++ 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.
#36
did not mean to set to needs work.
#37
Followup created in #1642070: Make use of entity labels in templates.
#38
+++ b/core/modules/field/modules/options/options.api.php@@ -61,10 +61,10 @@ function hook_options_list($field, $instance, $entity_type, $entity) {
- $terms = taxonomy_get_tree($tree['vid'], $tree['parent']);
+ $terms = taxonomy_get_tree($tree['vid'], $tree['parent'], NULL, TRUE);
if ($terms) {
foreach ($terms as $term) {
- $options[$term->tid] = str_repeat('-', $term->depth) . $term->name;
+ $options[$term->tid] = str_repeat('-', $term->depth) . $term->label();
Sry for missing that earlier, but we cannot switch to entity_load() here. That not-entity-load version of taxonomy_get_tree() is need in order to avoid memory issues in cases like this one. There is an issue somewhere which did that because of memory issues + performance penalties with entity_load() due to all the fields being loaded as well. So I don't think we can change that here now. :/
However, that feels very odd. Just being able to use $term->label() partly because of that would make it quite random where it's used and where not. My feeling is that we have to convert to entities at some point and solve the performance problems somehow differently...?
+++ b/core/modules/forum/forum.admin.inc@@ -327,7 +327,7 @@ function _forum_parent_select($tid, $title, $child_type) {
- $children = taxonomy_get_tree($vid, $tid);
+ $children = taxonomy_get_tree($vid, $tid, NULL, TRUE);
Same here.
+++ b/core/modules/forum/forum.admin.inc@@ -335,12 +335,12 @@ function _forum_parent_select($tid, $title, $child_type) {
- $tree = taxonomy_get_tree($vid);
+ $tree = taxonomy_get_tree($vid, 0, NULL, TRUE);
and here.
+++ b/core/modules/forum/forum.module@@ -793,7 +793,7 @@ function forum_forum_load($tid = NULL) {
- $_forums = taxonomy_get_tree($vid, $tid);
+ $_forums = taxonomy_get_tree($vid, $tid, NULL, TRUE);
and here.
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermStorageController.php@@ -70,7 +70,7 @@ class TermStorageController extends DatabaseStorageController {
+ if (isset($conditions['name']) && drupal_strtolower($conditions['name'] != drupal_strtolower($term->label()))) {
Another glitch, this compares labels to names.
+++ b/core/modules/taxonomy/taxonomy.admin.inc@@ -281,7 +281,7 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
- $tree = taxonomy_get_tree($vocabulary->vid);
+ $tree = taxonomy_get_tree($vocabulary->vid, 0, NULL, TRUE);
Again entity load issue.
+++ b/core/modules/taxonomy/taxonomy.admin.inc@@ -734,14 +734,14 @@ function taxonomy_form_term($form, &$form_state, Term $term = NULL, Vocabulary $
- $tree = taxonomy_get_tree($vocabulary->vid);
+ $tree = taxonomy_get_tree($vocabulary->vid, 0, NULL, TRUE);
again.
+++ b/core/modules/taxonomy/taxonomy.module@@ -1297,9 +1297,9 @@ function taxonomy_allowed_values($field, $instance, $entity_type, $entity) {
- if ($terms = taxonomy_get_tree($vocabulary->vid, $tree['parent'])) {
+ if ($terms = taxonomy_get_tree($vocabulary->vid, $tree['parent'], NULL, TRUE)) {
again.
#39
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().
#40
oh shit, the "16.42 KB" and the "2.6 KB" is the correct one... (so nr 3 & 4)
#41
Actually, that's the most problematic one. By default, this would load the whole vocabulary anyway, so if that blows up your site it will blow up your edit form. If it doesn't blow up, the performance decrease would be more relevant on the node edit form than on the taxonomy admin page as well.
Thus, once we start with going with entities there, we can do it everywhere. 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.
#42
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 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?
#43
see #556842: taxonomy_get_tree() memory issues.
I'd like to have catch to weigh on this as he is both, taxonomy and performance expert.
#44
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().
#45
unassigning from me
#46
#1616962: Replace $node->title with $node->label() is now in core, so let's get going again with this!
#47
#39: 1616972-39-taxonomy-replace_term_name.patch queued for re-testing.
#48
The last submitted patch, 1616972-39-taxonomy-replace_term_name.patch, failed testing.
#49
Re-rolled.
#50
#49: 1616972-49-taxonomy-replace_term_name.patch queued for re-testing.
#51
The last submitted patch, 1616972-49-taxonomy-replace_term_name.patch, failed testing.
#52
Re-roll.
#53
#52: taxonomy-term-label-1616972-52.patch queued for re-testing.
#54
The last submitted patch, taxonomy-term-label-1616972-52.patch, failed testing.
#55
will fix this in Munich :)
#56
rerolling, should apply
#57
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)
#58
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.
#59
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.
#60
@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"
#61
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
#62
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.
#63
Assigning to catch as per discussion with @webchick following @catch's approval for the assignment itself :)
#64
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?
#65
All right, thanks for the review. Let's do that then :)
#66
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.
#67
@berdir
There is already a followup from the node->label() discussion:
#1642070: Make use of entity labels in templates
#68
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.
#69
Change notice at http://drupal.org/node/1739820
#70
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
#71
Done, off sprint. Thanks all!
#72
Automatically closed -- issue fixed for 2 weeks with no activity.