Rewrite examples in taxonomy.api.php and remove references to synonyms

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

Initial patch.

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

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?

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

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

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

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.

Fixed

sorry, replaced by LF

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

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

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

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(

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

fix indent

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

Fixed - comma.

needs review

#19 looks good for me
if bot happy - RTBC

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.

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

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

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.

1. Change db_select

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

I think it's ready to go

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;
   }
 }

Fixed

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

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

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.

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

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

ported to Drupal 7

D7 have no hook_taxonomy_vocabulary_create()

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?

Fix for Drupal 8

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

Thank you. Good code!

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

Back to 7.x

Port Drupal7

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

Committed to 7.x. Thanks again everyone!