Problem/Motivation

The entity system in Drupal 7 provides a mechanism to handle all data structures using unified hooks. While this concept was applied to the node system, it was not fully applied to the taxonomy system thus several expected hooks do not exist.

Proposed resolution

Trigger hook_entity_view and hook_taxonomy_term_view during the display execution for taxonomy term pages.

Remaining tasks

Selection of an appropriate implementation for Drupal 7 that reduces the chance of introducing compatibility problems.

User interface changes

None.

API changes

Both hook_entity_view and hook_taxonomy_term_view would trigger during the page load of a taxonomy term page.

Original report by Dave Reid

We are missing important calls to hook_entity_view() and there is no hook_taxonomy_term_view() unlike all other entities so it makes it hard for an entity-general module (like the Meta tags module) to be able to simply use hook_entity_view() for it's code rather than having to maintain 15 different view hooks.

Files: 
CommentFileSizeAuthor
#177 taxonomy-view-documentation-fixes-1067120-176.patch1.51 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,426 pass(es).
[ View ]
#166 taxonomy-term-view-followup-1067120-166.patch5.53 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 39,623 pass(es).
[ View ]
#160 taxonomy-term-view-followup-1067120-160.patch4 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 39,676 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#159 drupal-7.16-term-page.png139.82 KBDavid_Rothstein
#159 drupal-7.17-term-page.png132.81 KBDavid_Rothstein
#145 drupal-n1067120-145-d7.patch11.4 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 39,445 pass(es).
[ View ]
#143 drupal-n1067120-143-d7.patch11.44 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 39,445 pass(es).
[ View ]
#135 drupal-n1067120-135-d7.patch10.25 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 39,402 pass(es).
[ View ]
#134 drupal-n1067120-134-d7.patch10.25 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 39,419 pass(es).
[ View ]
#132 drupal-n1067120-132.patch10.29 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 39,383 pass(es).
[ View ]
#114 1067120-114.patch10.9 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 40,696 pass(es).
[ View ]
#109 drupal-n1067120-109.patch10.64 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-n1067120-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#105 drupal-n1067120-105.patch10.8 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 40,333 pass(es).
[ View ]
#100 1067120-100.patch10.87 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 40,256 pass(es).
[ View ]
#97 1067120-97.patch10.87 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] 40,250 pass(es), 3 fail(s), and 16 exception(s).
[ View ]
#96 0001-Issue-1067120-Missing-hook_taxonomy_term_view-hook_e.patch11.2 KBfgm
PASSED: [[SimpleTest]]: [MySQL] 40,252 pass(es).
[ View ]
#94 1067120-94.patch10.23 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 40,254 pass(es).
[ View ]
#92 1067120-92.patch10.19 KBcorvus_ch
PASSED: [[SimpleTest]]: [MySQL] 40,254 pass(es).
[ View ]
#86 drupal-n1067120-86-taxonomy_term_view.patch10.44 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 37,216 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#84 drupal-n1067120-84-taxonomy_term_view.patch10.42 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 37,212 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#82 drupal-n1067120-82-taxonomy_term_view.patch10.43 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 37,217 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#73 taxonomy-term-build-extension-1067120-73-d7.patch10.3 KBmrfelton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-term-build-extension-1067120-73-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#65 1067120_65.patch10.37 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1067120_65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#60 1067120_60.patch10.09 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,020 pass(es).
[ View ]
#56 1067120_56.patch10.01 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 33,634 pass(es).
[ View ]
#47 1067120_47.patch9.96 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1067120_47.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#45 1067120_45.patch9.96 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 33,077 pass(es).
[ View ]
#44 1067120_44.patch9.99 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 33,017 pass(es), 43 fail(s), and 24 exception(es).
[ View ]
#41 1067120_41.patch3.78 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 32,963 pass(es).
[ View ]
#40 1067120_40.patch3.78 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 32,949 pass(es), 0 fail(s), and 2 exception(es).
[ View ]
#39 1067120_39.patch3.74 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 32,960 pass(es), 0 fail(s), and 2 exception(es).
[ View ]
#39 1067120_testonly.patch2.53 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 32,961 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#34 taxonomy_term_build_extension_1067120_d8_34.patch8.84 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 32,997 pass(es).
[ View ]
#33 taxonomy_term_build_extension_1067120_d8_33.patch8.86 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 33,576 pass(es).
[ View ]
#30 taxonomy_term_build_extension_1067120_d8_30.patch8.83 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 33,592 pass(es), 0 fail(s), and 8 exception(es).
[ View ]
#28 taxonomy_term_build_extension_1067120_d8_28.patch8.83 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 32,183 pass(es).
[ View ]
#27 taxonomy_term_build_extension_1067120_d8_27.patch9.54 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 32,178 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#24 taxonomy_term_build_extension_1067120_d8_25.patch9.48 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 32,187 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#14 taxonomy_hook_entity_view-1067120-2.patch1.28 KBvalthebald
PASSED: [[SimpleTest]]: [MySQL] 29,407 pass(es).
[ View ]
#12 taxonomy_hook_entity_view-1067120-2.patch1.28 KBvalthebald
PASSED: [[SimpleTest]]: [MySQL] 29,405 pass(es).
[ View ]
#9 taxonomy_hook_entity_view-1067120-1.patch1.28 KBvalthebald
PASSED: [[SimpleTest]]: [MySQL] 29,402 pass(es).
[ View ]
#2 taxonomy_hook_entity_view-1067120.patch788 bytesvalthebald
PASSED: [[SimpleTest]]: [MySQL] 29,397 pass(es).
[ View ]

Comments

Dave Reid’s picture

Issue tags:+Metatags

This affects the Metatags module for D7.

valthebald’s picture

Status:Active» Needs review
Issue tags:-Metatags
StatusFileSize
new788 bytes
PASSED: [[SimpleTest]]: [MySQL] 29,397 pass(es).
[ View ]

Attached should solve the problem (added module_invoke_all('entity_view') to taxonomy_term_page)

Dave Reid’s picture

Status:Needs review» Needs work

We need to actually store that return data and have it be output on the page... and we also need to call hook_entity_view() as well.

Dave Reid’s picture

Issue tags:+Metatags
Dave Reid’s picture

Issue tags:+needs backport to D7
valthebald’s picture

Issue tags:-needs backport to D7

Where do you think it's necessary to store hook result? Module additions should be stored in $entity->content:

The module may add elements to $entity->content prior to rendering. The structure of $entity->content is a renderable array as expected by drupal_render().

(http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...)

About output - it is possible to rely on current check if (!empty($term->description)) or replace it with other condition (but which one - no idea)

valthebald’s picture

Issue tags:+needs backport to D7
Dave Reid’s picture

Issue tags:-needs backport to D7

Gah, that's true. I had a plan to make this more like node views, and add a taxonomy_term_build_content() function that handles all the hook and page output.

valthebald’s picture

Issue tags:+needs backport to D7
StatusFileSize
new1.28 KB
PASSED: [[SimpleTest]]: [MySQL] 29,402 pass(es).
[ View ]

