on admin/block => style.css is imported twice

I don't really think this is that big a deal. Sending the style sheet twice doesn't hurt anything.

Not sure I like that code. I think it can be done more elegantly.

This bug happens because init_theme() is called twice, which can be fixed by declaring theme as global in that function. But also I think it is nice to prevent duplicated stylesheets, which could happen when you have a pair blocks from the same module adding both the same stylesheet.

This patch addresses both issues, being kind of a double bug fixing.
- prevents init_theme() from being called twice defining $theme as global.
- prevents stylesheets from being added twice by keeping them in an indexed array.

Big +1 on the indexed array. I like PHP arrays. :-) For the other, how exactly does making it global fix anything? And could it be fixed by making it static instead of global? global variables should be used only as a last resort. If you need something to keep its value between calls of the function, just declare it static. Drupal does that a lot and it's a very simple and effective way of caching data.

OK, I kinda see now. :-) I checked the references page for init_theme().

block_admin_display() calls init_theme() unconditionally, but doesn't save it anywhere. Currently, that means $theme isn't saved at all, but $theme_key is, which is what that function uses. That's where the double-call to init_theme() is coming from, I suspect.

theme() calls init_theme() inside an if ($theme === NULL), but it takes $theme back as a return value and saves it to the global $theme itself. It also uses $theme_engine, which is a global.

_block_rehash() calls init_theme() inside an if (empty($theme_key)) and then doesn't use the rest of the assorted $theme values.

So... this patch causes the buggy behavior to go away, and therefore I'm going to set it to ready to commit.

However!

The root cause is actually the inconsistent usage of init_theme(). It's used 3 times in 3 different ways that don't really mesh well at all. That should be cleaned up. I see three options:

1) Make $theme et al global (as this patch does) and have init_theme() return nothing, just initialize the globals. Then each function that needs one of those variables declares it global, checks it with isset() (which we keep being told is the fastest method), and then if needed calls init_theme().

2) Make $theme et al global and have init_theme() itself track whether the values are set. If they are, it just returns and does nothing. If they aren't, it sets the globals appropriately. Other functions can then blindly call init_theme() and init_theme() will be smart about not doing too much work.

3) Remove the globals and make init_theme() return an associative array of the three $theme variables ($theme, $theme_key, $theme_engine). Functions that need that data can then just call init_theme() and take the return values and use them. init_theme() would use static caching to avoid calculating each value more than once, a technique that we use a LOT in Drupal.

My knee-jerk preference would be for #3, on "globals are bad, m'kay?" grounds. However, I'm not sure what all else uses those globals at the moment, and that has the largest potential for breaking something. #2 would have the least overall impact and still be reasonably clean, since the avoid-repeating-yourself logic is contained in one place. #1 may be marginally the fastest, since it eliminates a short function call, but I don't think any of these functions are called often enough for it to make a serious difference.

So after this patch is in, I suggest we implement #2. Thoughts?

I suggest we go with #2 as a quick fix, and that we investigate #3 after #2 landed. Can you look into this, and update Jose's patch accordingly? Thanks!

Yeah, I figured I'd get drafted for this. :-)

Attached patch implements both the patch in comment #4 and cleanup option #2. While working on it I realized that while only those few functions call init_theme(), theme() is called a bajillion times, which in turns means a bajillion calls to init_theme() only to have it return immediately. I did some quick benchmarking both with and without theme() doing its own isset() check to avoid the function call, by going to admin/block and hitting "clear cache" repeatedly.

Without isset() check in theme():
1148.9 ms
1026.74
923.14
1003.07
1009.89
Average: 1022.348

With isset() check in theme():
954.65 ms
922.94
986.09
936.48
975.16
Average: 955.064

Difference: ~5%

For performance reasons, therefore, I left an isset() check in theme() to avoid the useless function call. That also means that cleanup option #3 may not be as good an idea afterall, as global + isset() is faster than calling init_theme() every time to get the same value back. I suppose static caching could work too, though. Eh, deal with that later. For now, try the patch. :-)

Committed to HEAD. Thanks.