If you're in database 1, which has vocabs A, B, and C, and then you switch to database 2, which has vocabs E, F, and G, modules expecting to find vocabs E, F, and G are going to be mighty surprised when they get A, B, and C back.

This patch adds the ability to reset this function's static variable cache.

CommentFileSizeAuthor
#8 vocabulary_static_reset.patch2.54 KBcatch
#7 vocabulary_static_reset.patch4.1 KBcatch
#7 vocabulary_static_reset.patch2.48 KBcatch
#6 vocabulary_static_reset.patch5.68 KBcatch
#5 vocabulary_static_reset.patch5.71 KBcatch
#1 taxonomy-reset-vocabulary-cache-296910-1.patch932 byteswebchick

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Active » Needs review
StatusFileSize
new taxonomy-reset-vocabulary-cache-296910-1.patch932 bytes
moshe weitzman’s picture

moshe weitzman commented

any chance you can add same for taxonomy_get_tree?

dries’s picture

dries commented

Where is the test case? :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Dag nabbit. ;) I also should document the new parameter. I was in a hurry.

I actually don't know how to write a test case for this. :( The only thing I can think is to do a db_set_active to the parent Drupal site, add some terms there, then db_set_active() back to the test instance's database and add some terms there, and make sure the ones I get back are the proper ones. But currently, all of our test runs are isolated from the main database and I don't really want to break that rule. And I'm not sure otherwise how to get at that static $vocabularies variable in order to verify that it's been reset when I ask it to.

Any ideas?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new vocabulary_static_reset.patch5.71 KB

Here's an updated patch and test for this. Patch includes fixes to #244662: Vocabulary load returns NULL as well because these will both be in the taxonomy_vocabulary_load() test case. I'll re-roll when that gets in.

We may want to add this reset into taxonomy_save_vocabulary() - but if we do that then I can't use it as easily for the test ;) - remove the TRUE argument from taxonomy_vocabulary_load to reproduce the bug. iirc, there's other issues with taxonomy_save_vocabulary() - so I might want to leave changes there for (yet another) followup patch.

catch’s picture

catch
he/him
Primary language English
commented
Assigned: Unassigned » catch
Priority: Normal » Critical
StatusFileSize
new vocabulary_static_reset.patch5.68 KB

Now that #244662: Vocabulary load returns NULL is in, here's an updated patch for the $reset. Since forum.test contains hacks to avoid failing without this, I'm marking as critical. forum.test cleanups will be attached to a new issue per webchick's request on the previous one shortly.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new vocabulary_static_reset.patch2.48 KB
new vocabulary_static_reset.patch4.1 KB

And the right patch.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new vocabulary_static_reset.patch2.54 KB

Only reset the cache for the vid that's being loaded. All taxonomy tests pass.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

This looks really great. And that static is blocking me from writing a test for #277200: Add tests for vocabulary hierarchy.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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