The hook-documentation for the taxonomy module in taxonomy.api.php is not consistent. The earliest examples stem from the removal of synonyms (see: #314870: UNSTABLE-4 blocker: Add api.php for every core module, #567572: Remove taxonomy synonyms since Field API is better) from core in Drupal 7. During subsequent updates, documentation for more hooks were added. However later examples do not blend in well with the original docs because they illustrate the hooks based on other (hypothetical) cases (#764726: hook_taxonomy_term_presave() is missing, #835046: hook_taxonomy_vocabulary_presave() is missing, #949576: Missing hook_entity_view() and hook_entity_view_alter()).

The original docs however have two problems:

I propose that the examples for the following functions should be rewritten in order to remove references to old synonym-tables and variables.

  • hook_taxonomy_vocabulary_load
  • hook_taxonomy_vocabulary_insert
  • hook_taxonomy_vocabulary_update
  • hook_taxonomy_vocabulary_predelete
  • hook_taxonomy_vocabulary_delete
  • hook_taxonomy_term_insert
  • hook_taxonomy_term_update
  • hook_taxonomy_term_predelete
  • hook_taxonomy_term_delete

hook_taxonomy_term_load could serve as an example.

Files: 
CommentFileSizeAuthor
#46 drupal_core-taxonomy_doxy_cleanup-1831540-46.patch4.04 KBorb
PASSED: [[SimpleTest]]: [MySQL] 40,331 pass(es).
[ View ]
#41 drupal_core-taxonomy_doxy_cleanup-1831540-41.patch1.11 KBorb
PASSED: [[SimpleTest]]: [MySQL] 57,576 pass(es).
[ View ]
#37 drupal_core-taxonomy_doxy_cleanup-1831540-37.patch4.06 KBorb
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es).
[ View ]
#29 drupal_core-taxonomy_doxy_cleanup-1831540-29.patch5.82 KBorb
PASSED: [[SimpleTest]]: [MySQL] 55,051 pass(es).
[ View ]
#26 drupal_core-taxonomy_doxy_cleanup-1831540-26.patch5.79 KBorb
PASSED: [[SimpleTest]]: [MySQL] 56,410 pass(es).
[ View ]
#19 drupal_core-taxonomy_doxy_cleanup-1831540-19.patch5.76 KBorb
PASSED: [[SimpleTest]]: [MySQL] 57,265 pass(es).
[ View ]
#17 drupal_core-taxonomy_doxy_cleanup-1831540-17.patch5.76 KBorb
PASSED: [[SimpleTest]]: [MySQL] 55,642 pass(es).
[ View ]
#15 drupal_core-taxonomy_doxy_cleanup-1831540-15.patch5.76 KBorb
PASSED: [[SimpleTest]]: [MySQL] 55,636 pass(es).
[ View ]
#11 drupal_core-taxonomy_doxy_cleanup-1831540-11.patch5.72 KBorb
PASSED: [[SimpleTest]]: [MySQL] 56,086 pass(es).
[ View ]
#10 drupal_core-taxonomy_doxy_cleanup-1831540-10.patch4.98 KBorb
PASSED: [[SimpleTest]]: [MySQL] 55,919 pass(es).
[ View ]
#8 drupal_core-taxonomy_doxy_cleanup-1831540-8.patch5.96 KBorb
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.api.php.
[ View ]
#4 drupal.1831540.4.taxonomy_doxy_cleanup.patch5.64 KBGaelan
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.api.php.
[ View ]
#1 1831540-taxonomy-remove_synonyms_hook_api_docs.patch4.86 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 49,277 pass(es).
[ View ]

Comments

cam8001’s picture

StatusFileSize
new4.86 KB
PASSED: [[SimpleTest]]: [MySQL] 49,277 pass(es).
[ View ]

These hooks aren't used anywhere in core, so I had to make up my own dubious examples. Hope they are ok.

cam8001’s picture

Status:Active» Needs review
znerol’s picture

