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

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.

The last submitted patch, 25: 1415278-25-do-not-use.patch, failed testing. View results →

Test failed on DrupalCI as well.

Adding another related/duplicate issue.