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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: 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
CreditAttribution: dawehner commented
FileSize
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 CreditAttribution: pwolanin commented
Status: Active » Needs work

use FALSE

lilou’s picture

lilou CreditAttribution: lilou commented
FileSize
drop.patch488 bytes
lilou’s picture

lilou CreditAttribution: lilou commented
FileSize
taxonomy.module.patch938 bytes

+doxygen

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Looks great, thank you.

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

lilou’s picture

lilou CreditAttribution: lilou commented

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

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: 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 CreditAttribution: Damien Tournoud commented

By the way, thanks lilou.

Dries’s picture

Dries CreditAttribution: Dries commented

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

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: pwolanin commented
Status: Reviewed & tested by the community » Patch (to be ported)

still need to go to D6

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
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
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Looks good, thanks, committed.

Anonymous’s picture

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

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