Problem

  • A stale 'controller class' definition is not updated.

Details

  • Entity info is statically cached. Once defined, it sticks.
  • drupal_flush_all_caches()
    • does not reset any statics.
    • triggers all kind of core rebuilds that depend on entity info.
    • triggers flushing of core cache tables.
    • triggers flushing of module cache tables, which also rebuilds data, depending on entity info.
    • is invoked after full bootstrap, but entity info might have been prepopulated during bootstrap already (hook_menu_custom_theme(), hook_admin_paths(), hook_init(), etc).
  • Stale entity info can easily lead to WSODs elsewhere.
  • The cache flushing operation is effectively never reached, because Drupal dies before already. Hence, the entity info cache is never cleared.

_____________________

In a contributed module, I added custom 'controller class' to its hook_entity_info() implementation.

Upon trying to load any page, including admin_menu's flush all caches menu callback as well as drush cc all,

WSOD:

Fatal error: Class name must be an object or a string [...]

Further analysis showed that

- {registry} contained the new controller class.

- {system}.info contained the new files[] definition and the filename that contains the new controller class.

- Directly invoking registry_rebuild() after a full bootstrap worked.

- Directly invoking drupal_flush_all_caches() after a full bootstrap did not work.

- Directly invoking entity_get_info() after a full bootstrap did not show the module's entity type at all (even though I changed the controller class property only; the entity type itself was registered before already).

Clearing the entity info cache in drupal_flush_all_caches(), as in attached patch, fixed the problem.

Not sure whether this can be tested in any way.

Files: 
CommentFileSizeAuthor
#94 drupal.entity-info-cache-clear.94.patch3.18 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,010 pass(es).
[ View ]
#89 drupal_entity_info_cache.patch3.11 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,004 pass(es).
[ View ]
#85 drupal_entity_info_cache-D7.patch3.11 KBfago
#82 drupal8.entity-cache-flush.81.patch3.97 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,592 pass(es).
[ View ]
#77 drupal_entity_info_cache_8.patch4.12 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_entity_info_cache_8_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#73 drupal_entity_info_cache_8.patch4.12 KBfago
PASSED: [[SimpleTest]]: [MySQL] 34,243 pass(es).
[ View ]
#73 drupal_entity_info_cache-D7.patch3.25 KBfago
#73 drupal_entity_info_cache_test_only_8.patch3.55 KBfago
FAILED: [[SimpleTest]]: [MySQL] 34,239 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#73 drupal_entity_info_cache_test_only-D7.patch2.25 KBfago
#67 drupal_entity_cache_quick-for-bot.patch564 bytesfago
PASSED: [[SimpleTest]]: [MySQL] 37,306 pass(es).
[ View ]
#66 drupal_entity_cache_quick-for-bot.patch564 bytesfago
PASSED: [[SimpleTest]]: [MySQL] 37,291 pass(es).
[ View ]
#65 drupal_entity_cache_quick-for-bot.patch564 bytesfago
PASSED: [[SimpleTest]]: [MySQL] 37,291 pass(es).
[ View ]
#63 drupal_entity_cache_quick-D7.patch564 bytesfago
#60 drupal_entity_cache_quick.patch584 bytesfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_entity_cache_quick.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#57 drupal_entity_cache.patch6.48 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#54 drupal8.entity-cache-flush.54.patch4.56 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#49 drupal_entity_cache.patch5.33 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#47 drupal_entity_cache.patch3.08 KBfago
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#45 drupal_entity_cache-D7.patch1000 bytesfago
#45 drupal_entity_cache.patch986 bytesfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_entity_cache_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#37 drupal8.entity-cache-flush.37.patch6 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#35 drupal8.entity-cache-flush.35.patch4.19 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,531 pass(es), 18 fail(s), and 4 exception(es).
[ View ]
#32 drupal8.entity-cache-flush.32.patch4.08 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#27 clear_entity_cache-996236-27.patch2.37 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 29,837 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#25 clear_entity_cache-996236-25.patch2.37 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 29,907 pass(es), 2 fail(s), and 3,120 exception(es).
[ View ]
#20 clear_entity_cache-996236-20.patch2.19 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch clear_entity_cache-996236-20.patch.
[ View ]
#16 clear_entity_cache-996236-16.patch2.17 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 29,863 pass(es), 18 fail(s), and 4 exception(es).
[ View ]
#14 clear_entity_cache-996236-14.patch2.16 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#8 drupal.entity-cache-flush.8.patch1.92 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#2 drupal.entity-cache-flush.2.patch513 bytessun
PASSED: [[SimpleTest]]: [MySQL] 30,378 pass(es).
[ View ]
drupal.flush-entity-cache.0.patch587 bytessun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.flush-entity-cache.0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Comments

