Using entity_get_info in a hook_hook_info results in an incomplete module_implements cache.

Yes, and there are also similar issues related with when hooks are being called. I.e. some hooks being called on bootstrap phase (not all modules available yet) may result on corrupted (mostly incomplete) static caches, some of which are then cached into the db.

We need at the very least some documentation on when is safe to call which hooks and which hooks can call other hooks.

Some of the 'rules' could be:
- Modules in hook_boot() shouldn't trigger invokation of any other hook (nor generally call other modules' functions)
- Functions like module_invoke_all() shouldn't be called from bootstrap, unless used for bootstrap hooks. Related #578044: move module_invoke_all() out of the reach of hook_boot()

One idea: Hooks could have some 'level' meaning hooks with lower level can't call hooks with higher level. This level could 'propagate' to functions triggering that hook. Example:
- hook_boot() or other bootstrap hooks should be level 0
- hook_hook_info() should be level 1 (it cannot be called from hook_boot())
- hook_entity_info() should be level 2 (Then hook_hook_info() cannot call hook_entity_info())
- Other hooks using entity_get_info() should have a level > 2
- Etc...

Somehow related: #1568126: Apachesolr causes all simpletests to fail #6

This is major and D8 staff also related #968264: hook_hook_info() can lead to an infinite loop

I got this trouble when some module has notice inside (my case the same constant defined twice) backtrace is
module_load_all()
- drupal_load() - hit notice -> watchdog()
-- watchdog() calls module_implements() -> module_hook_info() (static cache generated from incomplete loaded modules)
...
system runs with incomplete module_hook_info() and caches wrong module_implements()

D8 has protection within module_hook_info() to return empty array if not all modules loaded

I don't think this is a major bug, core doesn't do this. If anything it could just be documentation.

True, but documentation alone doesn't keep people from doing this. The problem is that it makes people's sites break in unpredictable and hard to trace ways.

A patch to at least not cache incomplete module_implements() data, is uploaded to #496170-14: module_implements() cache can be polluted by module_invoke_all() being called (in)directly prior to full bootstrap completion.

This bug is quite important to solve, I did trigger it on a production site and it results on an incomplete hook_hook_info() cache, which involves an incomplete module_implements() cache, which leads to serious data inconsistencies in our case, this needs to be solved.

This is relatively unpleasant bug for debugging. I had a problem with undefined variable in the .module file root. Because of it Drupal wanted to log "Notice: Undefined variable..." and called sequence: _drupal_error_handler(), _drupal_error_handler_real(),_drupal_log_error(), watchdog(),module_implements(),module_hook_info() - which cached hook_info into persistent cache. But only few modules were loaded that time, because this Notice was triggered in the module loading process (after include of .module file). So since then my site was missing many hooks (till clear of persistent cache) and worked oddly.

I suggest attached patch which doens't allow to cache hook_info till all modules are loaded.

See if testbot likes it, setting to needs review

I am not sure about the testbots results, because 3 test runs ended with different results, so I even don't know what to fix. On my local enviroment tests of core ends with different results (test failures) even without my patch. Can you give me feedback about the tests please?

tests passed finally.

We have been using this production for some months now. It has resolved an issue we were having where tokens were not being replaced, and the metatag module was "losing" it's configuration. The site is heavily bloated, and without this patch the cached item is often incomplete of all hook implementations.

I believe that this patch is critical to a stable Drupal 7 site. (At least when the site is bloated to an extent which is difficult to describe.)

We've continued to have this running in production without issue, I have triggered a few more tests even though I'm moving this to being reviewed.

Wow, this has been RTBC for a while. The patch has helped resolve issues I immediately ran into with PHP 8, because deprecation warnings were being logged early in bootstrap, causing the cache of hooks to be broken - see #3305946-16: [PHP 8.1] TypeError: Unsupported operand types: array + null in RulesData::addSiteMetadata()

What do we need to do to progress this?

Patch #10 helped me to fix an error on the Drupal 7.94 project with PHP 8.1 and modules entity and og

EntityMetadataWrapperException: Entity property og_user_node doesn't support writing. in EntityStructureWrapper->setProperty() (line 519 of /var/www/html/sites/all/modules/entity/includes/entity.wrapper.inc).
which appears when the administrator requests a user edit form.

Rerolled #10 against the latest 7.x branch in MR 3458

Thanks for the reroll. However could we please stick to patches rather than MRs for D7 core issues, especially when there are already patches.. switching from patches to an MR makes review harder (for me at least).

I've tweaked the comment a little but that's all. The interdiff is against the reroll.

I think this looks good, and I've certainly come across very tricky to debug problems when something causes an incomplete cache entry.

Over to @poker10 for final review and commit if he's happy.

As this does not have any test added, I tried to test this manually according to the steps from issue summary (see the uploaded patch). When I added entity_get_info(); call to the module_test_hook_info() (see ModuleUnitTest::testModuleImplements()), then I get an error TypeError: array_keys(): Argument #1 ($array) must be of type array, null given in array_keys() (line 7401 of includes/common.inc with and without the patch #24 (tested locally on PHP 8.1 / PostgreSQL).

So I think that the patch is probably not fixing the issue mentioned in the issue summary, but some other issue/s (see comments #10, #18, #20). In this case I am probably OK with commiting this, but we need to update Issue summary and Issue title about what exactly we are fixing (and ideally include a test for this).

If this fixes entity_get_info(); in hook_hook_info() problem, then I would be happy to see a failing test-only patch and green fix+test patch for this.

Some other notes to consider:

There is a very similar issue here: #1934192: hooks disappear from module_implements cache, which tries to handle it a bit sooner. Is that a duplicate? It seems to me that both issues tries to verify if all modules are loaded (but different ways).

Also there was another possible duplicate: #968264: hook_hook_info() can lead to an infinite loop, which @catch closed (for D8) as Works as designed. Therefore it seems more likely as "Don't do this" with possible warning in the documentation.

---------

Bottom line:

1. I still think we should make this caching more robust and fix cases like #18 and #20 (caused by watchdog calls / deprecation warnings), because these are causing for example tokens to disappear (I experienced this personally on some sites doing upgrades to PHP 8.1). But we should consider if this (#24) is the right approach or this: #1934192: hooks disappear from module_implements cache.

2. If the fix from the point 1 does not fix also entity_get_info(); in hook_hook_info() problem, then I am not sure if we should try to fix it at any cost - probably a mention in the documentation (e.g "Don't do this") will be sufficient.

Test failed on DrupalCI as well.

Adding another related/duplicate issue.