Run Skinr first to avoid problems with other modules during hook_init

This issue (if related to module_invoke_all('skinr_data') ) does not apply to the 2.x branch of skinr, only the 1.x branch.

Either way, I believe a better approach to this problem would be to check in skinr_init() was run at the top of the preprocess rather than changing the weight of skinr (through use of a static variable, perhaps). At least in the 7.x branch of skinr I've run into issues with the module weight, so better not to take any chances.

Please review these patches.

Committed to CVS.

Need to port this to D7.

And here's the ported patch.

+++ skinr.module	Wed Oct 20 10:33:31 EDT 2010
@@ -21,8 +21,12 @@
+  static $run = FALSE;

Shouldn't this be done with drupal_static()? If not, can you please explain why &drupal_static() wouldn't be used here instead? Thanks :)

Powered by Dreditor.

From api pages: All functions requiring a static variable to persist or cache data within a single page request are encouraged to use this function unless it is absolutely certain that the static variable will not need to be reset during the page request.

So this patch should be good.

1. Adding a static cache to Skinr's hook_init() does not solve anything. neochief already outlined that as well as the underlying cause in his *awesome* bug report. (Thanks!)
2. Since the module_implements() cache has been backported from D7, this bug report applies to all branches, including 7.x-2.x.
3. For D7, the solution is simple: Move that code from hook_init() into hook_module_implements_alter().
4. For D6, there is no simple workaround. Adjusting the module's weight makes most sense to me. I can't think of a better solution, since D6 is quite limited with regard to hooks that run before hook_init() [none].

   However, a weight of -1000 looks a bit too extreme to me. -10 or -100 would be more than sufficient. Elsewhere, I used -1, due to a similar invocation order problem, which is sufficient, because I don't know of any other module that uses a negative module weight. So -10 should work.

Here's a stab at that patch for D7.

Thanks @moonray. Could you also provide a little more info with the patches? A brief description or summary of how the patch addresses each of the points and would be very helpful.

Thanks again!

Description for #13: Move that code from hook_init() into hook_module_implements_alter().

+++ skinr.module
@@ -26,6 +26,20 @@ function skinr_init() {
+ * Implementation of hook_module_implements_alter().

Should be: "Implements hook_module_implements_alter()"

+++ skinr.module
@@ -26,6 +26,20 @@ function skinr_init() {
+  if ($hook == 'init') {

A comment should be added explaining why this is necessary.

Powered by Dreditor.

Here's a patch that does that.

Just out of curiosity (not disputing): why is it Implements instead of Implementation?

Coding standards: http://drupal.org/node/1354#hookimpl :)

If you approve of the patch, please mark it "Reviewed & tested by the community" and I will commit it.

hmm, Couldn't we just move skinr to the beginning instead of looping thru it?

function skinr_module_implements_alter(&$implementations, $hook) {
  // Run Skinr first to avoid issues with other modules during hook_init().
  if ($hook == 'init') {
    $skinr = array('skinr');
    $skinr['skinr'] = $implementations['skinr'];
    unset($implementations['skinr']);
    array_unshift($implementations, $skinr);
  }
}

I didn't test this but just curious. It seems like a loop to manipulate the array is a bit overkill.

the problem is that it's a keyed array, and it's passed by reference. I don't think array_unshift lets you add by key.
What you'd end up with your example is:

<?php 
array(
0 => array(
'skinr' => FALSE,
),
'module_a' => FALSE,
'module_b' => FALSE,
);
?>

hmm, @moonray: Yep, array_unshift sucks. lol

Ok this should work.

<?php
function skinr_module_implements_alter(&$implementations, $hook) {
  // Run Skinr first to avoid issues with other modules during hook_init().
  if ($hook == 'init') {
    $skinr['skinr'] = $implementations['skinr'];
    unset($implementations['skinr']);
    $implementations = array_merge($skinr, $implementations);
  }
}
?>
 
lol, I just feel like it should be accomplish without the loop. :)

Because implementations is passed by reference, does that work properly? Have you tested it?

And here's #23 in patch form. I've tested it, and it works as advertised.
Perhaps we can get a second opinion before RTBC?

Did you forget to attach the patch?

I need to get me a mac desktop. :-/ I always seem to leave my computer somewhere lol.

Here's a patch,. uhhm, I wrote the patch but it should applied :-/

nice, the bot will tell me if it applies correctly (sweet)

edit: @moonray, I also tested this. Works as expected it makes skinr run first in the hook_init.

Oops, wrong patch :-/, Sorry for spamming the issue queue.

OK, let's try and actually attach the patch. :)

Committed! Thanks :)

http://drupal.org/cvs?commit=443780

Needs to go to 6.x next.

This code will not work for D6. There is alternative code in place that fixes this bug already. It's not ideal, but changing the weight of the module might cause other problems. I believe we should leave this as is.