Somehow, I can't believe that no one else ran into this issue yet.

The problem is that without this patch, the stale 'controller class' definition cannot be updated in any way -- flushing caches leads to the WSOD, and manually rebuilding modules and the registry also leads to the WSOD.

The entity info cache is effectively never cleared.

StatusFileSize
new513 bytes
PASSED: [[SimpleTest]]: [MySQL] 30,378 pass(es).
[ View ]

Updating to latest code of Entity module requires this patch, too, due to major directory and file changes in the module.

Upon testing the patch once more in that situation, it became clear that we should rebuild the registry prior to flushing the entity info cache.

holy crap.

Status:Needs review» Needs work

hm, what was the error message? It's usually only problematic if the *old* controller class cannot be found. So perhaps the problem was that the cache got cleared before the registry was rebuilt?

For the entity.module upgrade, this was may problem for the 'entity class'. This is why the old file is left there for the upgrade to work. At least for me it did work?

Anyway, drupal_flush_caches() clears the cache tables, thus it should clear the entity cache too. It just doesn't clear the static cache, but drupal_flush_caches() doesn't do that for others either.

for completeness, here the description of my entity API upgrade troubles:

Also upgrading the module is rather weird, though I managed to get it working. For that it was required to leave entity.db.inc in place for the upgrade. Without that file even update.php cannot be accessed as modules might require the file to be loaded, thus the bootstrap requires it. Once people have upgraded, that can be removed - so we can do so in a subsequent release.

The problem is that update.php bootstraps drupal, what includes all enabled modules. Therefore if a module X has an entity class, the parent class is required. Consequently it tries to load it from the old path using the registry - but that happens already *before* the first cache-clear.

Status:Needs work» Needs review

Entity module's major update was just the second occasion that made me run into this bug. Related to that: Contrary to what you're saying, I was successfully able to run update.php -- until the final 'finished' op/page.

Yes, basically, drupal_flush_all_caches() already flushes the cache tables. However, it does not clear the static entity info cache, even though registry_build() might have written new class and interface information to the registry already.

Only the subsequently invoked menu_rebuild() in drupal_flush_all_caches() actually leads to the WSOD, because even though we have new registry and module information, the entity system still tries to access/load the old information.

As explained in the OP, detailed inspection and analysis showed that all of the new information was properly recorded already.

Repetitive invocation of drupal_flush_all_caches() does not help, because menu_rebuild() is invoked before the cache tables are flushed, so the entity system still reloads the stale/wrong entity info data from the cache.

Lastly, even if we would change the order of menu_rebuild() and cache_clear_all(), there would still be no guarantee that entity_get_info() was not invoked before drupal_flush_all_caches() in the request, so subsequent invocations of entity_get_info() via menu_rebuild() may still act on stale statically cached entity info data.

Status:Needs review» Needs work

Yes, I see the problem with the static cache. However I think drupal_flush_all_caches() is not designed to clear any static caches, as it just deals with "permanent" caches and doesn't clear any other static caches.

However, having menu_rebuild() in there is problematic as it immediately triggers the menu rebuild. In particular it is odd that it is invoked before hook_flush_caches() gets called. Thus this problem is very similar to #986024: ensure static caches are up2date - in both cases the static caches are inaccurate so the immediate menu_rebuild() may cause troubles.

So if we include the static cache reset for entities, why not others too? What about calling drupal_static_reset() there then? Also I think we have to move menu_rebuild() down, as it issues the menu cache rebuild and so rebuilding of other caches.
I do think menu_rebuild() shouldn't issue the immediate rebuild either, but I guess for d7 that's set in stone.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Short of: it's a mess.

