Just opening this issue in case anyone has run into a similar problem. This is not technically an issue with the crumbs module itself, but since it's the only module that uses hook_module_implements_alter in this "creative" way this feels like an appropriate place to file this for now.

The issue is that crumbs_module_implements_alter() is not running for us under certain circumstances. It is yet to be determined what the cause is, as we can only reproduce this on a system with all other kinds of caching, where after a cache clear, crumbs_module_implements_alter() will only run if we have a low realpath_cache_size value.

As the require_once for crumbs.info.inc (with the theme hooks in it) only happens whenever module_implements() runs, the crumbs_theme hook doesn't get picked up and we get empty breadcrumbs.

For now it seems like the drupal_alter cache isn't cleared correctly, as it only considers the module_implements_alter() implementations for "bootstrap" modules, as described in #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion.

CommentFileSizeAuthor
#19 2488208-19.patch2.13 KBstefan.r
#12 2488208-12.patch2.17 KBstefan.r
#9 2488208-9.patch2.01 KBstefan.r
#6 2488208-5.patch19.13 KBstefan.r
#2 2488208-2.patch533 bytesstefan.r

Comments

stefan.r’s picture

stefan.r commented
Title: crumbs_module_implements_alter not running with realpath_cache_size values » crumbs_module_implements_alter not running with high realpath_cache_size values
Issue summary: View changes

One of the issues with this might be: #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion

stefan.r’s picture

stefan.r commented
StatusFileSize
new 2488208-2.patch533 bytes

One way to fix this would be this patch, but that feels like hack upon hack upon hack :)

The cleanest way may be to just put the Drupal hook implementations back into crumbs.module.

Or alternatively, as the dynamic include file loading that happens in crumbs_module_implements_alter() is clever, but this is the first module to try this, it's doing it in a way unsupported by core, and fakes hook_hook_info() data: perhaps just requiring all of hook/help/form files on every request (the way we do with crumbs.callbacks.inc and crumbs.block.inc) might be better?

donquixote’s picture

donquixote commented

Hi!
I am ready to drop the "creative" use of hook_module_implements_alter(), once I understand what is going on there.

stefan.r’s picture

stefan.r commented

@donquixote thanks. That was not a criticism by the way, I actually thought it was quite clever ;)

I think we just stumbled upon a nasty core bug, the complaints in the issue in #1 sound similar. I'll report back when I find out more in a week or so.

stefan.r’s picture

stefan.r commented
Title: crumbs_module_implements_alter not running with high realpath_cache_size values » Crumbs module breaks if module_implements cache is poisoned and only lists bootstrap modules
Priority: Normal » Major

This moves the hook implementations back into the .module file. I realize it becomes a "longer" file, but on the other hand, that's the case for the large majority of Drupal modules anyway :)

In my particular case the hook_module_implements_alter() issue is still unexplained as of yet, it seems to be happening due to a core bug (#496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion, which links to several other critical issues), where on some installs the module_implements cache can become unreliable. This means the Crumbs module breaks quite badly as it relies on its hook_module_implements_alter() implementation being available to even display the breadcrumbs.

stefan.r’s picture

stefan.r commented
Status: Active » Needs review
StatusFileSize
new 2488208-5.patch19.13 KB
donquixote’s picture

donquixote commented

I just had a look at the core issue, #496170: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion

So, if I understand correctly.

  1. An unexpected chain of events causes module_implements($hook) being called from hook_boot().
    The $hook typically has nothing to do with Crumbs, it can be any random hook.
  2. This causes module_implements('module_implements_alter') to be called.
  3. Drupal looks for all implementations of hook_module_implements_alter().
    At this time, crumbs.module is not included yet.
    Therefore, for the rest of the request, Drupal assumes that crumbs does NOT implement hook_module_implements_alter().
  4. Later in the request, implementations of other hooks are being discovered, but the crumbs implementations are not being found, because crumbs_module_implements_alter() is not being executed.

Your analysis (also what you sent me by email) is spot-on.
We should also update this stackexchange question:
http://drupal.stackexchange.com/questions/108884/hook-module-implements-...

donquixote’s picture

donquixote commented

As for your patch in #6:
Since I am already working on 7.x-3.x, I would prefer to simply do require_once from the module file, to reduce noise in git history.

// __DIR__ not defined in older PHP versions.
require_once dirname(__FILE__) . '/crumbs.block.inc';
require_once dirname(__FILE__) . '/crumbs.help.inc';
require_once dirname(__FILE__) . '/crumbs.info.inc';
...

Yes, I can see how putting it all in the module file could be preferable, but atm I feel that less noise = easier merging = better.

stefan.r’s picture

stefan.r commented
StatusFileSize
new 2488208-9.patch2.01 KB
donquixote’s picture

donquixote commented

Great!
Maybe a comment,

// Include other files that contain callbacks and hook implementations.
// See https://www.drupal.org/node/2488208
require_once ...
stefan.r’s picture

stefan.r commented
Issue summary: View changes
stefan.r’s picture

stefan.r commented
StatusFileSize
new 2488208-12.patch2.17 KB

The last submitted patch, 9: 2488208-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2488208-12.patch, failed testing.

Status: Needs work » Needs review

stefan.r queued 12: 2488208-12.patch for re-testing.

stefan.r’s picture

stefan.r commented

Re-queueing in case this was a testbot issue.

Status: Needs review » Needs work

The last submitted patch, 12: 2488208-12.patch, failed testing.

donquixote’s picture

donquixote commented

Yes, it was a testbot failure the first time. But not this time :)

+require_once dirname(__FILE__) . '/includes/crumbs.block.inc';
+require_once dirname(__FILE__) . '/includes/crumbs.info.inc';
+require_once dirname(__FILE__) . '/includes/crumbs.help.inc';
+require_once dirname(__FILE__) . '/includes/crumbs.form.inc';

These files don't live in "includes" subfolder..
(The "creative" hook_module_implements_alter() would not have worked with files in a subfolder)

stefan.r’s picture

stefan.r commented
Status: Needs work » Needs review
StatusFileSize
new 2488208-19.patch2.13 KB

ha :)

  • donquixote committed 8f1cf25 on 7.x-2.x authored by stefan.r 
    Issue #2488208 by stefan.r: Crumbs module breaks if module_implements...
donquixote’s picture

donquixote commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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