The fact that you're using hook_boot() is causing problems because language_cookie_set() is calling drupal_alter('language_cookie', $cookie) and that itself is then calling module_implements() which is now happening far too early in Drupal's bootstrap. An example is the pathauto module where when language_cookie got installed and then cache got cleared again, the module_invoke_all('pathauto', 'settings') does no longer get hooks for modules that get loaded later than language_cookie.

The solution to the problem is quite simple: rename language_cookie_boot() to language_cookie_exit() and the result will be exactly the same and you'll never get close to breaking anything else anymore.

CommentFileSizeAuthor
#5 2463727-3.patch722 bytesstefan.r
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jurgenhaas’s picture

There is one side effect that I just experienced myself: when files get downloaded then they start sending content before the exit hook and therefore the setrawcookie() doesn't work anymore. But that's no issue as I don't think that for file downloads you would want this functionality to work anyway. THe solution would therefore be to use a silent warning prefix like @setrawcookie();.

stefan.r’s picture

Title: Set the cookie during exit, not during boot » Language cookie can poison module implementation cache, breaking hooks for other modules
Priority: Normal » Critical

  • stefan.r committed 560d3d0 on 7.x-2.x
    Issue #2463727: Language cookie can poison module implementation cache,...

  • stefan.r committed c8999d7 on 7.x-1.x
    Issue #2463727: Language cookie can poison module implementation cache,...
stefan.r’s picture

Status: Active » Needs review
FileSize
722 bytes

Patch against 1.8 and 2.0-rc1 included. Would this fix the issue? (would rather not change too much and stick to hook_boot() for now)

jurgenhaas’s picture

This looks pretty dangerous as it is going to call other callback/hooks that may unintentionally call functions that are either not loaded yet or that will populate static variables far too early with potentially terrible side-effects. Using hook_boot is fairly dangerous and as the documentation for hook_boot says, "most include files" won't be loaded when this hook is going to be called.

stefan.r’s picture

@jurgenhaas your concern is addressed in the core issue #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion (and related).

This patch was done so as to not break BC in any way, ie. not with the alter hook, nor with the hook_boot implementation. While this may look dangerous, this piece of code is a copy of the core hook bootstrap_invoke_all(), which is there exactly for the purpose of being used during hook_boot(). So all of the 3 calls to other drupal core functions ought to be safe here and should not cause cache contamination, even when called during hook_boot().

So while for the previous call to drupal_alter() during hook_boot() your concerns definitely applied, I don't know that they do in this case.

However, if anyone implements a hook_language_cookie_alter() that calls a hook that it shouldn't, that's another story. Maybe we can document another warning in a comment?

  • stefan.r committed e8051ce on 7.x-2.x
    Issue #2463727: Language cookie can poison module implementation cache,...

  • stefan.r committed 50a7cfe on 7.x-1.x
    Issue #2463727: Language cookie can poison module implementation cache,...
stefan.r’s picture

Status: Needs review » Fixed

New releases for both 1.x and 2.x posted.

Status: Fixed » Closed (fixed)

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