Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term

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

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)

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

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

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.

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

subscribe

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

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()?

Subscribe

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

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

8.x and 7.x behave not the same, see #796692: Only show the term heading if the term has a description

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.

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.

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.

#24: taxonomy_term_build_extension_1067120_d8_25.patch queued for re-testing.

Some small cleanup of what was in there.

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

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.

Updated patch with cleanup of spacing.

Ack...forgot to mark as needs review.

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

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

#34: taxonomy_term_build_extension_1067120_d8_34.patch queued for re-testing.

#34: taxonomy_term_build_extension_1067120_d8_34.patch queued for re-testing.

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

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

Ack...whitespace errors.

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

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.

#34: taxonomy_term_build_extension_1067120_d8_34.patch queued for re-testing.

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.

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

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

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.

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

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

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.

@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 :)

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

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

Try the rerolled patch.

Mark as needs review.

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

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

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

Forgot to unassign after adding patch.

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

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.

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.

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.

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

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

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

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

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

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

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?

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

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

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

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.

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.

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

A rerolled version of #65 that should apply cleanly.

Doh. This should work.

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

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.

Wrong status.

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.

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

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.

+++ 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)

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.

There was one type hint missing.

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.

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

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

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

@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?

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

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

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

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?

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

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)

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

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

Made patch apply again.

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

Tagging for the Munich sprint.

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? :-)

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

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

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 ?

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

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.

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.

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

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

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.

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

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

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.

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

I've updated the issue summary accordingly.

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

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.

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!

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:
    

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

Missed a period at the end of a comment.

i tested and it worked in D7.

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.

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

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.

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? :)

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

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.

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

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

Almost :(

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

Yes, looks good.

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.

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

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.

#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

@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:

/**
 * 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',
  );
}

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

Posted: http://drupal.org/node/1808870

As for me it describes all the changes

Back to its original priority

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

Fixing tags accordingly.

oh :(

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.

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.

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.

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

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?

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

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

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

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

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.

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

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.

Did this get into 7.17?

@thedosmann: Yes.

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

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

k

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?

I think this is good to go.

Yay for closing issues. :)

Committed and pushed to 8.x. Whew!

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.

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

Issue summary updated.

@sic, see my answer over on Drupal Answers.

This was committed to both Drupal 7 and 8 a long time ago.

To the best of my knowledge these hooks are consistently invoked when a term is being viewed - for example by taxonomy_term_page() which calls taxonomy_term_view() which invokes the hooks. The Drupal Answers post above is several years old and looks like it's out of date. However, if there is a problem with these hooks not being invoked, please create a new issue with details and link to it here.