Status:Needs review» Needs work

The last submitted patch, drupal.entity-cache-flush.8.patch, failed testing.

I have no idea why the Drupal installation fails with that patch.

Status:Needs work» Needs review

#8: drupal.entity-cache-flush.8.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, drupal.entity-cache-flush.8.patch, failed testing.

Updating entity.module certainly is troubling every time :)

I tried installing manually with the patch. Everyone works fine except that the success message is on a otherwise empty page, looks like the theme can't be found. Once the "go to your site" link is clicked, everything looks ok.

Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review
StatusFileSize
new2.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-roll using git format-patch.

Status:Needs review» Needs work

The last submitted patch, clear_entity_cache-996236-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
FAILED: [[SimpleTest]]: [MySQL] 29,863 pass(es), 18 fail(s), and 4 exception(es).
[ View ]

Wondering if it would be acceptable to call entity_info_cache_clear() instead of drupal_static_reset().

Status:Needs review» Needs work

The last submitted patch, clear_entity_cache-996236-16.patch, failed testing.

Status:Needs work» Needs review

#16: clear_entity_cache-996236-16.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, clear_entity_cache-996236-16.patch, failed testing.

Status:Needs review» Active
StatusFileSize
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch clear_entity_cache-996236-20.patch.
[ View ]

Okay, so something in the comments module is b0rken when clearing caches also calls entity_info_cache_clear()

But just to be sure, let's try with that particular call commented out.

Status:Needs work» Needs review

Status:Active» Needs work

The last submitted patch, clear_entity_cache-996236-20.patch, failed testing.

Status:Needs work» Needs review

#20: clear_entity_cache-996236-20.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, clear_entity_cache-996236-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 29,907 pass(es), 2 fail(s), and 3,120 exception(es).
[ View ]

Maybe if I fiddle with the order of things...

Status:Needs review» Needs work

The last submitted patch, clear_entity_cache-996236-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 29,837 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

Fixed typo:

Status:Needs review» Needs work

The last submitted patch, clear_entity_cache-996236-27.patch, failed testing.

@pillarsdotnet: I think you can stop trying here, please, your follow-ups along the submitted patches sound too much like a guessing game, which is not particularly helpful and rather noise for this issue.

We have to go back to the discussion that ended in #8, and precisely evaluate how this issue may be fixed. Meanwhile, I'm 99% confident that this issue cannot be resolved by shuffling around some lines of code. There are too many recursive interdependencies.

I'm a bit confused by this issue. If we had #534594: system info cache writing and registry rebuilding is broken without a full bootstrap in and this actually ran on update.php, would that help or not?

subscribing

Status:Needs work» Needs review
Issue tags:+DrupalWTF, +API clean-up, +needs backport to D7
StatusFileSize
new4.08 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

@catch: AFAICS, that only affects a small subset of the problem; only providing a workaround for recovering from a WSOD due to a stale registered class.

#1 tried to clarify, but let me try again:

Problem

  • A stale 'controller class' definition is not updated.

Details

  • Entity info is statically cached. Once defined, it sticks.
  • drupal_flush_all_caches()
    • does not reset any statics.
    • triggers all kind of core rebuilds that depend on entity info.
    • triggers flushing of core cache tables.
    • triggers flushing of module cache tables, which also rebuilds data, depending on entity info.
    • is invoked after full bootstrap, but entity info might have been prepopulated during bootstrap already (hook_menu_custom_theme(), hook_admin_paths(), hook_init(), etc).
  • Stale entity info can easily lead to WSODs elsewhere.
  • The cache flushing operation is effectively never reached, because Drupal dies before already. Hence, the entity info cache is never cleared.

_________________________

OK, a proper issue summary really helps... ;)

Attached patch might resolve the issue and explains the effin' mess of interdependencies in inline comments.

Status:Needs review» Needs work

The last submitted patch, drupal8.entity-cache-flush.32.patch, failed testing.

sigh. This patch reveals yet another interdependency between theme registry and caches/rebuilds.

Status:Needs work» Needs review
StatusFileSize
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] 33,531 pass(es), 18 fail(s), and 4 exception(es).
[ View ]

