hook_custom_theme sometimes isn't called resulting in fatal error [#1182190]

FB module seems to abuse the use of hook_custom_theme to load its settings, according to the doxygen. It seems that sometimes though the hook isn't invoked, specifically when the site is in maintenance mode. This causes later calls to FB code to fail because the fb_settings function never gets defined, and a fatal error results.

Comment	File	Size	Author
#1	fb-settings-error-1182190-no-prefix.patch	478 bytes	Steven Jones

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Steven Jones CreditAttribution: Steven Jones commented 8 June 2011 at 09:46

Status:

Active

» Needs review

File	Size
fb-settings-error-1182190-no-prefix.patch	478 bytes

Here's a patch that calls our implementation manually if it looks like it hasn't been called.

Comment #2

Dave Cohen CreditAttribution: Dave Cohen commented 8 June 2011 at 16:23

Thanks for report and patch. As for the "abuse" I blame Drupal's api! If you have a better approach let me know.

Comment #3

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 27 March 2012 at 15:34

Why not use hook_init() or hook_boot()? The earlier patch effectively moves the contents of fb_custom_theme() into fb_init() when the site is in maintenance mode.

Comment #4

Dave Cohen CreditAttribution: Dave Cohen commented 29 March 2012 at 18:16

Status:

Needs review

» Reviewed & tested by the community

Modules like fb_canvas.module need to implement hook_custom_theme(), and when implementing it, they need information from fb.module.

Why name it "hook_init()" if it's not the first hook called? Drupal's API is the one inflicting abuse.

I will get this patch in.

Comment #5

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 3 April 2012 at 14:34

What about moving the code to hook_boot, which is run before hook_custom_theme (I'm not sure if that's a good solution, but it would stop the abuse of hook_custom_theme).

hook_init is called after hook_custom_theme so that you can do theme-specific initialisation. I agree it's not clear though. I'm going to file a documentation bug as currently the docs imply caching is the only difference between the functions.

Comment #6

Dave Cohen CreditAttribution: Dave Cohen commented 3 April 2012 at 17:29

hook_init is called after hook_custom_theme so that you can do theme-specific initialisation.

Why can't you do theme-specific initialization after hook_init()?

What about moving the code to hook_boot...

My understanding is that modules should avoid hook_boot as it makes them incompatible with caching. My understanding is limited. I only moved that code into hook_custom_theme() because something was broken otherwise.

Comment #7

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 4 April 2012 at 08:43

Why can't you do theme-specific initialization after hook_init()?

Agreed, it would make more sense if there were separate functions.

My understanding is that modules should avoid hook_boot as it makes them incompatible with caching.

I'm new to Drupal, but I can't see anything to suggest that would be the case. It looks like Drupal disables external page caching by default (http://drupal.org/node/804864 ), but I don't think modules defining hook_boot() has any impact on that. Obviously the code in hook_boot itself will be run on cached pages which may not be desireable.

Comment #8

Dave Cohen CreditAttribution: Dave Cohen commented 4 April 2012 at 09:29

Only use hook_boot() if your code must run even for cached page views

http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

The performance admin page warns about the modules that implement this hook, if I recall correctly. I'd rather implement hook_custom_theme() than hook_boot().

I think of hook_custom_theme() as poorly named. I consider it the first hook_init(). I have no idea why people thought it was a good idea to add a hook before hook_init().