Component:taxonomy.module» documentation
Category:bug» task
Issue tags:+developer

Add tag and move to documentation project.

The examples are still not to coherent but frankly I'm not able to come up with a more realistic use-case touching all of the hooks. Let's see if we get input from the docs team.

Gaelan’s picture

StatusFileSize
new5.64 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.api.php.
[ View ]

Initial patch.

EDIT: Whoops! I got this task and forgot to read the issue. Gaelan--

Status:Needs review» Needs work

The last submitted patch, drupal.1831540.4.taxonomy_doxy_cleanup.patch, failed testing.

jhodgdon’s picture

Thanks for the patches!

So... It is kind of important that the sample function bodies in hook documentation follow the Drupal coding standards. I noticed a couple of things in hook_taxonomy_vocabulary_insert() (they apply to other functions in the patch too):
- Normally we use single quotes instead of double quotes for strings (unless the string itself contains apostrophes/single quotes).
- We don't normally split watchdog() calls into multiple lines.

Elsewhere in the patch, I noticed:
- The word "translator" is misspelled several times.
- The term delete hook body is not valid (it references vocabulary not terms). Also the vocabulary create hook body references term not vocabulary.
- I think each sample function body could be improved by starting it with a code comment explaining what the sample body does. For instance "// A term has changed, so notify translators." etc.
- I like some of the examples, but I'm not too excited about:
-- The "paper trail" examples. Can we come up with something better?
-- The vocabulary load one that multiplies all the weights by 10 (what for?)
-- The ones that set a value for "foo", without explaining what it would do... and will that even work? Where is this "foo" field/property defined?

orb’s picture

Assigned:Unassigned» orb

We are working now with this issue during Code Sprint UA.

orb’s picture

Status:Needs work» Needs review
Issue tags:-developer+CodeSprintUA
StatusFileSize
new5.96 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.api.php.
[ View ]

Fix:
1. Change examples
2. fix Drupal coding standards

Berdir’s picture