Resorted to specifically clearing the entity info cache only, clarified in a comment.

Status:Needs review» Needs work

The last submitted patch, drupal8.entity-cache-flush.35.patch, failed testing.

StatusFileSize
new6 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Back to drupal_static_reset(), but in the proper location (first).

Only remaining problem is that the invocation of drupal_flush_all_caches() in install_finished() breaks the maintenance theme for an unknown reason. I've spent the past three hours with stepping through the subsystems, but wasn't able to figure out what exactly goes wrong. theme.inc doesn't really use drupal_static(), and neither does theme.maintenance.inc, nor install.core.inc. But nevertheless, the drupal_static_reset() in drupal_flush_all_caches() makes the install_finished() page render without a theme (plain page output, as in Stark, but without Stark).

Contains debugging code, no need for testing.

Status:Needs work» Needs review

Testing last patch

Status:Needs review» Needs work
Issue tags:-DrupalWTF, -API clean-up, -needs backport to D7

The last submitted patch, drupal8.entity-cache-flush.37.patch, failed testing.

Status:Needs work» Needs review

#37: drupal8.entity-cache-flush.37.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+DrupalWTF, +API clean-up, +needs backport to D7

The last submitted patch, drupal8.entity-cache-flush.37.patch, failed testing.

#1216454: Custom controller classes must not be deleted is probably a duplicate of this issue.

Status:Needs work» Needs review

ok, let's try it with a patch keeping the changes to the bear minimum.

StatusFileSize
new986 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_entity_cache_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
new1000 bytes

Where are my patches?

Status:Needs review» Needs work

The last submitted patch, drupal_entity_cache.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.08 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Strange, the failing test worked for me with the D7 version of the patch.
Anyway, here is a try to properly clean up the mess for D8. Let's see what the bot thinks.

There are for sure some race conditions left, e.g. consider a page request coming in after the registry has been rebuilt but cache bins have not been flushed yet. First we've to get the basics right though.

Status:Needs review» Needs work

The last submitted patch, drupal_entity_cache.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.33 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

ok, ran into the installation problem as sun in #37. Figured out the problem: the added css is statically cached, and thus gone after flushing caches.

I've documented this behavior + also unset $theme so the whole theme is marked as un-initialized after clearing caches. That way, one can still output pages with a fresh initialized theme. Usually, one wants a redirect afterwards anyway though, so clearing added css/js should be acceptable. In turn, we get properly reset static caches everywhere, thus less bugs during the following cache rebuilds.

Let's see what the test bot thinks now.

Status:Needs review» Needs work

The last submitted patch, drupal_entity_cache.patch, failed testing.

Status:Needs work» Needs review

Strange, installation works for me. :/

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new4.56 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-roll of #37 (without debugging code), as I wasn't sure what was changed since then. Hadn't time for in-depth comparison, so chose to simply re-roll it...

Status:Needs review» Needs work
Issue tags:-DrupalWTF, -API clean-up, -needs backport to D7

The last submitted patch, drupal8.entity-cache-flush.54.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+DrupalWTF, +API clean-up, +needs backport to D7

drupal.flush-entity-cache.0.patch queued for re-testing.

Status:Needs review» Needs work
StatusFileSize
new6.48 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

sry, here some explanations:

  • I tried to re-arrange stuff so that the static-reset + the cache clears happen at the same time. This is important so any possible triggered rebuilds don't get to use stale caches - neither from statics of from the cache system.
  • I wanted to remove node rebuilding as its static + permanent caches are cleared already. But the rebuild operation additionally updates the node-type table based upon module hooks. That's a node specific operations which doesn't belong in the system cache clear operation and only needs to happen on module enable or disable, so I've moved it over to such node module hooks. Also, fixed _node_types_build() to fill the static cache upon cache miss.
  • Ran into the same problem as you with the installation finished page and figured out it's due to emptied statics from drupal_add_css. See #49.
  • Removed the needless call to drupal_theme_rebuild() - cache tables are already cleared.

I compared it to #54 and incorporated improvements from it:

  • Moved clearing cache_filter to filter.module.
  • Moved clearing page cache to the bottom. - I also moved the call to _drupal_flush_css_js() to the bottom (same logic).