I suggest simple change in logic - if modules have filled $term->content, call taxonomy_term_view (see patch)

Dave Reid’s picture

Note that #796692: Only show the term heading if the term has a description will likely help us part of the way by allowing hook_entity_view_alter() and hook_taxonomy_term_view_alter() to be executed.

Damien Tournoud’s picture

<?php
// Allow modules to make their own additions to the node.
module_invoke_all('entity_view', $term, 'taxonomy_term', 'full', $GLOBALS['language_content']->language);
?>

To the *term*, I guess? Also, are we consistent in invoking hook_entity_view() for all core entities?

valthebald’s picture

StatusFileSize
new1.28 KB
PASSED: [[SimpleTest]]: [MySQL] 29,405 pass(es).
[ View ]

Of course, *term*
Regarding consistency - isn't it a little bit outside the scope of this specific issue?
In any case, addition of hook_entity_view call makes taxonomy_term_page more consistent than now.
If you ask my opinion, better consistency can be achieved if hook_node_view and hook_user_view are discarded in favor of unified hook_entity_view, which would be called in every _view function.

valthebald’s picture

By the way, how can I cancel original patch and replace it with the last one?

valthebald’s picture

Status:Needs work» Needs review
StatusFileSize
new1.28 KB
PASSED: [[SimpleTest]]: [MySQL] 29,407 pass(es).
[ View ]
andypost’s picture

subscribe

valthebald’s picture

What's the process now? What steps are needed to move this issue to another status (rtbc=>patch to be ported)?

BTMash’s picture

Status:Needs review» Needs work

I'm following up in here based on my issue (#1187810: Taxonomy module should also invoke hook_entity_view on taxonomy view - wish I'd done a better search ^_^), I was expecting what you have in taxonomy_term_page to be in function taxonomy_term_view(). I guess I'm imagining if in the event there are other display modes for the taxonomy (to be used in views / whatnot) that this behavior would allow for those build modes to also run the currently missing hooks. But maybe I'm missing something that makes it difficult to have this in taxonomy_term_view()?

mparker17’s picture

Subscribe

valthebald’s picture

#17: This issue is something different. taxonomy_term_page (page callback) does not call taxonomy_term_view(), if term description is empty. So it would not help if you invoke hook_entity_view in taxonomy_term_view.

BTMash’s picture

Based on the name of the issue, my assumption was this tackled any context of when the term is being viewed (in the different display modes that can be created for the term which means pulling up the viewing of a taxonomy term via a module like views on another viewing of a taxonomy page). In my scenario, the use case is that the term page is used to display upcoming events that have that term and I have an archive view page that displays the taxonomy term in another view mode (the different view mode displays all past events). Having hook_entity_view only in taxonomy_term_page does not allow for other modules to add in their content based on (the lack of) such modes.

Also,taxonomy_term_page is called on by taxonomy_term_page (atleast what I see via drupalcode.org - line 35 onwards of http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/modules/ta...).

valthebald’s picture

BTMash’s picture

Looking at what is in the head of the drupal 7.x and the 8.x branches, the if conditional from that issue in taxonomy_term_page is no longer there. And again, it discounts what happens on other view modes. They would not necessarily call taxonomy_term_page_view for hook_entity_view to run - they would be calling taxonomy_term_view. To me, it seems like taxonomy_term_view should be behaving in the same manner as the node and user modules.

BTMash’s picture

