1. There are problems with deletion of taxonomy terms (term that has been deleted, stored in cache (see attachment img)).
2. Changing taxonomy term weight or parent doesn't work (drupal_static should be reset).
2. Rebuilding the cache does not work correctly (if it concerns of taxonomy terms with parents).

This creates a lot of problems and should be fixed.
Patch attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alan-ps’s picture

FileSize
2.45 KB

drupal_static_reset should be only for taxonomy_get_tree.
Replaced drupal_static_reset() to drupal_static_reset('taxonomy_get_tree').

nevergone’s picture

Status: Needs review » Reviewed & tested by the community

#1 tested and works well!

joelpittet’s picture

Got bit by this one, so bumping.

joelpittet’s picture

and a big +1 from me for the RTBC. This fixed a nasty error page!

CatherineOmega’s picture

Looks good! Can we roll a new version?

pcambra’s picture

+++ b/shs.module
@@ -980,7 +976,7 @@ function shs_form_taxonomy_overview_terms_submit(&$form, &$form_state) {
-    array_unshift($form['#submit'], 'shs_form_taxonomy_form_term_submit');
+    $form['#submit'][] = 'shs_form_taxonomy_form_term_submit';

I don't think this is needed, if there's some other submit handler it will be stripped, won't it?

joelpittet’s picture

@pcambra that is just moving it from the front to the end. It won't strip anything out.

l0ke’s picture

Since upgrading to dev was not a good option for me, I've made port to 7.x-1.6.
Just want to share if someone will need it.

joelpittet’s picture

Small bump, #1 is still RTBC. I'm hiding #8 as it may confuse the maintainer.

stBorchert’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/shs.module
    @@ -967,11 +964,10 @@ function shs_form_taxonomy_overview_terms_alter(&$form, &$form_state, $form_id)
    -  shs_term_get_children($form_state['complete form']['#vocabulary']->vid, 0, array('node_count' => TRUE), TRUE);
    +  shs_term_get_children($form['#vocabulary']->vid, 0, array('node_count' => TRUE), TRUE);
    

    This does not work if you reset the vocabulary to alphabetic order. $form['#vocabulary'] does not exists in that case.

  2. +++ b/shs.module
    @@ -980,7 +976,7 @@ function shs_form_taxonomy_overview_terms_submit(&$form, &$form_state) {
    -    array_unshift($form['#submit'], 'shs_form_taxonomy_form_term_submit');
    +    $form['#submit'][] = 'shs_form_taxonomy_form_term_submit';
    

    Can you explain, why you move the submit handler to the end?

  3. +++ b/shs.module
    @@ -988,18 +984,7 @@ function shs_form_taxonomy_form_term_alter(&$form, &$form_state, $form_id) {
    +  shs_term_get_children($form['#vocabulary']->vid, 0, array('node_count' => TRUE), TRUE);
    

    Why do you remove the logic of updating the terms parents here?
    Try moving a term to a different point within the hierarchy and select one of its former parents within the widget ...

joelpittet’s picture

@stBorchert it may be tricky to get responses to this, the original patch was in Sept 2014.

re #4 I came in because there was a traceable fatal error that was mitigated by this patch. If I recall correctly, it was with article tags, and it would fatal when adding or removing more than one tag on the field.

If I had to guess on #10.2 it would be moved to the end to let the normal taxonomy form submit handlers run first and possibly prime the cache.

I'll see if I can reproduce the problems you indicate with #10.1 and #10.3, as I haven't run into those yet. But it was very easy to hit that fatal as my client did it by adding a couple article tags.

joelpittet’s picture

Wish I could tell my past self to write down notes on the issue... I can't reproduce the problem. It could have manifested with cache_consistent + redis which I was using back then.

alan-ps’s picture

I'will try to remember what this patch do and explain in short:)

So, the original problems:

(1) Term that has been deleted is stored in a cache. Steps to reproduce:
- Create term "Tag A" and add it to a node;
- Remove term "Tag A".
- Edit node. Term is still present in a field.

In order to fix it we should reset static variable (taxonomy_get_tree) before shs_term_get_children() will be executed:

+  // Reset centrally stored static variables.
+  drupal_static_reset('taxonomy_get_tree');

It won't work, because shs_form_taxonomy_form_term_submit() is executed before the original submit taxonomy_form_term_submit(), so:

#10.2

-    array_unshift($form['#submit'], 'shs_form_taxonomy_form_term_submit');
+    $form['#submit'][] = 'shs_form_taxonomy_form_term_submit';

and this (#10.3) in order to rebuild the shs cache:

 function shs_form_taxonomy_form_term_submit(&$form, &$form_state) {
-  // Update vocabulary cache for the terms parents.
-  $parents = db_select('taxonomy_term_hierarchy', 'tth')
-          ->fields('tth', array('parent'))
-          ->condition('tid', $form_state['term']->tid)
-          ->execute()
-          ->fetchAll();
-  if ($parents) {
-    // Update vocabulary cache for the terms parents.
-    foreach ($parents as $parent) {
-      shs_term_get_children($form_state['term']->vid, $parent->parent, array('node_count' => TRUE), TRUE);
-    }
-  }
+  shs_term_get_children($form['#vocabulary']->vid, 0, array('node_count' => TRUE), TRUE);
 }
Why do you remove the logic of updating the terms parents here?
Try moving a term to a different point within the hierarchy and select one of its former parents within the widget ...

This won't break nothing. This is executed, after the confirmation of deleting taxonomy term.

Try moving a term to a different point within the hierarchy and select one of its former parents within the widget ...

This is related to shs_form_taxonomy_overview_terms_submit() and this is the second problem.

(2) Rebuilding the cache does not work correctly (if it concerns of taxonomy terms with parents).

The patch is fixed this problem (it doesn't work with original code).

About #10.1:
I didn't remember why I made this change. It seems you are right and it can cause a notice, if we will use "Reset to alphabetic". But from other side $form_state['complete form']['#vocabulary']->vid won't work for "Reset to alphabetic" too. This will avoid notices only.

So, I've prepared new patch, which corrects #10.1, it should work like a charm. I guess, I missed nothing:)

alan-ps’s picture

Status: Needs work » Needs review
stBorchert’s picture

Status: Needs review » Needs work

Q: Why do you remove the logic of updating the terms parents here?
A: Try moving a term to a different point within the hierarchy and select one of its former parents within the widget ...
This won't break nothing. This is executed, after the confirmation of deleting taxonomy term.

If you edit a term and select a new parent for the term the cache for all parents within the hierarchy needs to be rebuild. I guess this was the reason why the submit hook needs to run first. Unfortunately I can't remember :)
Whoa, every time I see this "node-count" thing it makes me want to simply remove it completely (as done in 8.x). This is so ugly in terms of caching :(

alan-ps’s picture

Why do you remove the logic of updating the terms parents here?
Try moving a term to a different point within the hierarchy and select one of its former parents within the widget ...

hmm, probably I don't understand something but it is executed after deletion of a taxonomy term (I mean this function shs_form_taxonomy_form_term_submit()). We can't do this "moving a term to a different point" here. This is a part of code where submit is added:

function shs_form_taxonomy_form_term_alter(&$form, &$form_state, $form_id) {
  if (isset($form_state['confirm_delete']) && isset($form_state['values']['vid'])) {
    // Add custom submit handler to update cache.
    $form['#submit'][] = 'shs_form_taxonomy_form_term_submit';
  }
}
If you edit a term and select a new parent for the term the cache for all parents within the hierarchy needs to be rebuild.

It works fine. I checked different ways. Try to apply patch;)
I don't think that there is some problems with cache:)

  • stBorchert committed de3faca on 7.x-1.x
    Issue #2344653 by charlie-s, alan-ps, l0ke, joelpittet: SHS cache system...
stBorchert’s picture

Status: Needs work » Fixed

Committed some other changes that should fix this too.

Status: Fixed » Closed (fixed)

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