Use of hook implementations outside of the module file [#279171]

Hi,

This error will occur in only certain Drupal installations with specific modules. I have Given one such instance which would cause his error

Drupal 5.7
Modules - Views 5.x.1.6, Views Bulk Operations and voting api

Description:
View uses a hook called "views_tables". Once views, gets the list of modules implementing this hook, it caches it in a static variable for that request (as well as in the database for future request).

When the init hook for GR is invoked, it calls "_menu_item_is_accessible(menu_get_active_item))" to check for access. This in turn causes a lot of function calls, resulting in views module trying to collect the list of modules implementing "views_tables".

In my setup , the problem with this scenario is,

The init hook for GR is called before the init hook of the voting api module. The voting api module implements the hook "views_tables" in a seperate file and includes it using the votingapi_init hook.

When views module build the table list, it does not see the voing api modules table, since votingapi_init is not yet invoked.
So view caches the wrong table list, which is missing a few tables created by other modules like voting api. This causes incomplete queries, resulting in errors like

user warning: Unknown column 'votingapi_cache_vote_percent_average.value' in 'field list' query: SELECT node.nid, node.created AS node_created_created, node_data_field_images.field_images_fid AS
...

Solution:

Increase the weight of GR module to allow the init hooks of all other modules before GR.

Comments

Comment #1

nicholasthompson

English

commented 7 July 2008 at 08:52

Status:

Active

» Postponed (maintainer needs more info)

I see!

Hmm this is a problem.

On one hand, increasing the module's weight SHOULD solve this - but then on the flip side it means more of the site "boots up" even though it might need a redirect which isn't really very efficient.

Also... Is it not bad practice to put hook implementations into a separate include file? If you wanted to do it that way shouldn't you implement the hook in the module and the implementation THEN includes the other file and calls a private function?

I honestly don't want to shift the blame or anything as I'm perfectly happy to look for a solution, however it does seem (to me at least) bad practice to have your hook implementation in a file which needs including.

I suppose the alternative is to find out how to test a callbacks access permission for the current user without invoking hook_menu...

Comment #2

mulogic commented 7 July 2008 at 14:54

Hi,

yes, You are correct. it would be a good practice to implement hooks in the module file. However, I am not the author of any of these modules. I just using them for my site. I can post a suggestion to the voting api module, explaining the issue.

Comment #3

nicholasthompson

English

commented 7 July 2008 at 15:02

Title:	Module weight of GR	» Use of hook implementations outside of the module file
Project:	Global Redirect	» Voting API
Version:	5.x-1.3	» 5.x-1.x-dev
Priority:	Normal	» Critical

According to the report by mulogic, VotingAPI appears to have a hook implementation which is stored in an include file. This means that anything pre-init (or pre-votingapi-init) that tries to access that hook wont find it.

Basically GlobalRedirect uses the menu system to check if the current access is valid. This appears to cause hook_menu to be invoked. In turn this causes Views to lookup which modules use hook_views_tables... But VotingAPI hasn't had its hook_init run yet so the include hasn't been loaded up and therefore the hook isn't registered.

Is it not better practice to have all the hook implementations in the main module? If you wish to move large blocks of code out to an include file, shouldn't the hook implementation function include the file (include_once?) and then execute a private function?

Would appreciate any feedback you have here...