Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Currently the JS module tries to take care of possible cache poisoning of the module_implements cache e.g. in js_deliver(). Unfortunately this has no control over what's happening in $delivery_callback function.
A call to module_implements() with a not yet cached hook will re-enable the cache write for the module_implements cache.
Unfortunately there are modules e.g. Admin Menu - which trigger module_implements_write_cache() without actually invoking other hooks e.h. hook_exit().
This leads to cache poisoning - especially together with js_custom_theme() which adjusts the implementations of hook_init() to avoid double invocation.
Proposed resolution
Implement hook_module_implements_alter() and ensure it unsets the #write_cache flag whenever the hook is invoked during a JS callback.
hook_module_implements_alter() will always be invoked after $implementations['#write_cache'] is set - which should provide a more reliable way of unsetting it.
I think with the new hook we can remove the handling in js_deliver() and just rely on the new hook implementation.
The only thing I'm not completely sure yet is if this covers all scenarios in which elseif (!isset($verified[$hook])) { is entered in module_implements().
Remaining tasks
reviews needed.
| Comment | File | Size | Author |
|---|---|---|---|
| js-cache-poisoning-of-module_implements.patch | 1.43 KB | das-peter |
Comments
Comment #2
plachMark asked me to have a look at this issue, since I worked on skipping double
hook_init()invocation. The patch looks reasonable to me, but I could not reproduce the issue you are mentioning with Admin Menu. Can you post the exact steps to see the issue you are reporting, ideally on a clean installation?Also, I find it a bit concerning that the
elseifbranch ofmodule_implements()would be skipping the logic to clear the#write_cacheflag. However, I wasn't able to reproduce the issue and I couldn't tell whether actually problematic conditions can arise.Comment #3
markhalliwellSame. Seeing how this issue is old, I'll just go ahead and commit as I trust @das-peter's work and this doesn't seem like it would adversely affect anything.