The taxonomy_vocabulary_load() function returns NULL on non-existing vocabularies, where it should return FALSE.

As a consequence, pretty errors are displayed when calling /admin/content/taxonomy/xxx on a non existing vocabulary.

CommentFileSizeAuthor
#14 taxonomy-ret-false-277214-14.patch1.03 KBpwolanin
#5 taxonomy.module.patch938 byteslilou
#4 drop.patch488 byteslilou
#2 drop.patch492 bytesdawehner

Comments

damien tournoud’s picture

damien tournoud commented
Title: taxonomy_vocabulary loader should return FALSE on non-existing vocabularies » DROP task - all menu loaders should return FALSE on object not found
Component: taxonomy.module » menu system

Registered this as DROP task #107, maybe other menu loaders don't properly return FALSE.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new drop.patch492 bytes

here is the patch
for this small issue // i will look for more things like this tomorrow morning

pwolanin’s picture

pwolanin commented
Status: Active » Needs work

use FALSE

lilou’s picture

lilou commented
StatusFileSize
new drop.patch488 bytes
lilou’s picture

lilou commented
StatusFileSize
new taxonomy.module.patch938 bytes

+doxygen

damien tournoud’s picture

damien tournoud commented

Looks great, thank you.

Can we check that all other _load() functions correctly return FALSE?

lilou’s picture

lilou commented

I check all _load() function and I haven't find NULL return.

damien tournoud’s picture

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

Ok, so this looks ready to go in.

damien tournoud’s picture

damien tournoud commented

By the way, thanks lilou.

dries’s picture

dries commented

Is checking the return value of _load() functions something we can write a test for?

damien tournoud’s picture

damien tournoud commented

@Dries: we need to think a bit about that.

With static code analysis, it would be easy to cover all cases. On our testing framework, we could just call each _load function with a dummy value (NULL, for example) and verify that FALSE is returned. That's not perfect but it can provide a safety net for future regressions. I'll check if that's possible / easy to do.

dries’s picture

dries commented
Version: 7.x-dev » 6.x-dev

I've committed this to CVS HEAD. Should probably be back-ported to DRUPAL-6.

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Patch (to be ported)

still need to go to D6

pwolanin’s picture

pwolanin commented
Status: Patch (to be ported) » Reviewed & tested by the community
StatusFileSize
new taxonomy-ret-false-277214-14.patch1.03 KB

Patch needed to be re-rolled from the drupal root dir, but applies and works.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Looks good, thanks, committed.

Anonymous’s picture

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

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