I have attached a one line patch for taxonomy_vocabulary_load. The function uses isset() to test the variable $vocabularies[$vid], initializes the variable to FALSE (to which isset returns TRUE), then tests it using empty() to determine the return value of NULL.

The function seems to be called multiple times when loading a module. The first time through the variable is set to false (because there are no rows in the database) and the function returns NULL (because false is considered empty). On subsequent calls the variable passes the isset() condition (preventing it from being loaded from the database) and the function again returns NULL.

Another one line patch might be to initialize the variable to NULL (instead of FALSE). This value would be considered both !isset() and empty().

To my knowledge, SimpleTest calls standard drupal API functions to load a module and, thus, I would expect this to be an issue outside of the testing environment. Is that correct?

See issue #4 at http://drupal.org/node/244602.

CommentFileSizeAuthor
#11 taxonomy_isset_plus_test.patch4.53 KBcatch
#9 taxonomy_isset_plus_test.patch4.53 KBcatch
#8 taxonomy_isset_plus_test.patch4.71 KBcatch
#7 taxonomy_isset_plus_test.patch4.58 KBcatch
#5 vocabulary_load_fixes.patch4.74 KBcatch
#4 vocabulary_load_fixes.patch4.8 KBcatch
taxonomy_vocabulary_load.patch744 bytessolotandem

Comments

lilou’s picture

lilou commented

Still applied.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Still applies, looks like a simple mis-use of isset to me.

webchick’s picture

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

Fix looks good, but let's fix forum.test while we're at it.

catch’s picture

catch
he/him
Primary language English
commented
Title: Vocabulary load returns NULL » Vocabulary load returns NULL and needs a static $reset
Assigned: solotandem » catch
Status: Needs work » Needs review
StatusFileSize
new vocabulary_load_fixes.patch4.8 KB

Done, but the test was also going to fail in one spot due to taxonomy_vocabulary_load() not having a $reset argument for the static, so added that here too.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new vocabulary_load_fixes.patch4.74 KB

Removed an errant drupalGet

webchick’s picture

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

Oh, my. :) That took a fair bit more code than I was counting on. Great job, though!

+    $cur_settings = taxonomy_vocabulary_load($vid, TRUE);

Hm. I don't know that we should do this here. Doesn't it make more sense for taxonomy_save() or whatever should do this automatically whenever terms are added, updated, or deleted? It seems like if we keep the logic in the calling code, we're going to end up with an awful lot of duplicated lines like this.

+function taxonomy_vocabulary_load($vid, $reset = FALSE) {

We need PHPDoc on that new function parameter.

Incidentally, in #296910: Allow resetting of static cache for taxonomy_vocabulary_load() where the $reset param was brought up originally, Dries noted that we ought to have a test case for this. :P

So, the question becomes, how do we solve this tangled rats' nest of inter-dependent issues in a way that leaves baby kittens unharmed? Seems to me the fastest way out is:

1. Remove forum.test clean-up from this patch and keep it just at the isset/empty switch, but add a test to taxonomy.test that verifies the original bug is fixed.
2. Add the reset parameter to taxonomy_vocabulary_load (+test) over in #296910: Allow resetting of static cache for taxonomy_vocabulary_load().
3. Start a new issue that cleans up forum.test with the changes made here.

Sound sane?

catch’s picture

catch
he/him
Primary language English
commented
Title: Vocabulary load returns NULL and needs a static $reset » Vocabulary load returns NULL
Status: Needs work » Needs review
StatusFileSize
new taxonomy_isset_plus_test.patch4.58 KB

Who's eating kittens now eh? ;)

Forum cleanup is out, fix left in, new taxonomy test (which I had to put in an entire class to itself because the lack of vocabulary reset was killing subsequent test functions - but then a test cases just for regressions is probably fine here).

This should cover 1. Hopefully see you soon over at 2 and 3.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new taxonomy_isset_plus_test.patch4.71 KB

Renamed the test case per discussion in irc.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new taxonomy_isset_plus_test.patch4.53 KB

grr.

chx’s picture

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

Looks very good to me.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new taxonomy_isset_plus_test.patch4.53 KB

Damz spotted a minor code style issue in irc, re-rolled.

pwolanin’s picture

pwolanin commented

The code looks ok - the downside of course is that the query may be run extra times - but until there is a reset parameter I think this code is fine.

dries’s picture

dries commented

This all looks RTBC to me, but I'll let Angie do the final review as it seems like she did a deep dive in #6.

webchick’s picture

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

Yep! :)

Committed, thanks!

Anonymous’s picture

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

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