+  // Rebuilding node types requires fresh entity information, since node_menu()
+  // defines router items based on node types, so they need to be rebuilt before
+  // hook_menu() is invoked.

Why does node_menu() require entity info? On the contrary entity-info requires node-type-info (see node_entity_info()).

+  // @todo Replace with a hook_rebuild(); contributed modules potentially also
+  //   need to rebuild data before hook_menu() is invoked. Rebuilding logic
+  //   should not be contained in hook_menu(), since these are separate
+  //   operations, and menu_rebuild() might not invoke hook_menu() at all due to
+  //   the locking framework.

I'm not sure about that. Is those rebuilding really a valid operation? If it's duplicated data, rebuilt for performance or so, it's a cache. That means, don't do a rebuild operation but clear the cache using hook_flush_caches() and rebuild the cache on demand.
-> By always going with that approach caches automatically rebuild in the right order and we don't end up with accidentally wrong rebuild orders possibly resulting in wrong caches/rebuild info. Also, it's important to be able to initiate that rebuild without it to happen automatically.

E.g. I really think menu_rebuild should work using the "cache-clear approach". Now, a cache rebuild may trigger lots of menu rebuilds if lots of items trigger them. Added a new page manager page? Rebuild! A new profile type? Rebuild! A new node type? Rebuild! ...

So maybe instead we should document clearing-caches (bot not issuing rebuilds) from hook_flush_caches() is intended behavior?

Merged patch attached, please review. Still, I've no idea why the test-bot fails. Tests basically work for me, as well as installing manually.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal_entity_cache.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new584 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_entity_cache_quick.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Well, looks like cleaning this up isn't an easy task, but we need to fix the entity-info cache problem for D7 soon. So let's better do a quick fix first and properly clean up the mess for d8 afterwards. Patch attached.

Point being, the simple fix didn't solve the problem in 2 out of 3 occasions or so. (actually I'm no longer sure whether it ever worked or whether the situation resolved itself through repetitive and/or concurrent brute-force cache clearing)

Contrary to that, #54 even worked for the extreme case of the /core directory change; i.e., completely bogus registry and controller/class autoloading info.

Status:Needs review» Needs work

Is those rebuilding really a valid operation? If it's duplicated data, rebuilt for performance or so, it's a cache. That means, don't do a rebuild operation but clear the cache using hook_flush_caches() and rebuild the cache on demand.

Yes it is a valid operation. The rebuilding accounts for the fact that various things are merely known to the system via info hooks. The rebuild operations are not caches, they actually re-retrieve, collect, prepare, and create/update known, semi-dynamic information registered via module hooks, and also delete previously known info.

Rebuild operations cannot be compared to caches. The module or subsystem (re)building data almost always has a cache layer in front of the built data, and that cache is also cleared as part of the rebuilding, but that's not the point of rebuild operations.

Changing and revamping rebuild operations is a different issue and requires lots of discussion first. Hence the @todo only. Let's keep this discussion out of this issue, please.

In light of that, the node type rebuild changes in the latest patch need to be reverted, because they break current expectations.

Why does node_menu() require entity info? On the contrary entity-info requires node-type-info (see node_entity_info()).

You're totally right on that. No longer sure what made me believe that.

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new564 bytes

I get regularly bug reports in the entity api queue by people stumbling over this. Thus, let's fix this asap. Ideally before 7.10 ships.

Point being, the simple fix didn't solve the problem in 2 out of 3 occasions or so. (actually I'm no longer sure whether it ever worked or whether the situation resolved itself through repetitive and/or concurrent brute-force cache clearing)

Still, #54 has issues. Doing a whole drupal_static_reset() sounds good, but it's no simple change and needs work. I think best, we clean up the whole mess in D8 first and then look at what makes sense to backport. But that's going to take time and people run into this issue now, so let's do the 1-line stop-gap fix now.

I've attached the d7 version of #60. I'm setting the issue temporarily to d7 to let the test-bot run for d7 too.

Yes it is a valid operation. The rebuilding accounts for the fact that various things are merely known to the system via info hooks. The rebuild operations are not caches, they actually re-retrieve, collect, prepare, and create/update known, semi-dynamic information registered via module hooks, and also delete previously known info.