I'm not sure if I've been going off on a tangent but I decided to test it with a case that I know hasn't worked - with any module that implements hook_field_extra_fields(). I've decided to test out if the patch works by creating a module that implements it and used devel generate to generate the nodes and terms for this. And I implemented hook_entity_view() to display some stub content. The content shows up fine on a node view, comment view but not for a taxonomy view (I couldn't test out the user view though from looking at its code and how it is set up similar to nodes and viewing them, it should work as well).

The code I used to test this is at https://github.com/btmash/Taxonomy-Testing (would the proper protocol be to have the code in a sandbox on d.o?) since my simpletest writing skills are poor.

BTMash’s picture

Status:Needs work» Needs review
StatusFileSize
new9.48 KB
FAILED: [[SimpleTest]]: [MySQL] 32,187 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Ok, I'm giving this a shot with some other stuff added in. Namely turning the nodes that are shown into an extra field that can be set where to display within the set of fields on the page. It seems to work with the taxonomy testing module above and the items get reordered as necessary. It needs a thorough review, however.

Status:Needs review» Needs work
Issue tags:-Entity system, -Metatags, -entity API, -entity cleanup, -needs backport to D7

The last submitted patch, taxonomy_term_build_extension_1067120_d8_25.patch, failed testing.

BTMash’s picture

Status:Needs work» Needs review
Issue tags:+Entity system, +Metatags, +entity API, +entity cleanup, +needs backport to D7
BTMash’s picture

StatusFileSize
new9.54 KB
FAILED: [[SimpleTest]]: [MySQL] 32,178 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Some small cleanup of what was in there.

BTMash’s picture

StatusFileSize
new8.83 KB
PASSED: [[SimpleTest]]: [MySQL] 32,183 pass(es).
[ View ]

It seems like the test failed due to me trying to set drupal_set_title(). Removed that part along with some more unnecessary code.

redndahead’s picture

Status:Needs review» Needs work

Here are some nitpicks. Didn't really review the functionality.

+++ b/modules/taxonomy/taxonomy.module
@@ -174,6 +174,11 @@ function taxonomy_field_extra_fields() {
+        'taxonomy_term_nodes' => array(
+          'label' => t('Tagged nodes'),
+          'description' => t('All nodes that have been tagged with term'),
+          'weight' => 5,
+        )

Add comma after the closing ")"

+++ b/modules/taxonomy/taxonomy.module
@@ -687,6 +692,126 @@ function taxonomy_term_delete($tid) {
+      '#weight' => $fields['description']['weight'],
+      '#prefix' => '<div class="taxonomy-term-description">',
+      '#suffix' => '</div>',
+    );
+  }
+  ¶
+  if (isset($fields['taxonomy_term_nodes']) && $fields['taxonomy_term_nodes']['visible']) {

Remove Extra Spacing

+++ b/modules/taxonomy/taxonomy.module
@@ -687,6 +692,126 @@ function taxonomy_term_delete($tid) {
+  entity_prepare_view('taxonomy_term', array($term->tid => $term), $langcode);
+  $term->content += field_attach_view('taxonomy_term', $term, $view_mode, $langcode);
+  ¶
+  // Allow modules to make their own additions to the taxonomy term.
+  module_invoke_all('taxonomy_term_view', $term, $view_mode, $langcode);

Remove extra spacing

+++ b/modules/taxonomy/taxonomy.module
@@ -705,34 +830,27 @@ function taxonomy_term_view($term, $view_mode = 'full', $langcode = NULL) {
-
-  // Allow modules to modify the structured term.
+  ¶
+  // Allow modules to modify the structured node.
+  ¶
   $type = 'taxonomy_term';
   drupal_alter(array('taxonomy_term_view', 'entity_view'), $build, $type);
-
+  ¶
   return $build;
}

Remove extra spacing

1 days to next Drupal core point release.

BTMash’s picture

StatusFileSize
new8.83 KB
FAILED: [[SimpleTest]]: [MySQL] 33,592 pass(es), 0 fail(s), and 8 exception(es).
[ View ]

Updated patch with cleanup of spacing.

BTMash’s picture

Status:Needs work» Needs review

Ack...forgot to mark as needs review.

Status:Needs review» Needs work

The last submitted patch, taxonomy_term_build_extension_1067120_d8_30.patch, failed testing.

BTMash’s picture

Status:Needs work» Needs review
StatusFileSize
new8.86 KB
PASSED: [[SimpleTest]]: [MySQL] 33,576 pass(es).
[ View ]

Patch updated to create a build variable to pass on into drupal_render.

BTMash’s picture

StatusFileSize
new8.84 KB
PASSED: [[SimpleTest]]: [MySQL] 32,997 pass(es).
[ View ]

Gah...I forgot the one line with extra whitespace. Let's try again...

BTMash’s picture

BTMash’s picture

BTMash’s picture

Given the patch has passed automated testing, would someone else be able to review the patch?

Dave Reid’s picture

Thinking about this... that maybe we need to abstract 'display nodes tagged with this term' out of these API functions as terms can be attached to anything and not just nodes. We won't be able to use these functions when displaying forum terms. If the functions just displayed the term description and called the hooks then it could be.

BTMash’s picture

Issue tags:+Needs tests
StatusFileSize
new2.53 KB
FAILED: [[SimpleTest]]: [MySQL] 32,961 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
new3.74 KB
FAILED: [[SimpleTest]]: [MySQL] 32,960 pass(es), 0 fail(s), and 2 exception(es).
[ View ]

Hmm...I would love to modify the patch in #34 further so we have the things @Dave Reid mentions. However, I do not know what the scope of this patch is supposed to be and more importantly, I will need some guidance with getting it working as intended.. I modified valthebald's code so we have something else up and running. HOWEVER, I have expanded on TaxonomyHooksTestCase basic test that will test implementation of hook_taxonomy_term_view (and hook_entity_view) on a page view. Getting this further along would be beneficial as well but atleast we have something working on a core basis (and we can now see if the test fails on the page view to call the hooks).

BTMash’s picture

StatusFileSize
new3.78 KB
FAILED: [[SimpleTest]]: [MySQL] 32,949 pass(es), 0 fail(s), and 2 exception(es).
[ View ]

Ack...whitespace errors.

BTMash’s picture

StatusFileSize
new3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 32,963 pass(es).
[ View ]

Woops, entity view needs to use $entity, not $term.

BTMash’s picture

Status:Needs review» Needs work

In talking with @Dave Reid on irc, he recommended I modifying the previous patch (but not include the display nodes portion as an extra field but as something added to a normal page). Setting to needs work so I remember to tackle it.

BTMash’s picture

Status:Needs work» Needs review
BTMash’s picture

StatusFileSize
new9.99 KB
FAILED: [[SimpleTest]]: [MySQL] 33,017 pass(es), 43 fail(s), and 24 exception(es).
[ View ]

Ok, the new patch is based off the work done in #34 except that the node retrieval is now only limited to the term page view. The other build modes won't have it available as a field to add (in its current state, it is limited to nodes which would make sense on why not to do so; should likely be a separate issue). Tests added in and things seem to pass.

BTMash’s picture

StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 33,077 pass(es).
[ View ]

Ack...some whitespace issues along with a call on devel. So minor cleanup.

andypost’s picture

Status:Needs review» Needs work
+++ b/modules/simpletest/tests/taxonomy_test.moduleundefined
@@ -55,6 +55,34 @@ function taxonomy_test_taxonomy_term_delete($term) {
/**
+ * Implements hook_taxonomy_term_view().
+ */
+function taxonomy_test_taxonomy_term_view($term, $view_mode, $langcode) {
+  if ($view_mode == 'full') {
+    $term->content['taxonomy_test_term_view_check'] = array(
+      '#prefix' => '<div>',
+      '#markup' => t('The antonym is %antonym', array('%antonym' => $term->antonym)),
+      '#suffix' => '</div>',
+      '#weight' => 10,
+    );
+  }
+}
+
+/**
+ * Implements hook_taxonomy_term_view().
+ */
+function taxonomy_test_entity_view($entity, $type, $view_mode, $langcode) {
+  if ($type == 'taxonomy_term' && $view_mode == 'full') {
+    $entity->content['taxonomy_test_entity_view_check'] = array(
+      '#prefix' => '<div>',
+      '#markup' => t('The antonym is %antonym', array('%antonym' => $entity->antonym)),
+      '#suffix' => '</div>',
+      '#weight' => 20,
+    );
+  }
+}

php-doc is the same

+++ b/modules/taxonomy/taxonomy.moduleundefined
@@ -705,6 +705,103 @@ function taxonomy_term_delete($tid) {
/**
+ * Generate an array which displays a term detail page.
+ *
+ * @param term
+ *   A taxonomy term object.
+ * @return
+ *   A $page element suitable for use by drupal_page_render().
+ */
+function taxonomy_term_show($term) {
+  return taxonomy_term_view_multiple(array($term->tid => $term), 'full');
+}

What this wrapper for?

7 days to next Drupal core point release.

BTMash’s picture

StatusFileSize
new9.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1067120_47.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Talking about this on irc, the latter is to try and be on the same playing field as node_view is on (and go through the same steps). Comment seems to go in a similar route so going with the behavior of the majority of the core entities (could be useful in other ways down the line?).

Attaching patch with updated php-doc.

BTMash’s picture

Status:Needs work» Needs review

Ack...forgot to set to 'needs review'

christefano’s picture

I didn't review the functionality but this conforms to coding standards.

jdolan’s picture

Subscribing.

Is this something we should expect in an upcoming D7 point release, or is it a ways out? I'm particularly interested in having hook_entity_view implemented for Taxonomy terms.

BTMash’s picture

@jdolan, This will first need to land in D8 (and hopefully, the patch is in a state that it can be backported to D7). Further review would certainly help speed up the process towards getting it in core :)

karlos007’s picture

just subscribing from #1111222: How to use it? (what a nice id :) )

Dave Reid’s picture

BTMash’s picture

#47: 1067120_47.patch queued for re-testing since I am not sure if the patch needs to get rerolled.

Status:Needs review» Needs work

The last submitted patch, 1067120_47.patch, failed testing.

BTMash’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +Entity system, +Metatags, +entity API, +entity cleanup, +needs backport to D7, +Contributed project blocker
StatusFileSize
new10.01 KB
PASSED: [[SimpleTest]]: [MySQL] 33,634 pass(es).
[ View ]

Try the rerolled patch.

BTMash’s picture

Status:Needs work» Needs review

Mark as needs review.

xjm’s picture

Status:Needs review» Needs work
Issue tags:+Novice
+++ b/modules/taxonomy/taxonomy.moduleundefined
@@ -705,6 +705,103 @@ function taxonomy_term_delete($tid) {
+ * Generate an array which displays a term detail page.
...
+ * Construct a drupal_render() style array from an array of loaded terms.

Very minor point, but these should begin "Generates" and "Constructs."

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades. When you reroll, please incorporate the minor comment corrections above.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

BTMash’s picture

Assigned:Dave Reid» BTMash

Assigning the patch to myself since no one else has taken on it :) Will unassign once I reroll it and add your suggested changes.

BTMash’s picture

Status:Needs work» Needs review
StatusFileSize
new10.09 KB
PASSED: [[SimpleTest]]: [MySQL] 34,020 pass(es).
[ View ]

Ok, patch has been rerolled for the core changeover and with comments from @xjm.

BTMash’s picture

Assigned:BTMash» Unassigned

Forgot to unassign after adding patch.

BTMash’s picture

#60: 1067120_60.patch queued for re-testing.

xjm’s picture

Alright, I haven't done a complete review yet, but here are a few things I noticed so far:

+++ b/core/modules/simpletest/tests/taxonomy_test.moduleundefined
@@ -55,6 +55,34 @@ function taxonomy_test_taxonomy_term_delete($term) {
/**
+ * Implements hook_taxonomy_term_view().
+ */
...
+/**
+ * Implements hook_entity_view().
+ */

Maybe we should add @see to the test cases these support, and/or a brief decription of their purpose? The reverse might also be a good idea (adding @see to these in the test method).

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -17,6 +17,11 @@ function taxonomy_term_page($term) {
+  // If there is a menu link to this term, the link becomes the last part
+  // of the active trail, and the link name becomes the page title.
+  // Thus, we must explicitly set the page title to be the node title.
+  $uri = entity_uri('taxonomy_term', $term);

This is a good comment; however, it confused me at first because the line that follows is not obviously related to the comment. Can we move these lines (comment and definition of $uri both) down to just above the lines that are added later?

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -939,6 +940,12 @@ class TaxonomyHooksTestCase extends TaxonomyWebTestCase {
+    // View the term

This comment needs a period. Also, I'd change it to:
// View the term and ensure that hook_taxonomy_term_view() and hook_entity_view() are invoked.

+++ b/core/modules/taxonomy/taxonomy.testundefined
@@ -939,6 +940,12 @@ class TaxonomyHooksTestCase extends TaxonomyWebTestCase {
+    $this->assertFalse(empty($term_build['taxonomy_terms'][$term->tid]['taxonomy_test_term_view_check']), t('Antonym was success loaded for viewing for hook_taxonomy_term_view.'));
+    $this->assertFalse(empty($term_build['taxonomy_terms'][$term->tid]['taxonomy_test_entity_view_check']), t('Antonym was success loaded for viewing for hook_entity_term_view.'));

The assertion text here isn't quite grammatically correct. Maybe all we need is:

hook_foo() was invoked when viewing the term.

Also, note that parens should be used after the hook name. Finally, we probably do not need to use t() for assertion messages. See #500866: [META] remove t() from assert message.

I'm also a bit hesitant about the function taxonomy_term_show(). Do we need this factored into its own function? Is that the best name for it? Should it have more documentation as to what its purpose is? Etc.

BTMash’s picture

Thanks for the review. I'll figure out how to make the changes you've recommended.

As for the function taxonomy_term_show, the reason I felt this needed to be factored into its own function was because I wanted it to be similar to how node works (node_page_view -> node_show -> node_view_multiple -> node_view) and hence we have the same style of naming convention.

BTMash’s picture

StatusFileSize
new10.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1067120_65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, I've attempted to make the changes you recommended. I wasn't entirely sure on how to write out the @see references (I figured it made more sense at the top of the file/class where it was being used. I've kept taxonomy_term_show as it is for now, however.

BTMash’s picture

BTMash’s picture

sun’s picture

Issue tags:-entity API, -entity cleanup

Standardizing on "entity" tag, which will be renamed to "Entity system".

BTMash’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +Entity system, +Metatags, +Novice, +needs backport to D7, +Contributed project blocker

The last submitted patch, 1067120_65.patch, failed testing.

BTMash’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests

I'm removing the 'needs tests' tag since tests are included with the patch (hopefully it passes).

Status:Needs review» Needs work

The last submitted patch, 1067120_65.patch, failed testing.

mrfelton’s picture

StatusFileSize
new10.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-term-build-extension-1067120-73-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Needed this for D7. Here is a reroll against D7 that can be applied using drush make. It is the patch from #65 verbatim.

BTMash’s picture

First off, thank you @mrfelton. Someone else following up in the issue is very much appreciated :)

With that said, I haven't been following the status of the taxonomy module move to being in line with the work in the entity module (#1361232: Make the taxonomy entities classed objects)? Looks like its getting closer and closer. It seems like the patch for D8 is going to *wildly* differ from the patch for D7. In which case, is it even going to be possible for *a* patch to go in for D7?

Rontero’s picture

Status:Needs work» Needs review
Issue tags:-Entity system, -Metatags, -Novice, -needs backport to D7, -Contributed project blocker

#65: 1067120_65.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity system, +Metatags, +Novice, +needs backport to D7, +Contributed project blocker

The last submitted patch, taxonomy-term-build-extension-1067120-73-d7.patch, failed testing.

DamienMcKenna’s picture

@BTMash: I think this patch would probably (I *hope*) fit within the somewhat expanded guidelines for accepting changes to the stable release discussed a while back.

joachim’s picture

> In which case, is it even going to be possible for *a* patch to go in for D7?

Indeed. This is either a bug in D7, or hook_entity_view() is lying.

This blocks flag module too BTW.

DamienMcKenna’s picture

I tested the patch from #73 on a site with a related patch for Metatag (#1363476: Panels integration - ensure meta tags work OOTB on entity pages) and it works great.

DamienMcKenna’s picture

FYI I've committed a patch to Metatag so that it now uses hook_entity_view() instead of unstable alternatives (thank you jenlampton!), so now I'm really interested in getting this committed :) If nobody else beats me to it, I'm aiming to work on any necessary polish for this patch so that maybe we can push it to be added to the next release.

If anyone has suggestions for alternative hooks to use (especially for backwards compatibility with older D7 releases), please let me know in #1700160: Support taxonomy term pages until taxonomy supports hook_entity_view(). Thanks.

dropbydrop’s picture

I hope this to get fixed soon for metatag D7.
thanks!
keep up the good work

DamienMcKenna’s picture

Status:Needs work» Needs review
StatusFileSize
new10.43 KB
FAILED: [[SimpleTest]]: [MySQL] 37,217 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

A rerolled version of #65 that should apply cleanly.

Status:Needs review» Needs work

The last submitted patch, drupal-n1067120-82-taxonomy_term_view.patch, failed testing.

DamienMcKenna’s picture

Status:Needs work» Needs review
StatusFileSize
new10.42 KB
FAILED: [[SimpleTest]]: [MySQL] 37,212 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Doh. This should work.

Status:Needs review» Needs work

The last submitted patch, drupal-n1067120-84-taxonomy_term_view.patch, failed testing.

DamienMcKenna’s picture

StatusFileSize
new10.44 KB
FAILED: [[SimpleTest]]: [MySQL] 37,216 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Maybe I should have reviewed the code I changed before submitting the patch :-p

DamienMcKenna’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-n1067120-86-taxonomy_term_view.patch, failed testing.

DamienMcKenna’s picture

Status:Needs work» Needs review

This test is failing:

    $this->assertFalse(empty($term_build['taxonomy_terms'][$term->tid]['taxonomy_test_entity_view_check']), 'hook_entity_view() was invoked when viewing the term.');

That suggests the hook is not working correctly. Urk.

DamienMcKenna’s picture

Status:Needs review» Needs work

Wrong status.

corvus_ch’s picture

Assigned:Unassigned» corvus_ch
corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new10.19 KB
PASSED: [[SimpleTest]]: [MySQL] 40,254 pass(es).
[ View ]

The hook entity_view was called with the wrong parameters. I fixed this now and also made the patch to apply to current 8.x branch.

Berdir’s picture

Status:Needs review» Needs work

Here's a quick review...

+++ b/core/modules/system/tests/modules/taxonomy_test/taxonomy_test.moduleundefined
@@ -3,6 +3,8 @@
+ * @see TaxonomyHooksTestCase::testTaxonomyTermHooks()

Update for the PSR-0-ified test class name.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/HooksTest.phpundefined
@@ -35,7 +35,10 @@ class HooksTest extends TaxonomyTestBase {
-   * Test that hooks are run correctly on creating, editing and deleting a term.
+   * Test that hooks are run correctly on creating, editing, viewing,
+   * and deleting a term.

The first line of a docblock should not be more than a single line (nor longer than 80 characters...).

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -569,6 +569,101 @@ function taxonomy_term_delete_multiple(array $tids) {
+function taxonomy_term_show($term) {
+  return taxonomy_term_view_multiple(array($term->tid => $term), 'full');

Usually this function is called view, which in this case already exists... Not sure what to do here.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -569,6 +569,101 @@ function taxonomy_term_delete_multiple(array $tids) {
+  if (!isset($langcode)) {
+    $langcode = $GLOBALS['language_content']->language;

This isn't up to date anymore, should use language() and the DIC.

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -35,21 +35,24 @@ function taxonomy_term_page(Term $term) {
+  // of the active trail, and the link name becomes the page title.
+  // Thus, we must explicitly set the page title to be the node title.
+  $uri = entity_uri('taxonomy_term', $term);

Should use $term->uri().

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new10.23 KB
PASSED: [[SimpleTest]]: [MySQL] 40,254 pass(es).
[ View ]

Here a new patch including Berdirs feedback.

As of the function naming, this is more or less consistent with other modules. A possible standardisation is going on in issue #1026616: Implement an entity render controller.

Berdir’s picture

Status:Needs review» Needs work
+++ b/core/modules/system/tests/modules/taxonomy_test/taxonomy_test.moduleundefined
@@ -3,6 +3,8 @@
+ * @see Drupal\taxonomy\Tests/TaxonomyHooksTestCase::testTaxonomyTermHooks()

That / should be \

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -569,6 +569,101 @@ function taxonomy_term_delete_multiple(array $tids) {
+ * @param term
+ *   A taxonomy term object.

Should be "@param Drupal\taxonomy\Term $term".

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -569,6 +569,101 @@ function taxonomy_term_delete_multiple(array $tids) {
+ * @return
+ *   A $page element suitable for use by drupal_page_render().

@return array

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -569,6 +569,101 @@ function taxonomy_term_delete_multiple(array $tids) {
+ * @param $terms
+ *   An array of taxonomy terms as returned by taxonomy_term_load_multiple().
+ * @param $view_mode
+ *   View mode, e.g. 'full', 'teaser'...
+ * @param $weight
+ *   An integer representing the weight of the first node in the list.
+ * @param $langcode
+ *   (optional) A language code to use for rendering. Defaults to the global
+ *   content language of the current request.
+ *
+ * @return
+ *   An array in the format expected by drupal_render().

We unfortunately don't have a notion for an array of classes, so it's just @param array $terms here. The other @params and @return also need the type.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -569,6 +569,101 @@ function taxonomy_term_delete_multiple(array $tids) {
+ * @param $term
+ *   A taxonomy term object.
+ * @param $view_mode
+ *   View mode, e.g. 'full', 'teaser'...
+ * @param $langcode
+ *   (optional) A language code to use for rendering. Defaults to the global
+ *   content language of the current request.

Same.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -587,28 +682,20 @@ function taxonomy_term_view(Term $term, $view_mode = 'full', $langcode = NULL) {
+  // Populate $node->content with a render() array.
+  taxonomy_term_build_content($term, $view_mode, $langcode);
+  $build = $term->content;
+
+  // We don't need duplicate rendering info in node->content.
+  unset($term->content);

Comment is wrong ($node should be $term)

fgm’s picture

Status:Needs work» Needs review
StatusFileSize
new11.2 KB
PASSED: [[SimpleTest]]: [MySQL] 40,252 pass(es).
[ View ]

Rerolled accordingly. Having a unified view process will simplify #1026616: Implement an entity render controller: taxonomy terms are the only ones differing in their view process from the way comment / node / user do it.

corvus_ch’s picture

StatusFileSize
new10.87 KB
FAILED: [[SimpleTest]]: [MySQL] 40,250 pass(es), 3 fail(s), and 16 exception(s).
[ View ]

There was one type hint missing.

Status:Needs review» Needs work

The last submitted patch, 1067120-97.patch, failed testing.

fgm’s picture

Patch at #96 does not have the same problem as the one on #97, which seems to be based on an earlier version. Main issue in #97 is taxonomy_term_view_multiple still having a Term $terms hint, whereas its first parameter is an array, not a Term.

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new10.87 KB
PASSED: [[SimpleTest]]: [MySQL] 40,256 pass(es).
[ View ]

Oops, sorry my bad. I have used the wrong type hint. Fixed with this patch.

fgm’s picture

Indeed my #96 was missing a Term hint and an array hint. Good now.

Berdir’s picture

Status:Needs review» Needs work
+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -170,7 +170,7 @@ function taxonomy_entity_info() {
-function taxonomy_term_uri($term) {
+function taxonomy_term_uri(Term $term) {
   return array(
     'path' => 'taxonomy/term/' . $term->tid,

While correct, this change is unrelated and should IMHO not be in this patch to avoid discussions/conflicts. If you can re-roll without that, then I'm happy to RTBC it. After all, we're going to delete most of this code again anyway in the entity render issue, this is really just a preparation to make the conversion there easier.

DamienMcKenna’s picture

@Berdir: if this entire system is going to be scrapped in D8, can we give up on this change for D8 and instead focus it on D7?

Berdir’s picture

@DamienMcKenna: The system will stay (the view API functions added here), the other issue will just replace the actual implementation of it with a generic one. So I think it makes sense to get this in and it's close now.

DamienMcKenna’s picture

Status:Needs work» Needs review
StatusFileSize
new10.8 KB
PASSED: [[SimpleTest]]: [MySQL] 40,333 pass(es).
[ View ]

Does this work? Also I fixed the spelling of "URI" in the function's comment.

Status:Needs review» Needs work
Issue tags:-Entity system, -Metatags, -Novice, -needs backport to D7, -Contributed project blocker

The last submitted patch, drupal-n1067120-105.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+Entity system, +Metatags, +Novice, +needs backport to D7, +Contributed project blocker

#105: drupal-n1067120-105.patch queued for re-testing.

Berdir’s picture

The reason for removing the Term type hint there was to avoid including unrelated changes in this patch. The URI fix is exactly the same kind of unrelated change and just as problematic. Can we get a new patch with that removed and a separate issue opened to fix that function?

DamienMcKenna’s picture

StatusFileSize
new10.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-n1067120-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Without the 'uri' change.

FYI I opened another issue to fix all misspellings of the acronym "URI": #1742958: All spellings of URI should be uppercase

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Yay, let's get this in then! (Edit: assuming this will pass the tests but there's no reason it wouldn't, it's just a comment change or better a not-change)

Berdir’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Entity system, +Metatags, +Novice, +needs backport to D7, +Contributed project blocker

The last submitted patch, drupal-n1067120-109.patch, failed testing.

fgm’s picture

Patch no longer applies because parts of it were committed in the meantime.

corvus_ch’s picture

Status:Needs work» Needs review
StatusFileSize
new10.9 KB
PASSED: [[SimpleTest]]: [MySQL] 40,696 pass(es).
[ View ]

Made patch apply again.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, I confirmed manually that it works and the view mode alter stuff also works correctly (which was the reason the old patch conflicted).

fgm’s picture

Issue tags:+sprint

Tagging for the Munich sprint.

DamienMcKenna’s picture

I'd really, really, really like to see this added to D8 so I can reroll it for D7 and hopefully get it into the next stable release. Who do I need to send donuts / chocolate to so this can be committed? :-)

warmth’s picture

I'm trying to get a link beside each content tag (using terms + Flags module) to report if the tag is not valid for that content (it doesn't represent anything real about the content) but I can get the link to appear and I think is because of this bug, can you please help me?

Sorry for cross posting here: http://drupal.org/node/1035410#comment-6399632

DamienMcKenna’s picture

@warmth: Thanks for your interest, at a guess your issue should start to work once this item is resolved; that said, please deal with any follow-on either in the other Flag issue or open a new one for that module.

fgm’s picture

Note: during the Entity API Munich sprint, issue #1026616: Implement an entity render controller was implemented as depending on this patch. I would thus really like to see it committed so that the other issue can be considered.

OR ... maybe we should roll it into the other one ?

DamienMcKenna’s picture

Does this need an improved issue summary, or just more hours in the day (and my patience)?

corvus_ch’s picture

All this patch currently needs is to get the attention of a core committer. Currently there are still 43 Patches in the queue for committing. If it does not get committed soon this probably will need some rerolls for it will not apply any more. So this will take some more of your patience.

blischalk’s picture

I am also very interested in the status of this issue. I am trying to use hook_field_extra_fields to create "pseudo" fields as described in this article: http://www.computerminds.co.uk/drupal-code/add-stuff-node-and-configure-.... The method the author describes works just fine for nodes because hook_node_view exists. Because hook_taxonomy_term_view does not exist I am unable to apply this technique to taxonomies.

Summit’s picture

Hi, yes please, please commit #114 http://drupal.org/node/1067120#comment-6381112 and backport to D7 please!
Thanks a lot in advance,
greetings, Martijn

Berdir’s picture

#114: 1067120-114.patch queued for re-testing.

webchick’s picture

Version:8.x-dev» 7.x-dev
Assigned:corvus_ch» David_Rothstein
Status:Reviewed & tested by the community» Patch (to be ported)
Issue tags:+Needs issue summary update

Note: Begging for features doesn't get them committed any faster; fixing major/critical bugs and tasks so we get under issue queue thresholds does. :) That finally happened again tonight, so features are once again back on the table.

I read through this patch pretty extensively, checking it out alongside http://api.drupal.org/api/drupal/core!modules!node!node.module/function/... and http://api.drupal.org/api/drupal/core!modules!node!node.module/function/... and the like. Everything in this patch seems to make sense, and is simply unifying the taxonomy page functions w/ the node page functions.

Committed and pushed to 8.x. Thanks! Marking to 7.x for backport, although it's a pretty extensive change so could likely use David's sign-off. Temporarily assigning to him. An up-to-date issue summary, including the API impacts of this on D7, would help expedite his review. Tagging accordingly.

DamienMcKenna’s picture

@webchick: Thanks!!! And your comment about bugfixing is noted.

I'll work on a re-roll and updating the issue summary.

Berdir’s picture

I think the backport version of this should be as small as possible and not include any API changes. I'm quite sure we have to live with the inconsistent functions, missing multiple and so on and just add the hooks. More or less going back to the initial small patch, I guess.

David_Rothstein’s picture

Assigned:David_Rothstein» Unassigned

I looked this over and Berdir's comment makes sense to me. If the earlier patches in this issue are sufficient to solve the problem, those seem very straightforward (just adding new hooks) and get a +1 from my point of view.

In general, I don't see a problem with reorganizing code into new functions in Drupal 7 if we really have to... but the full patch here is pretty complicated and it seems like it has lots of opportunity to introduce (either intentionally or accidentally?) data structure or order-of-operations changes that could cause problems in Drupal 7. So at the very least, the simpler patches here would be a lot quicker to get reviewed and committed :)

DamienMcKenna’s picture

I've updated the issue summary accordingly.

DamienMcKenna’s picture

@David_Rothstein: The initial patches from valthebald do not add hook_taxonomy_term_view, while the patches from BTMash go on a tangent to change the actual output. I'd personally vote for just backporting the most recent patch as a) the execution is not that different, b) there weren't any hooks explicitly executed during taxonomy_term_page() so there's little-to-no chance of introducing any regressions. I'll have an initial patch in a few minutes.

DamienMcKenna’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new10.29 KB
PASSED: [[SimpleTest]]: [MySQL] 39,383 pass(es).
[ View ]

This is a reroll of mrfelton's patch from #73 which has been working well for me on a site I've been building over the past two months; other than some differences in between D7 and D8, including a different testing structure, it is pretty much identical to the patch from #114.

xjm’s picture

Status:Needs review» Needs work

Chatted with @DamienMcKenna about this in IRC. The old patch needs a few changes to bring it back into line with the newer 8.x patch. Thanks folks!

DamienMcKenna’s picture

Status:Needs work» Needs review
StatusFileSize
new10.25 KB
PASSED: [[SimpleTest]]: [MySQL] 39,419 pass(es).
[ View ]

Summary of differences between the D7 and D8 versions of taxonomy.module:

  • taxonomy_term_show() comments & "prototype": parameters and return for D8 code are different.
  • taxonomy_term_show() code: identical.
  • taxonomy_term_view_multiple() comments & "prototype": parameters and return for D8 code are different per newer coding standards.
  • taxonomy_term_view_multiple() code: identical.
  • taxonomy_term_build_content() comments & "prototype": parameters and return for D8 code are different per newer coding standards.
  • taxonomy_term_build_content() code differences:
    • The D8 version handles the $langcode differently.
    • The D8 version also has the following:
      <?php
       
      // Allow modules to change the view mode.
       
      $context = array('langcode' => $langcode);
       
      drupal_alter('entity_view_mode', $view_mode, $term, $context);
      ?>

      This is to support the new hook_entity_view_mode/hook_entity_view_mode_alter already added to D8 and in progress for D7 (#1154382: View mode no longer can be changed).

    • The D7 version uses different structures to identify the entity type and bundle info.
    • D7 and D8 handle hook_entity_view differently.
  • taxonomy_term_view() comments & "prototype": parameters and return for D8 code are different per newer coding standards.
  • taxonomy_term_view() code differences:
    • The D8 version handles the $langcode differently.
    • The drupal_alter() calls are handled differently but match the handling of drupal_alter() for node_view().

Summary of differences between the D7 and D8 versions of taxonomy.pages.inc:

  • taxonomy_term_page() comments & "prototype": parameters and return for D8 code are different per newer coding standards, e.g. the @return object is not defined.
  • taxonomy_term_page() code differences:
    • The D8 version handles the page title differently ($term->label() vs $term->name).
    • D7 has taxonomy_get_parents() while D8 has taxonomy_term_load_parents().
    • The D8 version handles the $uri differently ($term-uri() vs using entity_uri()).

This new patch changes the following:

  • The entity_get_info() call from taxonomy_term_build_content() has been removed - the $entity_data variable wasn't used and the function call made no difference in my testing (simpletests, and manual via Metatag).
  • Fixed a comment typo in taxonomy_term_view() where "node->content" was given instead of "$term->content".

I think this should cover everything.

DamienMcKenna’s picture

StatusFileSize
new10.25 KB
PASSED: [[SimpleTest]]: [MySQL] 39,402 pass(es).
[ View ]

Missed a period at the end of a comment.

brunorios1’s picture

Status:Needs review» Reviewed & tested by the community

i tested and it worked in D7.

webchick’s picture

We're once again over thresholds atm so I unfortunately can't commit any feature patches. Help with smashing critical and major bugs would be greatly appreciated.

joachim’s picture

As I said above, I really think this is a bug. hook_entity_view() promises something that isn't delivered.

DamienMcKenna’s picture

Category:feature» bug

It is a bug report:

  • Entities are supposed to be handled the same way, regardless of what the individual entity object is.
  • Entities should have the same hook_entity_* hooks invoked for all CRUD and display processes.
  • Taxonomy.module invokes many other entity hooks, e.g. hook_entity_presave, hook_entity_update, hook_entity_insert, hook_entity_delete, etc for both taxonomy terms and vocabularies.
  • There currently are no entity hooks invoked when viewing taxonomy term pages - hook_entity_view is triggered when viewing a node page but not when viewing a taxonomy term page.
  • A similar issue for the forum module is also marked as a bug report: #1122808: hook_entity_view and hook_taxonomy_term_view() are not implemented on forum term views

I'm not sure why Dave did not file this as a bug report when he did file the sibling issue (#1122808) as a bug report.

DamienMcKenna’s picture

The various core issue queues are all at or under their respective threshold levels, is there anything else I can do to help ready this issue or should I just be a little more patient? :)

DamienMcKenna’s picture

FYI this patch still applies with #1154382: View mode no longer can be changed committed.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Probably the best thing is not to repeatedly "bump" the patch, since I often process the D7 queue FIFO. :)

This still smells pretty feature-y to me, and we're once again over thresholds, but I can appreciate that this blocks contributed module progress, and Damien did a really nice job smacking around the thresholds the other week before I lost track of this patch. Therefore, I'm comfortable to go ahead and commit this. Thanks a ton!

Unfortunately, the patch no longer applies. :( Can we get a quick re-roll? And while you're at it, can you add a hunk to taxonomy.api.php to document this new hook addition? (NO idea how I missed that in the 8.x patch. :P) That should technically go against 8.x first, but I can do it manually from the 7.x patch.

DamienMcKenna’s picture

Status:Needs work» Needs review
StatusFileSize
new11.44 KB
PASSED: [[SimpleTest]]: [MySQL] 39,445 pass(es).
[ View ]

@webchick: Thank you :-)

Does this work? I snagged the hook_node_view() code from node.api.php, changed it to say $term instead of $node and removed the part about the RSS feeds as it didn't seem accurate for terms.

Berdir’s picture

Status:Needs review» Needs work
+++ b/modules/taxonomy/taxonomy.api.phpundefined
@@ -182,6 +182,32 @@ function hook_taxonomy_term_delete($term) {
+ * @ingroup node_api_hooks

Almost :(

DamienMcKenna’s picture

Status:Needs work» Needs review
StatusFileSize
new11.4 KB
PASSED: [[SimpleTest]]: [MySQL] 39,445 pass(es).
[ View ]

@berdir: Good catch. I didn't see another @ingroup in the taxonomy.api.php file so I just removed that line entirely.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Yes, looks good.

webchick’s picture

Title:Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term» Change notice: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term
Category:bug» task
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Awesome, thanks a ton!

Committed and pushed to 7.x, with api.php hunk to 8.x as well. YAY! :D

Let's get a change notice added for this.

Berdir’s picture

The 8.x should be updated a bit to follow the 8.x standards and be consistent with others, e.g. a type hint.

DamienMcKenna’s picture

What should the change notice document - the new hooks in both branches or just 7.x? How about the following?

Added functions:
taxonomy_term_show($term)
taxonomy_term_view_multiple($terms, $view_mode = 'teaser', $weight = 0, $langcode = NULL)
taxonomy_term_build_content($term, $view_mode = 'full', $langcode = NULL)

Added hooks:
hook_taxonomy_term_view($term, $view_mode, $langcode) - triggered during taxonomy_term_view() and before hook_entity_view() is triggered.

API change:
hook_entity_view() is now triggered during a taxonomy term page load.

Impacts:
Module developers.

andypost’s picture

#149 looks like good summary, D7 and D8 changes are the same so 1 change notice is enough. It would be good to provide an example code like #145 does in tests

DamienMcKenna’s picture

@andypost: Like this?

Added functions:
taxonomy_term_show($term)
taxonomy_term_view_multiple($terms, $view_mode = 'teaser', $weight = 0, $langcode = NULL)
taxonomy_term_build_content($term, $view_mode = 'full', $langcode = NULL)

Added hooks:
hook_taxonomy_term_view($term, $view_mode, $langcode) - triggered during taxonomy_term_view() and before hook_entity_view() is triggered.

API change:
hook_entity_view() is now triggered during a taxonomy term page load.

Impacts:
Module developers.

Example:

<?php
/**
 * Implements hook_taxonomy_term_view().
 *
 * Add a new value to the taxonomy term page output.
 */
function example_taxonomy_term_view($term, $view_mode, $langcode) {
 
$term->content['my_additional_field'] = array(
   
'#markup' => t('Surprise!'),
   
'#weight' => 10,
   
'#theme' => 'mymodule_my_additional_field',
  );
}
?>
andypost’s picture

@DamienMcKenna Yes, Great! Please file a change notice and make critical--

DamienMcKenna’s picture

andypost’s picture

Title:Change notice: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term» Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term
Status:Active» Fixed

As for me it describes all the changes

andypost’s picture

Priority:Critical» Normal

Back to its original priority

David_Rothstein’s picture

Issue tags:+7.17 release notes

Drupal 7.16 was a security release only, so this issue is now scheduled for Drupal 7.17 instead.

Fixing tags accordingly.

warmth’s picture

oh :(

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Priority:Normal» Critical
Status:Closed (fixed)» Needs work
StatusFileSize
new132.81 KB
new139.82 KB

Hm, I am playing around with this and seeing that while this patch mostly works as advertised (and works well), it also seems to result in some major data structure changes and HTML markup changes on the taxonomy term page.

The attached screenshots of Drupal 7.16 and (what is slated to become) Drupal 7.17 illustrate that, since I've included the output of dsm($build) for that page.

The data structure change probably only affects someone altering this via hook_page_alter() (probably not too common), but the corresponding HTML markup change is a big problem since this is a public-facing page (you can see it in the render array in the screenshot; the #prefix and #suffix that used to be there before are gone).

I am not sure we should release Drupal 7.17 tomorrow without some kind of resolution to this. I don't have a whole lot of time, but I'm trying to work on a quick patch for now.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new4 KB
FAILED: [[SimpleTest]]: [MySQL] 39,676 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

The attached patch seems to work for me.

Basically, since we're only rendering one term anyway, we shouldn't actually need to go through the whole taxonomy_term_show() => taxonomy_term_view_multiple() pipeline on the term page. If we call taxonomy_term_view() directly, all the same hooks that were added in the original patch here should still get fired, etc. But now we get a data structure back that we can easily make compatible with previous releases of Drupal 7.

I also fixed a few references to "node" and node-related concepts that looked like they were the result of copy-paste from the node module...

This patch could REALLY use a review if anyone has time between now and tomorrow.

Berdir’s picture

I haven't actually tested this, but i was working on a comment that proposed basically exactly what you just uploaded as a patch minus the documentation improvements.

+++ b/modules/taxonomy/taxonomy.pages.incundefined
@@ -33,17 +33,23 @@ function taxonomy_term_page($term) {

-  // If there is a menu link to this term, the link becomes the last part of
-  // the active trail, and the link name becomes the page title. Thus, we must
-  // explicitly set the page title to be the node title.
-  $uri = entity_uri('taxonomy_term', $term);
-
   // Set the term path as the canonical URL to prevent duplicate content.
+  $uri = entity_uri('taxonomy_term', $term);

Have you reverted the comment intentionally? It incorrectly talks about a node title I think but might otherwise be useful information? Other than that, the patch looks great to me and I agree that we should stick to the existing structure. if possible. The keys within the term looks identical to me, just a different order. Would have been a bigger issue if these were different too.

Berdir’s picture

Issue tags:-Needs change record

Removing the change notice tag, that was added and is not affected by the proposed follow-up patch.

David_Rothstein’s picture

Thanks for the quick review.

Have you reverted the comment intentionally? It incorrectly talks about a node title I think but might otherwise be useful information?

Well, the whole paragraph seemed to be talking about setting the page title, but the code around it isn't actually setting the page title at all. So I assumed it was a bad copy-paste. Maybe the relevant parts of the code comment should be moved somewhere else?

Status:Needs review» Needs work

The last submitted patch, taxonomy-term-view-followup-1067120-160.patch, failed testing.

Berdir’s picture

+++ b/modules/taxonomy/taxonomy.testundefined
@@ -1420,6 +1424,13 @@ class TaxonomyHooksTestCase extends TaxonomyWebTestCase {

+    // View the term and ensure that hook_taxonomy_term_view() and
+    // hook_entity_view() are invoked.
+    $term = taxonomy_term_load($term->tid);
+    $term_build = taxonomy_term_page($term);
+    $this->assertFalse(empty($term_build['taxonomy_terms'][$term->tid]['taxonomy_test_term_view_check']), 'hook_taxonomy_term_view() was invoked when viewing the term.');
+    $this->assertFalse(empty($term_build['taxonomy_terms'][$term->tid]['taxonomy_test_entity_view_check']), 'hook_entity_view() was invoked when viewing the term.');
+
     // Delete the term.

Looks like we also need to update these checks as they obviously still use the other structure.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new5.53 KB
PASSED: [[SimpleTest]]: [MySQL] 39,623 pass(es).
[ View ]

Yup. Here's a new patch which does that. I also moved/edited the code comment about the page title rather than deleting it.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

I think this change makes sense and the actual patch looks good to me.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed

Thanks so much for the review.

Committed this to 7.x so we can get Drupal 7.17 out soon: http://drupalcode.org/project/drupal.git/commit/7208952

David_Rothstein’s picture

Title:Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term» Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term (documentation followup)
Version:7.x-dev» 8.x-dev
Category:task» bug
Priority:Critical» Normal
Status:Fixed» Patch (to be ported)

Actually, I guess I messed up a little bit by including the documentation fixes in there also, since some of those belong in Drupal 8 as well.

Can try to get that done eventually.

DamienMcKenna’s picture

David, thanks for the follow-up work on this, Metatag users are all very grateful :-)

warmth’s picture

Thanks! Now that have been fixed is there any possibility of displaying them beside the tags on a content detail page (node tags)? I can only see the flag inside the tag page (the one that display all the items tagged with that taxonomy)!

Note: I couldn't find any thread talking about that, sorry if is not the proper place to do it.

thedosmann’s picture

Did this get into 7.17?

DamienMcKenna’s picture

@thedosmann: Yes.

thedosmann’s picture

Wow I just upgraded and lost all my meta descriptions in my forums I had from the patch I applied above.

DamienMcKenna’s picture

@thedosmann: Open an issue in the Metatag issue queue and I can help you there.

thedosmann’s picture

k

Berdir’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 49,426 pass(es).
[ View ]

Ok, I think that's all that's left to port back to 8.x from David's patch in #166, the other things have been removed as the render controller was added.

Quick RTBC so that we can finally close this issue?

DamienMcKenna’s picture

Status:Needs review» Reviewed & tested by the community

I think this is good to go.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Yay for closing issues. :)

Committed and pushed to 8.x. Whew!

thedosmann’s picture

I needed to footnote this. When I applied the patch above I did it from command line using the native nix patch command. I usually note when I do it this way but somehow overlooked. So when I went to 7.17 the files I patched didn't get updated. I had to go in as admin and delete the ones I patched and then plant the updated ones in meta tags. I didn't want to leave this dangling. After the update my meta tags came back. Thanks for the fix.

David_Rothstein’s picture

Title:Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term (documentation followup)» Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sic’s picture

May someone be that nice and summ up what I need to do to make the view and view_alter hook work for my drupal 7.19? Thanks so much

sic’s picture

Issue summary:View changes

Issue summary updated.