Clear entity info cache during module enable

Ok, I found the issue. It's the entity cache.

rules2 implements hook_watchdog() and then tries to get entity information.

It is a bit tricky to reproduce manually because it does work once the modules have been installed correctly once, because the entity cache actually contains the information about those entities at that point.

Therefore, I'm attaching a hacked version of the Module dependency test which uses rules and the entity modules. That one fails without the changes in module.inc but obviously needs those modules to be present so it can only be manually tested.

We probably need a proper test case for this.

Hm, shouldn't it suffice to implement a module providing a new entity and checking for the existence of the new entity_info directly after enabling the module?

Hm, strange. Probably you should invoke entity_get_info() before calling module_invoke too, so you make sure the cache is actually filled.

Strange, I just installed a new site and I didn't experience any problems!?

+++ modules/simpletest/tests/entity_cache.test
@@ -0,0 +1,24 @@
+  function setUp() {
+    parent::setUp('entity_cache_test');
+  }

Try it without that part. This already enables the test module, and the module_enable() call in your code doesn't doanything then...

Powered by Dreditor.

oh, of course.. Great that we have a working test case now. The fix is simple and looks fine, but the test case needs some work:

* The test case does not really test the entity cache, it tests module enabling and makes sure the entity cache is fine. Thus I think the test case should put to the other module enable tests.
* @entity_cache_test_watchdog() - we need to document why this is there and what it does. Else that's just confusing for anyone reading the code.

Please use entity_info_cache_clear() which encapsulates the drupal_static_reset() and the cache_clear_all().

Also I feel like module.inc shouldn't know anything about entities - i.e. this should be a hook implementation (system_modules_enabled()?), but I have doubts that actually fix the bug given the general mess in module enabling.

Yeah let's deal with interdependencies between includes in another issue, probably D8. It would also be nice to only clear this cache if the module being enabled implements hook_entity_info() or hook_entity_info_alter(), but that's not worth special casing here given all the other stuff that gets cleared regardless.

Looks like the patch was committed here along with another commit, without adding the new test module. So we either need to roll back the patch or commit the test module.

Committed to HEAD!

#910190: Entire schema cache needlessly loaded on all pages reveals that blatantly assuming a completed installation in hook_watchdog() is a weird assumption. This patch does not even check the actual watchdog message, so it expects that any watchdog message is the watchdog message this test is targeting. I'm not sure what exactly has been tried to asserted here.

/**
 * Implements hook_watchdog().
 *
 * This function is called during module_enable() and tries to access entity
 * information provided by the module this one depends on. The information is
 * stored in a temporary system variable and is later analyzed in the test
 * case.
 *
 * @see EnableDisableTestCase::testEntityCache()
 */
function entity_cache_test_watchdog() {
  $info = entity_get_info('entity_cache_test');
  variable_set('entity_cache_test', $info);
}

+++ modules/simpletest/tests/entity_cache_test.module
@@ -0,0 +1,22 @@
+function entity_cache_test_watchdog() {

+++ modules/simpletest/tests/entity_cache_test_dependency.module
@@ -0,0 +1,19 @@
+      'base table' => 'entity_cache_test',

1) So this should have been hook_modules_installed().

2) It's invalid do define a base table that does not exist.

My patch over in #910190: Entire schema cache needlessly loaded on all pages corrects this.

+++ modules/system/system.test
@@ -157,6 +157,16 @@ class EnableDisableTestCase extends ModuleTestCase {
+    $this->assertNotNull($info, t('Entity information must not be NULL'));

assertNotNull()? That does not assert any expectation.

Powered by Dreditor.

Thanks!

As far as I can see there are 2 watchdog() calls now, so we want to make sure that we call watchdog() only after the module was installed

For that, we don't have to change the watchdog invocations, but have to fix the entity_cache_test_watchdog() implementation instead. Additionally,

- change the assertNotNull() to something that actually asserts an expectation (I think you are expecting a fully loaded and fully usable entity info array - that is what needs to be asserted accordingly, if so).

- remove the 'base table' from hook_entity_info().

- Heavily improve the comments on entity_cache_test_watchdog() to let readers understand why this test looks a bit weird.

+++ modules/simpletest/tests/entity_cache_test.module
@@ -9,14 +9,17 @@
+ * This hook is called during module_enable() and tries to access entity
+ * information provided by the module this one depends on, which should be
+ * available. The information is stored in a temporary system variable and is
+ * later analyzed in the test case.

Can we clarify this a bit more? Instead of telling what it does (already visible in the code), we need to explain the "why" + expectations. Something along the lines of:

This hook is called during module_enable() and since this hook implementation is invoked, we have to expect that this module and dependent modules have been properly installed already, so we expect to be able to retrieve the entity information that has been registered by the required dependency module.

+++ modules/system/system.test
@@ -161,11 +161,14 @@ class EnableDisableTestCase extends ModuleTestCase {
-    $this->assertNotNull($info, t('Entity information must not be NULL'));
+    $this->assertEqual($info['label'], 'Entity Cache Test', 'Entity info label is correct.');
+    $this->assertEqual($info['controller class'], 'DrupalDefaultEntityController', 'Entity controller class info is correct.');

Much better, thank you!

Powered by Dreditor.

Much better, thank you!

#29: 742972-entity-cache-test.patch queued for re-testing.

Committed to HEAD. Thanks!