Rebuild operations cannot be compared to caches. The module or subsystem (re)building data almost always has a cache layer in front of the built data, and that cache is also cleared as part of the rebuilding, but that's not the point of rebuild operations.

Thinking about it, I must agree. I've even already implemented one: rebuilding exported entities upon cache clear. A separate hook would be important here, to ensure it doesn't conflict with cache-clears. But yes, let's handle that in another issue.

double post

StatusFileSize
new564 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,291 pass(es).
[ View ]

again d7 version for the bot.

StatusFileSize
new564 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,291 pass(es).
[ View ]

triple post...

StatusFileSize
new564 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,306 pass(es).
[ View ]

triple post...

#45: drupal_entity_cache.patch queued for re-testing.

I was having a problem when disabling my custom modules, it would error with basically "unable to find the entity's UIController".

I manually applied the patch proposed in #67 and now everything works as it should.

Thanks,
-Marcus

Issue tags:+Needs tests

The stop-gap fix looks good to me. But maybe we can add a test with it so that we have that to test a "proper" solution?

Version:7.x-dev» 8.x-dev

Moving back to 8.x.

Issue tags:-Needs tests
StatusFileSize
new2.25 KB
new3.55 KB
FAILED: [[SimpleTest]]: [MySQL] 34,239 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
new3.25 KB
new4.12 KB
PASSED: [[SimpleTest]]: [MySQL] 34,243 pass(es).
[ View ]

I agree that this needs tests -> I took a stab on that and implemented tests that prove changes to entity info appear in the cache after calling drupal_flush_all_caches() + after disabling a module.

It turned out the latter is already solved for d8, as the entity.module patch added an entity-info-cache-clear upon module_disable() already. Thus, I've improved the D7 version to do so too, but directly in module_disable() due to the lack of an entity module.
Changes to entity info do not appear after calling drupal_flush_all_caches() in both D7 and D8 though.

Updated patches for d7 and d8 attached, whereas I've added test-only patches to prove they fail too.

Status:Needs review» Needs work

The last submitted patch, drupal_entity_info_cache_test_only_8.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal_entity_info_cache_test_only_8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_entity_info_cache_8_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

yep, the test without the fix fails.
re-uploading the complete patch. equals the patch from #73.

People still ran into this, e.g. see #1352422: Uninstall breaks site.
Any remarks to the patches in ##73?

This looks good to me for fixing the bug but it is really a sad state that we have this mixture of 'registry' rebuilds, clears of individual static and persistent caches and then full bin clushes in drupal_flush_all_caches(). sun was mentioning trying to split this into distinct registry vs. cache steps - is there an issue for that?

Status:Needs review» Needs work
Issue tags:+DrupalWTF, +API clean-up, +needs backport to D7

The last submitted patch, drupal_entity_info_cache_8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.97 KB
PASSED: [[SimpleTest]]: [MySQL] 34,592 pass(es).
[ View ]

Patch will likely fail due to #375397: Make Node module optional

While this patch seems to fix a bug and is proven by tests, it slightly contradicts what I was after in #54; i.e., flushing/clearing everything first, and trigger any rebuilds only afterwards (from a clean slate).

In other words, I do not understand why you explicitly want to rebuild node types before clearing the entity info caches.

What is the rationale behind that?

Second, is it possible that the test also passes if you reverse the order? (i.e., is it possible that the test only passes due to the mere entity_info_cache_clear() being added, regardless of its position?)

Let me double-check that in attached patch.

In other words, I do not understand why you explicitly want to rebuild node types before clearing the entity info caches.

What is the rationale behind that?

Entity-info contains bundle info, what in case of nodes are the types. Thus, the node rebuild needs to run before entity info is built, as e.g. issued during menu_rebuild(). That said you are right that the entity-info cache clear could happen before node-type rebuilding as long as node-type rebuilding does not trigger rebuilding entity info itself.
Afaik it doesn't, but still that might happen on another page request in the meantime.... Thus, clearing entity-info afterwards ensures we are on the safe side.