Status:Needs review» Needs work
+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -53,7 +56,7 @@ function hook_taxonomy_vocabulary_load(array $vocabularies) {
-  $vocabulary->foo = 'bar';
+  $vocabulary->set('foo') = 'bar';

This is not PHP :)

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -66,8 +69,8 @@ function hook_taxonomy_vocabulary_presave(Drupal\taxonomy\Plugin\Core\Entity\Voc
+  if ($vocabulary->vid = 'my_vocabulary') {

This should be ==

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -98,9 +102,7 @@ function hook_taxonomy_vocabulary_update(Drupal\taxonomy\Plugin\Core\Entity\Voca
+  db_delete('mytable_index')->condition('vid', $vocabulary->vid)->execute();

@@ -115,9 +117,7 @@ function hook_taxonomy_vocabulary_predelete(Drupal\taxonomy\Plugin\Core\Entity\V
+  db_delete('mytable')->condition('vid', $vocabulary->vid)->execute();

fluent method calls should be on separate lines, indented by two spaces like this:

<?php
db_delete
()
  ->
condition()
  ->
execute()
?>
+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -152,9 +152,12 @@ function hook_taxonomy_term_create(\Drupal\taxonomy\Plugin\Core\Entity\Term $ter
-  $result = db_query('SELECT tid, foo FROM {mytable} WHERE tid IN (:tids)', array(':tids' => array_keys($terms)));
+  $result = db_select('mytable', 'm')
+    ->fields('m', array('tid', 'foo'))
+    ->condition('m.tid', array_keys($terms), 'IN')

This is not necessary, no need to use db_select() here.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -152,9 +152,12 @@ function hook_taxonomy_term_create(\Drupal\taxonomy\Plugin\Core\Entity\Term $ter
-    $terms[$record->id()]->foo = $record->foo;
+    $terms[$record->vid]->foo = $record->foo;

tid, not vid.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -181,17 +184,8 @@ function hook_taxonomy_term_presave(Drupal\taxonomy\Term $term) {
+  if (empty($term->description)) {

Term is a NG entity now, so this should be $term->description->isEmpty(). I would also suggest to use something else, description will soon become a field.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -204,19 +198,11 @@ function hook_taxonomy_term_insert(Drupal\taxonomy\Term $term) {
+    'tid' => $term->tid,

@@ -230,7 +216,7 @@ function hook_taxonomy_term_update(Drupal\taxonomy\Term $term) {
-  db_delete('term_synoynm')->condition('tid', $term->id())->execute();
+  db_delete('mytable_index')->condition('tid', $term->tid)->execute();

id() was correct, ->tid is an object now.

orb’s picture

Status:Needs work» Needs review
StatusFileSize
new4.98 KB
PASSED: [[SimpleTest]]: [MySQL] 55,919 pass(es).
[ View ]

Fixed

orb’s picture

StatusFileSize
new5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 56,086 pass(es).
[ View ]

sorry, replaced by LF

cam8001’s picture

Status:Needs review» Needs work

Thanks for your help on this.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -37,12 +37,15 @@ function hook_taxonomy_vocabulary_create(\Drupal\taxonomy\Plugin\Core\Entity\Voc
function hook_taxonomy_vocabulary_load(array $vocabularies) {
-  foreach ($vocabularies as $vocabulary) {
-    $vocabulary->synonyms = variable_get('taxonomy_' . $vocabulary->id() . '_synonyms', FALSE);
+  $result = db_select('mytable', 'm')
+    ->fields('m', array('vid', 'foo'))
+    ->condition('m.vid', array_keys($vocabularies), 'IN')
+    ->execute();
+  foreach ($result as $record) {
+    $vocabularies[$record->vid]->foo = $record->foo;
   }

Is this example accurate, given that it doesn't return anything, and that $vocabularies is not passed by reference?

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -66,8 +69,8 @@ function hook_taxonomy_vocabulary_presave(Drupal\taxonomy\Plugin\Core\Entity\Voc
+  if ($vocabulary->vid == 'my_vocabulary') {

A vid is an int, I don't think it can be a string, so this could be a confusing example.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -152,9 +156,10 @@ function hook_taxonomy_term_create(\Drupal\taxonomy\Plugin\Core\Entity\Term $ter
  */
function hook_taxonomy_term_load(array $terms) {
-  $result = db_query('SELECT tid, foo FROM {mytable} WHERE tid IN (:tids)', array(':tids' => array_keys($terms)));
+  $result = db_query('SELECT tid, foo FROM {mytable} WHERE tid IN (:tids)',
+    array(':tids' => array_keys($terms)));
   foreach ($result as $record) {
-    $terms[$record->id()]->foo = $record->foo;
+    $terms[$record->tid]->foo = $record->foo;
   }

Same reference/return question as hook_taxonomy_vocabulary_load().

Berdir’s picture

@cam8001: Those cases are fine. vid is a machine name, not an int, it's a config entity. And the objects are by reference, that's how load hooks work. The only thing that I'm not sure about is if we invoke load hooks for config entities at all. If not then we should simply delete that hook documentation.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -80,10 +83,11 @@ function hook_taxonomy_vocabulary_insert(Drupal\taxonomy\Plugin\Core\Entity\Voca
+  db_insert('mytable')->fields(array(
+    'vid' => $vocabulary->vid,
+    'foo' => $vocabulary->foo
+  ))

->fields( should be on the next line, indented by two spaces, the array keys by 4.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -152,9 +156,10 @@ function hook_taxonomy_term_create(\Drupal\taxonomy\Plugin\Core\Entity\Term $ter
-  $result = db_query('SELECT tid, foo FROM {mytable} WHERE tid IN (:tids)', array(':tids' => array_keys($terms)));
+  $result = db_query('SELECT tid, foo FROM {mytable} WHERE tid IN (:tids)',
+    array(':tids' => array_keys($terms)));

I wouldn't touch things line at all, it's fine to be longer.

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -181,18 +186,11 @@ function hook_taxonomy_term_presave(Drupal\taxonomy\Term $term) {
+  db_insert('mytable')->fields(array(

Same here.

andypost’s picture

Status:Needs work» Needs review

A vid is an int

No! It's a string (machine name)

$vocabularies is not passed by reference?

See comment_node_load() as working example, we have this all over core

orb’s picture

StatusFileSize
new5.76 KB
PASSED: [[SimpleTest]]: [MySQL] 55,636 pass(es).
[ View ]

Fix indents

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -181,18 +186,11 @@ function hook_taxonomy_term_presave(Drupal\taxonomy\Term $term) {
+  db_insert('mytable')->fields(array(
andypost’s picture

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -80,10 +83,12 @@ function hook_taxonomy_vocabulary_insert(Drupal\taxonomy\Plugin\Core\Entity\Voca
+  db_insert('mytable')
...
+  ->execute();

@@ -181,18 +187,12 @@ function hook_taxonomy_term_presave(Drupal\taxonomy\Term $term) {
+  db_insert('mytable')
...
+  ->execute();

@@ -204,19 +204,12 @@ function hook_taxonomy_term_insert(Drupal\taxonomy\Term $term) {
+  db_insert('mytable')
...
+  ->execute();

@@ -230,7 +223,9 @@ function hook_taxonomy_term_update(Drupal\taxonomy\Term $term) {
+  db_delete('mytable_index')
...
+    ->execute();

execute should have 2 spaces indent like db_delete()

orb’s picture

StatusFileSize
new5.76 KB
PASSED: [[SimpleTest]]: [MySQL] 55,642 pass(es).
[ View ]

fix indent

podarok’s picture

Status:Needs review» Needs work
+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -80,10 +83,12 @@ function hook_taxonomy_vocabulary_insert(Drupal\taxonomy\Plugin\Core\Entity\Voca
+      'vid' => $vocabulary->vid,
+      'foo' => $vocabulary->foo
...
+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -181,18 +187,12 @@ function hook_taxonomy_term_presave(Drupal\taxonomy\Term $term) {
+      'tid' => $term->id(),
+      'foo' => $term->foo
...
+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -204,19 +204,12 @@ function hook_taxonomy_term_insert(Drupal\taxonomy\Term $term) {
+      'tid' => $term->id(),
+      'foo' => $term->foo

https://drupal.org/coding-standards#array
Arrays should be ended with comma

orb’s picture

StatusFileSize
new5.76 KB
PASSED: [[SimpleTest]]: [MySQL] 57,265 pass(es).
[ View ]

Fixed - comma.

orb’s picture

Status:Needs work» Needs review

needs review

podarok’s picture

Status:Needs review» Reviewed & tested by the community

#19 looks good for me
if bot happy - RTBC

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

Shouldn't this example use db_select() and condition() rather than db_query?

+  $result = db_query('SELECT tid, foo FROM {mytable} WHERE tid IN (:tids)',
+    array(':tids' => array_keys($terms)));

And in the Term section, one example changed:

-    $terms[$record->id()]->foo = $record->foo;
+    $terms[$record->tid]->foo = $record->foo;

But then the next couple of examples do things like:

+  db_insert('mytable')
+    ->fields(array(
+      'tid' => $term->id(),
+      'foo' => $term->foo,
+    ))
+    ->execute();

So the first change implies to me that id() is bad to use, but then the next example uses it? I don't think both of them can be correct.

Berdir’s picture

Assigned:orb» Unassigned
Status:Needs work» Reviewed & tested by the community

tid/id() are correct. $record is a database result row, which is a stdClass without methods. $term is a Term object with an id() method. That $record is currently using ->id() was a search & replace bug when terms were converted.

And our current standard is AFAIK still to use db_query() unless there is a reason to use db_select(), e.g. some dynamic parts. As I said above, that line should probably not be touched at all, but I have no strong opinion on that, so back to you for now :)

jhodgdon’s picture

Well, how come most of the example hook function bodies use db_select() and this one uses db_query()? If this logic applies to one example, it should apply to all. Can't we keep them consistent?

I also personally find db_select() a lot easier to read/parse than a bare SQL query with placeholders, so my preference would be to use db_select() in the examples for clarity, even if our coding standards recommend db_query().

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

Anyway... Let's either use a db_query() for both vocabulary and term load examples, or use db_select for both. You are right that our documentation says to use db_query() if performance is an issue:
https://drupal.org/node/310075

But the queries in the examples for vocabulary load and term load are nearly identical. One is using db_query, and the other is using db_select. They can't both be the right thing to do. Let's make them the same.

orb’s picture

Status:Needs work» Needs review
StatusFileSize
new5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 56,410 pass(es).
[ View ]

1. Change db_select

2. Change replaced $vocabulary->vid of $vocabulary->id()

andypost’s picture

Status:Needs review» Reviewed & tested by the community

I think it's ready to go

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

Um... I thought in this case we were supposed to use the id() method and not ->vid?

function hook_taxonomy_vocabulary_insert(Drupal\taxonomy\Plugin\Core\Entity\Vocabulary $vocabulary) {
-  if ($vocabulary->synonyms) {
-    variable_set('taxonomy_' . $vocabulary->id() . '_synonyms', TRUE);
+  if ($vocabulary->vid == 'my_vocabulary') {
+    $vocabulary->weight = 100;
   }
}
orb’s picture

Status:Needs work» Needs review
StatusFileSize
new5.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,051 pass(es).
[ View ]

Fixed

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

It's the same, but agreed that id() is better here.

jhodgdon’s picture

Assigned:Unassigned» jhodgdon

Thanks! I'll get this one committed soon. Consistent docs++

jhodgdon’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks all! Committed to 8.x.

I think we could use a backport of this to 7.x? It would need a bit of work because taxonomy/vocabulary are not classes in D7.

andypost’s picture

Should be the same except ->id() become vid

Berdir’s picture

and vid would actually be an integer, so you might want to use ->machine_name for the conceptually same check there.

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
orb’s picture

Assigned:Unassigned» orb
orb’s picture

Assigned:orb» Unassigned
Status:Patch (to be ported)» Needs review
StatusFileSize
new4.06 KB
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es).
[ View ]

ported to Drupal 7

andypost’s picture

Status:Needs review» Reviewed & tested by the community

D7 have no hook_taxonomy_vocabulary_create()

jhodgdon’s picture

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work

Um... in the term_update and vocabulary_update hooks, shouldn't we be updating a record rather than inserting a new one? It seems like the tid/vid is probably the primary key, so we'd either need to delete the previous record before inserting, or use db_update() rather than db_insert()?

We did that in the 8.x patch too... guess we need to go back and fix it there?

orb’s picture

Assigned:Unassigned» orb
orb’s picture

Assigned:orb» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.11 KB
PASSED: [[SimpleTest]]: [MySQL] 57,576 pass(es).
[ View ]

Fix for Drupal 8

jhodgdon’s picture

Thanks! Looks good to me; anyone else want to give it a quick look?

sergeypavlenko’s picture

Status:Needs review» Reviewed & tested by the community

Thank you. Good code!

alexpott’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed 025aa2a and pushed to 8.x. Thanks!

Back to 7.x

orb’s picture

Assigned:Unassigned» orb
orb’s picture

Assigned:orb» Unassigned
Status:Patch (to be ported)» Needs review
StatusFileSize
new4.04 KB
PASSED: [[SimpleTest]]: [MySQL] 40,331 pass(es).
[ View ]

Port Drupal7

jhodgdon’s picture

Assigned:Unassigned» jhodgdon
Status:Needs review» Reviewed & tested by the community

Thanks orb! That looks like the right patch for D7, and I'll get it committed shortly.

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed

Committed to 7.x. Thanks again everyone!

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