Second, is it possible that the test also passes if you reverse the order? (i.e., is it possible that the test only passes due to the mere entity_info_cache_clear() being added, regardless of its position?)

Yes, the tests generally make sure entity info gets updated, but not in any way specific to node-types.

I've opened #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding so we can concentrate on the stop-gap fix for entity-info here.

I'd prefer #82 then. It merely clears the entity info cache early, and does not involve the previously added comment about a interdependency specific to node type rebuilding.

StatusFileSize
new3.11 KB

ok, let's go with #82 then.
Attached the d7 version of #82.

Status:Needs review» Reviewed & tested by the community

Alright then.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs review

Thanks! Committed/pushed to 8.x, moving to 7.x.

+1

StatusFileSize
new3.11 KB
PASSED: [[SimpleTest]]: [MySQL] 37,004 pass(es).
[ View ]

No commit credit; all I did was change the name of the file.

Anything left from setting the port RTBC? It's a straight port from what d8 does.

+++ b/includes/module.inc
@@ -545,6 +545,7 @@ function module_disable($module_list, $disable_dependents = TRUE) {
+    entity_info_cache_clear();

Why is there an additional clear here? The D8 patch only has the one in dfac().

Yep, as explained previously:

It turned out the latter (entity-info cache clear upon module disabling) is already solved for d8, as the entity.module patch added an entity-info-cache-clear upon module_disable() already. Thus, I've improved the D7 version to do so too, but directly in module_disable() due to the lack of an entity module.
Changes to entity info do not appear after calling drupal_flush_all_caches() in both D7 and D8 though.

Thus, it has been already fixed in d8, but is so far missing for d7.

Sorry for blatantly missing that clarification, my fault. :-/

I've double-checked the D8 code and since we're invoking entity_info_cache_clear() as part of hook_modules_disabled(), it is invoked before the registry_update(). Also, entity.module is likely invoked as one of the first hooks.

However, the D7 patch clears the entity info cache after hook_modules_disabled() and after registry_rebuild(). Given how extremely fragile this entire system is, I'd prefer to move the entity_info_cache_clear() right before hook_modules_disabled().

StatusFileSize
new3.18 KB
PASSED: [[SimpleTest]]: [MySQL] 37,010 pass(es).
[ View ]

Adjusted for #93. No other changes. RTBC if it comes back green.

Assigned:sun» fago
Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs review

This looks like it still needs review. This also means it won't make it to the February 1 release, unless there is absolutely no chance it could possibly break anything. But it can be part of the release after that!

Status:Needs review» Reviewed & tested by the community

I've double-checked the D8 code and since we're invoking entity_info_cache_clear() as part of hook_modules_disabled(), it is invoked before the registry_update(). Also, entity.module is likely invoked as one of the first hooks.

Right, sry for missing this order. :/ -> Your change looks good to me. I also tested your patch manually, it works fine. -> back to RTBC.

@webchick:
I doubt the added cache-clear will break anything. Instead, the breakage is already happening.. E.g. here are some issues I'm aware of:
#1316834: How to Clear Entity info cache ?
#1325126: uninstallation troubles due to outdated entity info
#1357640: SOS unchecked some modules related to support ticketing leads to critcal failure ...
#1294200: Complete uninstall still leaves phantom entries in module list
#1274692: Error on Disabling Module
#1412606: Fatal error: Class 'Profile2MetadataController' not found in /home/unitedor/public_html/sites/all/modules/entity/entity.info.inc

Thus, to avoid more headaches I'd recommend shipping this fix asap.

Update: I forgot to mention that the patch really helps, e.g. this broke a site I was working on yesterday. After applying the fix, the site was recovered. Without the fix, the only way to recover the site is manually clearing cache tables.

Status:Reviewed & tested by the community» Fixed

*wince* Ok...

Committed and pushed to 7.x. Thanks!

Thanks! Yes, this patch is known to only improve the situation.

The issue merely got lengthy as we were discussing what the actual root cause and "proper" approach/solution would be. This patch here is a stop-gap fix. The major API change to fix it properly happens over in #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding

Status:Fixed» Closed (fixed)

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

Thanks! :)

Issue summary:View changes

Updated issue summary.