Posted by fago on January 13, 2012 at 4:21pm
17 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | major |
| Assigned: | sun |
| Status: | closed (fixed) |
| Issue tags: | API change, API clean-up, Framework Initiative |
Issue Summary
Follow-up to properly fix #996236: drupal_flush_all_caches() does not clear entity info cache (stop-gap fix for D7)
Problem
- Data rebuild operations happening during
drupal_flush_all_caches()act on stale/outdated data of other modules and subsystems. hook_flush_caches()implementations rebuild data on stale caches. Then they return caches to flush, which may contain the data that was just rebuilt.
Goal
- Introduce
hook_rebuild()as unique rebuild operation. - Properly separate cache flushing and static variable resets from rebuild operations.
Details
drupal_flush_all_caches()clears caches, but also issues several kind of rebuilds.- The menu is rebuilt and module provided configuration-alike data structures (such as node types) are rebuilt.
- There are many interdependencies between the caches and rebuilds.
- Rebuilds may be based on other stale/old/invalid cached data, which hasn't been cleared yet.
- In turn, rebuilt data can be entirely bogus or incomplete.
Proposed solution
- Clean environment: Flush all database caches first. (
hook_cache_flush()) - Clean environment: Reset all static caches.
- Trigger module rebuilds. Guaranteed to be based on clean+fresh data. (
hook_rebuild()) - Rebuild the menu router. Guaranteed to contain most current data.
- Clear page, JS, and CSS caches. As an attempt to still serve cached pages during the entire operation.
API changes
hook_flush_caches()is renamed tohook_cache_flush()to ensure that modules are only clearing caches.hook_rebuild()is added as dedicated hook for rebuilding operations.menu_rebuild()(in menu.inc) is renamed tomenu_router_rebuild(), since the name clashes withhook_rebuild()when Menu module is installed.
Notes
- Some other D8 issue wants to introduce
hook_cache_bins(). That could be renamed intohook_cache_info()and it would cover the 90% use-case ofhook_cache_flush()here (returning names of cache bins that need to be flushed). - There's a long-time desire to split menu.inc into router.inc for the router system parts. So
menu_router_rebuild()might be renamed intorouter_rebuild()later. The new Symfony kernel might make that obsolete though, we'll see. - The new configuration system might make some implementations of
hook_rebuild()obsolete (those that attempt to synchronize active configuration with code definitions, such asnode_types_rebuild()). To be investigated later, doesn't matter for this issue. - This patch unintentionally speeds up cron runs. Cron invokes
hook_flush_caches()but only wants to flush caches. Many of the current implementations are performing additional rebuild operations (e.g., Block module).
Comments
#1
I don't think two simple rebuild and cache clear stages are going to suffice, as we have multiple different kinds of rebuilds in our process. First off we've underlying rebuilds for applying module provided configuration (read: exportables), then we've something like menu_rebuild(). However, rebuilding the menu basically requires most caches to be updated for changes to take affect, while rebuilds for applying module provided configuration are really foundational and need to happen before clearing caches, so caches are updated afterwards.
Thus, I think we should
1. Rebuild the registry
2. Apply module provided configuration (node types, system info, ..) ---> General rebuild phase.
3. Clear *all* caches, including all statics so changes take affect afterwards. ---> General cache clear phase.
4. Rebuild the menu, what's going to populate lots of caches again.
Then, we should make sure no module calls menu_rebuild() in an API function, but makes use of that:
<?phpvariable_set('menu_rebuild_needed', TRUE);
?>
instead. Maybe we could improve that by having menu_rebuild() to default to this behavior.
Idea:
Using the "menu_rebuild_needed" flag, makes menu_rebuild() work like any other permanent cache flush. Thus, we could even don't bother with statics and just make sure all permanent caches are cleared in phase 3.
However, I guess having all statics cleared after drupal_flush_all_caches() would be a desired behavior anyway.
#2
Possibly related: #1387776: Move some rebuilds outside of Drupal
#3
I partially disagree with the proposed order.
To clarify what you described in #1, we have:
Unless respective API functions have a primed static cache, the database cache is re-loaded.
Without primed static cache, respective API functions attempt to re-load from database cache. Otherwise, recollect/reconcile/rebuild from scratch.
Synchronizing the system's knowledge about existing classes, modules, and themes with actual filesystem data.
Synchronizing stored data in database with code changes. Typically priming static caches with results again. Also, these can have dependencies on rebuilds of other modules - typically following module dependencies (as declared in .info files).
Final update of router data to match new system state.
And that's basically also what I'd precisely envision for dfac() - to repeat:
hook_flush_caches(): Flush all database caches and static caches you have, so we can continue from a clean slate. (Static caches could be taken over by dfac().)registry_rebuild()(implyingsystem_rebuild_module_data()) +system_rebuild_theme_data(): Hard-coded update of class and extension data.hook_rebuild(): Rebuild whatever data you have that may be based on code.menu_build(): Hard-coded rebuild of the router, so following executed code and requests know about obsolete and new stuff.cache('page')->flush(): As a special exception, intentionally flush the page cache last, so any requests hammering the site during dfac() can potentially still be served.Notes:
hook_rebuild()would ideally be invoked in the order of module dependencies.hook_rebuild()implementations manually calling intomenu_build()are broken code and we do not babysit broken code.hook_menu()implementations do not manually call into any rebuild functions.#4
How would you like do that? This would mean we'd have to look the whole system, so no cache can get initialized in-between. However, I think if we'd have a solid order of steps with each step being locked, we should be already safe and minimize the down-time.
Agreed.
Makes sense. However, it sounds quite complicated and I can't think of any case requiring that right now. So, going with regular module weights and/or module controlled ordering via hook_module_implements_alter() should do it for now? We can still go and improve it once the need for it really comes up.
I don't think so any more. Point 3) (System components and extensions update) does not depend on 1) and 2) (permanent and static cache clears) in any way, however there are dependencies the other way round. Thus I think point 3) should run first.
Similarly, I think point 4) (Data structure and configuration rebuilds) usually won't require updated caches neither, as reading configuration files and synching them into the db mostly depends on 3). Furthermore, rebuilding items might issue quite some changes and cache-clears - thus flushing all caches afterwards would save us from quite some repetitive cache flushes. Next, rebuilt items won't be properly reflected in caches if the modules in question do not properly clear all affected caches, what sometimes is not even properly possible due to cache-dependency-chains (cache A depends on cache B, but modules clears only B so A stays outdated). Only doing a full-cache clear after rebuilding would guarantee completely fresh caches.
Thus, I'd suggest the following order:
Thinking about it again, the best solution probably boils down to the rebuild-operation one thinks of. Rebuild operations that sync rather foundational, defined configuration items (views, content types, rules, ...) should run before cache clears to make sure caches are properly flushed once, but the other way round rebuilding data that depends on quite some other data and configuration (menu, blocks) should run last and after clearing caches.
Thus, we might even want to go with two different rebuild steps or would require the latter rebuilding steps to move to a cache-like approach: 1. Clear caches/set the rebuild flag. 2. Rebuild once on de-mand and cache results. We even have that flag in place for the menu.
However, we really cannot skip the first rebuild step without having a repetitive drupal_flush_all_caches() to force everything is updated.
#5
Enhancing the lock system to allow for a special, global lock doesn't sound that bad, actually.
Nope. Consider:
1) After clearing caches
2) assume you need to rebuild
3) another request primes the caches again
4) call into the rebuild hooks, then
5) either nothing is rebuilt, or
6) the world ends with a nuclear disaster, fatal error.
Yeah, potentially. (It sounds like a very good idea to do though.)
They do, heavily even. Especially this kind of data is typically cached right within the function that builds and returns it.
Even essential stuff like module_list() itself relies on caches. What we're after is a clear separation between
1) flushing any and all old data and caches, and
2) (re)building data structures and configuration
This is essential. We do not want to have any old data in these operations, which (naturally) can only be stale and outdated.
Nope. Data is only rebuilt if there is no cached data in the first place. Again: We need a clear separation of stages and concepts.
A rebuild operation might set a cache after rebuilding - but that is fine, as long as we can be sure that everything was built from scratch.
"Not properly clearing all caches" during the (first) cache flush operation of dfac() means broken code.
We do not babysit broken code, so there cannot be cache dependency chains for rebuild operations in the first place.
Yes, I totally agree that all caches need to be flushed. However, caches are only created when they are built. And that's what the rebuild operation is for (though not everything is directly cached upon rebuild).
To make sure this really gets across, as I really can't stress enough on it:
You cannot reliably rebuild data structures and configuration, if there is a possibility of a function returning old/stale data somewhere in between.
Especially because of the module data dependency chains in the rebuild operations it would be utterly wrong to attempt to (re)build anything while having a chance of stale data anywhere.
All rebuild operations need to be able to rely on the fact that all caches have been cleared.
Without this essential difference in the flow, we're not going to improve anything. If there's any hook implementation deep down the rabbit hole that returns stale data from an old/outdated cache, then the data you are trying to (re)build is fucked up.
That's the exact systematic problem that #996236: drupal_flush_all_caches() does not clear entity info cache revealed.
The new paradigm has to be: Kill it. Build it.
#6
@sun:
I'd not say so. I think we are not talking about the same kind of 'rebuild'. So let's try to clarify what we mean with 'rebuild':
To me 'rebuild' is an operation that prepares data, that is used afterwards. There is a difference to caches in that, that the system relies upon this built data to be there afterwards, whereby caches can be refreshed anytime they have been cleared.
Examples of that would be the registry or synching node-types to the database. If node-types have not been imported, they are just missing as the system won't be able to determine their absence.
That makes it in particular problematic as caches might require those items to have been rebuilt in order to not contain stale data. In case of caches it's easier, as you can just clear everything and caches are re-initialized just on-demand, thus automatically building in the right order. Given that, we have to rebuild *before* the complete cache clear in order for caches to be able to reflect the rebuilt data.
(on cache vs rebuilding data: I already tried to handle rebuilding exportables like a regular cache clear by doing a rebuild on-demand with exportable entities, what results entity-crud hooks being possibly triggered by a simple entity-load operation. That way quite some ugly nested hook-invocations as seen in #1267070-7: Lazy-rebuilding the defaults for exported entities might not be desirable occur, what we better avoid by handling rebuilds explicitly.)
Example:
Node types need to be rebuilt before the entity info cache is initialized. Flushing all caches first, then going in rebuild phase probably ends up with someone triggering entity-info cache building before node types can be rebuilt, and voila your entity-info is outdated.
Possible solution:
1. complete cache clear
2. rebuilds
3. complete cache clear
I do think though the first complete cache clear is not necessary for most kind of rebuilds, however I agree that we should clear module lists first (as required by the registry rebuild).
->
1. clear module caches (no dependencies)
2. rebuilds (may depend on modules being up2date)
3. complete cache clear
If you do think of rebuilds that require more caches cleared, please give some examples.
#7
Extremely simple example of how not flushing caches upfront and not being able to rely on a clean slate fails:
/*** Implements hook_rebuild().
*/
function mymodule_rebuild() {
// Delete all mappings that no longer exist.
$entity_info = entity_get_info(); // FAIL.
$mappings = db_query('SELECT id, entity_type FROM {mymodule}')->fetchAllKeyed();
foreach ($mappings as $id => $type) {
if (!isset($entity_info[$type])) {
mymodule_mapping_delete($id);
}
}
}
Albeit abstract, there's most likely no simpler example.
If you really want just one of plenty real-world examples that involves a plethora of cached data in statics and database, witness _block_rehash().
There's no point in flushing caches after rebuilding. You've either rebuilt everything from scratch and from a clean slate, or you did not. At the point you're no longer sure, the system is unreliable and the design of the process broken.
#8
So what's with the example I've given then?:
The same holds for quite some other exportables that need to be rebuilt. To apply those exportables though, we'd require another cache clear. Thus, are you suggesting we should do it twice?
Indeed, that one needs to happen after all caches have been cleared. Probably best we'd just have two hooks, one before flushing caches and one afterwards. :/ Let's face it, we have real-world use-cases for both.
#9
ok, here is a patch overhauling cache-clearing in 3 phases. Let's see what the test-bot says.
Patch also fixes revamps simple-tests resetAll() methods to rely more on drupal_flush_caches() - a good proof it's working right (or not). Also, fixed a test-problem in the comments fields tests where previously caches were rebuilt without the full module list being loaded.
#10
The last submitted patch, drupal_8_flush_caches.patch, failed testing.
#11
>Drupal installation failed.
Cannot reproduce. :/
#12
#9: drupal_8_flush_caches.patch queued for re-testing.
#13
The last submitted patch, drupal_8_flush_caches.patch, failed testing.
#14
The situation you are describing can only happen in the current system, the broken system we need to fix.
In the new system, it does not matter what is rebuilt first or later, and which function primes its cache(s) after rebuilding. It does not matter at all, because we're running from a clean slate and everything has been reset to NULL in the first place.
Therefore, there cannot be a wonky dependency between node types and entity info caches. If one depends on the other, that's perfectly fine: the other is rebuilt, too. It's only rebuilt once, and only needs to be rebuilt once, because the new flow and clear separation between
hook_flush_caches()andhook_rebuild()ensures that everything old we knew before is forgotten, and the registry and extensions are updated before we reach the rebuild stage.Thus, if you happen to have a rebuild that primes a cache and it doesn't contain everything that's there, then that's obviously a major bug and broken code in the module that should have returned its data when the data was rebuilt.
Your argumentation would mean that Drupal wouldn't be able to build (not rebuild) the data it requires when starting from null, nada, scratch, ground zero, no pre-existing information. That's not the case, Drupal is and must be able to do so.
What this API change needs to achieve is to essentially make dfac() the system get "back" to that null state. Only after doing so, we have a reliable system state and are able to [re]build information. (Depending on the individual implementation, it's not necessarily a "rebuild", it may just be a straight "build" (because there's no previous information anymore).)
+++ b/core/modules/node/node.module@@ -765,6 +765,13 @@ function node_type_cache_reset() {
/**
+ * Implements hook_flush_caches_prepare().
+ */
+function node_flush_caches_prepare() {
+ node_types_rebuild();
+}
Sorry, but this is zero improvement to the current dfac() nightmare and getting nowhere. This doesn't separate concepts at all.
#15
I agree with you that ideally, we'd just have cache-like systems that we can flush + rebuild afterwards. However, in reality we haven't.
We have lots of such "broken code":
All of those are not flush-able like caches, so that they re-initialize on demand. If you just clear those data, then necessary information won't be rebuilt - it will be missing.
Let's consider the example of node types. First off there is
node_types_rebuild()what is necessary to detect new/gone-away node types from modules. Then there isnode_type_cache_reset(), which is about clearing the node-type cache. Clearing the cache won't help you to discover new node-types. Deleting all node-types from the db won't help you to automatically discover them either, you just loose your customizations and your non-code node types. That is why node_types_rebuild() is invoked explicitly on module enable / disable and dfac.#16
Attached patch implements a clear and concise separation between cache and rebuild concepts.
#17
The last submitted patch, drupal8.flush-rebuild.16.patch, failed testing.
#18
Meantime, I realized my proposal won't fly. Rebuilding even foundational stuff without properly cleared caches partly relies on a working system to do even this foundational basic rebuilds. However, we really should be able to recover from any wrong, incomplete or whatever caches by running dfac(). Thus, finally +1 for sun's approach even if it means some caches have to be re-freshed twice.
Ideally, I guess rebuilds should try to clear all necessary caches after rebuilding. E.g., like rebuilding node-types already triggers a menu rebuild. That said, rebuild operations should offer hooks other modules can use to clear necessary caches (= node type CRUD hooks).
function block_flush_caches() {+ return array('block');
+}
+
+/**
+ * Implements hook_rebuild().
+ */
+function block_rebuild() {
This requires the global lock to work, else we might end up with old blocks.
#19
We should figure out what's wrong with the testbot first. I'm able to successfully install Drupal with this patch applied and do not see any kind of hiccups.
#20
Possibly related: #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations
#21
#16: drupal8.flush-rebuild.16.patch queued for re-testing.
#22
The last submitted patch, drupal8.flush-rebuild.16.patch, failed testing.
#23
Hrm. Re-rolling this patch is painful.
#24
The last submitted patch, drupal8.flush-rebuild.23.patch, failed testing.
#25
I'm not able to reproduce the installation test failure locally -- neither through the UI, nor via run-tests.sh.
Regardless of that, trying whether further adjustments to install_finished() make any difference.
#26
+++ b/core/includes/common.inc@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+ * @todo Rename hook_flush_caches() to enforce developers to re-implement it
+ * according to this contract.
I think this is a good idea. Not sure whether to do it in this patch or in a follow-up issue though?
+++ b/core/includes/common.inc@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+ * @todo Make this obsolete:
+ * Note that after clearing all static caches also the theme is not initialized
+ * any more and any possible previously added Javascript or CSS is gone.
I've no idea how to resolve this. (this is actually the same comment as the @todo in install_finished())
+++ b/core/includes/common.inc
@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+ // Clear all non-drupal_static() static caches.
+ // @todo Grep entire core for statics that need to be reset and can be reset,
+ // and reset them here, or for modules, in hook_flush_caches().
There are actually no non-drupal statics to reset currently. But it would be good to keep the comment (without @todo), in order to state that - if regular statics need to be reset at some point - then they should be reset here.
+++ b/core/includes/install.core.inc@@ -1571,28 +1571,34 @@ function install_import_translations_remaining(&$install_state) {
+ // Re-initialize the maintenance theme.
+ // @todo drupal_flush_all_caches() resets everything, and afterwards rebuilds
+ // various data structures, among which menu_rebuild() invokes hook_menu()
+ // implementations, which may trigger a theme initialization (e.g., Block
+ // module defines separate menu items per theme).
+ unset($GLOBALS['theme']);
+ drupal_maintenance_theme();
This @todo is actually wrong. It should instead clarify that this is needed, because the drupal statics of list_themes() and drupal_add_js/css() are completely reset by dfac(), so the install finished page would only contain the bare/raw text output without any maintenance theme styling.
+++ b/core/modules/system/system.module@@ -2531,6 +2531,7 @@ function _system_update_bootstrap_status() {
+ // @todo This the caller's job and not always necessary.
system_list_reset();
Let's fix the calling functions accordingly as part of this patch.
#27
+++ b/core/includes/common.incundefined
@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+ // Rebuild all information based on new module data.
+ module_invoke_all('rebuild');
....
- }
+ // Lastly, rebuild the menu router based on all rebuilt data.
+ menu_rebuild();
Doesn't this run menu_rebuild twice?
-24 days to next Drupal core point release.
#28
@aspilicious: Nope, menu_rebuild() is not a hook implementation of hook_rebuild().
Resolved all issues in #26:
Note: If this patch fails, then we need a system_list_reset() after invoking it in dfac(). (or re-order the invocation)
#29
The last submitted patch, drupal8.flush-rebuild.28.patch, failed testing.
#30
whoopsie -- didn't catch all instances of 'flush_caches' throughout core ;)
+++ b/core/includes/common.inc@@ -7289,50 +7289,88 @@ function drupal_implode_tags($tags) {
+ * - Clears page and asset/support file caches (last, in order to serve requests
+ * from cache during re-initialization, if possible) @todo While a good idea,
+ * this breaks the contract and doesn't work out, since cache_bootstrap would
+ * also have to be retained in order to serve pages from cache. :(
whoopsie -- missed the inline @todo here ;)
This patch is RTBC from my perspective.
#31
#32
Skim read the issue, so may be missing out anyone bringing this up, but perhaps a less generic hook then hook_rebuild would be good?
Feels like it could cause some confusion when newer people see it vs. registry_rebuild, menu_rebuild, etc. and also limits the namespace (can't have a module named registry, menu, etc.). Also, seems like a possibility of mistakenly naming a function [module]_rebuild and accidentally implementing that hook XD!
#33
Replaced "database caches" with "persistent caches."
#34
i really like this patch. a couple of smallish things:
- i think 'database caches' reference should be replaced with 'persistent caches'
- the moving around of _system_update_bootstrap_status() could do with some comments, reading the patch it's unclear why that's important
- i think we should implement system_cache_flush() and return those bins, rather than special casing 'core' bins
#35
Based on #34:
1) renamed (in #33 already)
2) The system_list_reset() is performed in dfac(), module_enable(), and module_disable() already. _system_update_bootstrap_status() invoked it another time, needlessly.
3) Incorporated the system_cache_flush() suggestion. Thanks!
#36
Minor phpDoc adjustments.
#37
ok, i think this is ready to fly. setting to RTBC so catch will see it.
#38
Huh. I have a lot of problems with the new doxygen. The old had examples which seem to be gone. Also,
+ * Flushes and resets all caches, and rebuilds data structures.
I presume you mean
All static caches are reset and all cache bins are flushed. And then what is rebuilt...? That's where it gets confusing. What about adding to+ * - Rebuilds and synchronizes data structures (invoking hook_rebuild()).
"The following core modules implement this hook: block doing bananas" etc. The big problem is that without an intimate knowledge of core you have no chance to know whether something is rebuilt on this call or the next request when a cache miss occurs and is stored into cache.
#39
#38 asks for phpDoc clarification on what subsystem caches are cleared and what subsystem data is rebuilt. Also, to clarify in phpDoc that, in fact, the menu router has not been rebuilt yet when hook_rebuild() is invoked.
#40
Applied the patch, executed update.php, then my theme information was lost on the final confirmation page.
Ah-ha. Then how do you explain this?:
<?phpvar_dump(module_implements('rebuild'));
array(5) { [0] => string(5) "block" [1] => string(5) "field" [2] => string(4) "menu" [3] => string(4) "node" [4] => string(6) "system" }
?>
We do have a menu.module....
#41
Hahahaha. I mean, stabbity stab stab.
#42
Note: Doing this revealed many calls to menu_rebuild() throughout the system that look very dubious to me. I'll create a follow-up issue to analyze whether those invocations are actually correct and needed, or whether we're unnecessarily rebuilding the router repeatedly. In any case, separate issue.
Also updated the issue summary with API changes, as well as some more notes about other/related efforts elsewhere.
#43
The last submitted patch, drupal8.flush-rebuild.42.patch, failed testing.
#44
+++ b/core/includes/common.inc@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+ menu_rebuild();
D'OH! Forgot the instance in dfac() ;) No other change, so the last interdiff still applies.
#45
The bastard is green! Good work! ;)
+++ b/core/includes/common.inc@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+ * This means the entire system is reset to be effectively empty. After that is
The entire system isn't empty, the content stays. This should be phrased better.
+++ b/core/includes/common.inc@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+ * @todo This function resets all static caches. The theme is not initialized
+ * anymore and all previously added JavaScript and CSS is gone. But since that
+ * is the whole point of this function, all callers need to take care for
+ * backing up needed variables and properly re-initializing the theme on their
+ * own. See install_finished() for an example. Normally, this function is
Outdated comment? It's now set to the maintenance theme and install_finished() doesn't re-initialize anything.
+++ b/core/includes/common.inc@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+ // Rebuild the schema and cache a fully-built schema based on new module data.
+ // This is necessary for any invocation of index.php, because setting cache
+ // table entries requires schema information and that occurs during bootstrap
+ // before any modules are loaded, so if there is no cached schema,
+ // drupal_get_schema() will try to generate one, but with no loaded modules,
+ // it will return nothing.
+ drupal_get_schema(NULL, TRUE);
ouch! Imho that should be fixed in the bootstrap.. Well, I guess that's a no trivial task and is best done by the bootstrap cleanup anyway. Maybe we should add a @todo here to remove it once the bootstrap has been cleaned up?
+++ b/core/modules/system/system.api.php@@ -2087,22 +2087,63 @@ function hook_mail($key, &$message, $params) {
+ * This hook is only invoked after the system has been completely cleared;
+ * i.e., all previously cached data is known to be gone and every API in the
+ * system is known to return current information, so your module can safely rely
+ * on all available data to rebuild its own.
hm, not necessarily. There might be data that needs to be rebuilt in order to be available to the module. E.g. consider a block_node module "building" blocks for each node type using a rebuild mechanism - it won't see new node types as hook_node_rebuild() is invoked later on by default. Thus, we should clarify that the order of rebuilds might be important (which can be altered as usual for hooks).
That said, modules rebuilding data should clear their caches *again* once the have rebuilt the data. Taken the node-block example, the block rebuild might have primed the node type cache which might contain old information once node types got rebuilt. Thus, node_types_rebuild() should make sure to update/clear the node-type cache once rebuilt.
Also, that shows that rebuilding data is more painful than the regular cache-approach of building the data on demand, as possible dependencies have to be resolved manually. Maybe we should try to clarify on when it is a good idea to pursue which approach (cache vs rebuild)?
#46
1) Fixed docs on "empty."
2) Fixed docs on the theme reset. (removed the todo)
3) The schema reset and corresponding comment is not touched by this patch, merely relocated. Separate issue.
4) Honestly, I'd like to see a separate issue for the multidimensional rebuild/cache issue you're describing. Because I don't really get what you're trying to explain. ;) That issue will be able to rely on hook_rebuild() being there, so can focus on that operation only, and can properly discuss that detail. Discussing it in here only leads to more confusion around this patch. I'd really like to get rid of this issue here first, because the change is anything but trivial.
EDIT: Last patch had "0 passes". Let's watch out.
#47
I added further doxygen. Yes, we usually don't list the core implementation of a hook but this is so hard more documentation can not hurt.
#48
Still "0 passes."
#49
updating patch for latest commits, reroll only except for a menu_rebuild() call in install.core.inc which was removed in latest head.
#50
hmm, there was a menu_rebuild() in standard_install() that wasn't ported over.
#51
The last submitted patch, 1404198-50.patch, failed testing.
#52
#50: 1404198-50.patch queued for re-testing.
#53
The last submitted patch, 1404198-50.patch, failed testing.
#54
wow. so, inside the deep rabbit hole i just dug around in, i discovered something rather obvious.
in the parent drupal, when you properly blow away all caches and rebuild node types, you don't get any new node types from modules enabled in the child drupal. because module_implements(), $deity bless it, checks to see if you have loaded the module's implementation before returning it.
so, adding the three lines at the bottom to testCommentEnable() makes the test pass:
<?php// Enable core content type modules (book, and poll).
$edit = array();
$edit['modules[Core][book][enable]'] = 'book';
$edit['modules[Core][poll][enable]'] = 'poll';
$this->drupalPost('admin/modules', $edit, t('Save configuration'));
// YEAH! Load that code!
drupal_load('module', 'poll');
drupal_load('module', 'book');
?>
but is also clearly completely wrong.
#55
here's a patch that makes DrupalWebTestCase::resetAll() load all code for enabled modules before calling drupal_flush_all_caches() to make sure we don't rebuild stuff in the parent Drupal and miss any implementations.
so the only change #50 is to resetAll():
<?php
protected function resetAll() {
// Make sure we load the code for all enabled modules. Module's enabled via
// form posts from the parent Drupal will not have their code loaded. This
// can lead to module hooks not being returned from module_implements(),
// which will check that hook implementations are in memory.
foreach (module_list(TRUE) as $module) {
drupal_load('module', $module);
}
// Clear all database and static caches and rebuild data structures.
drupal_flush_all_caches();
// Reload global $conf array and permissions.
$this->refreshVariables();
$this->checkPermissions(array(), TRUE);
}
?>
#56
umm. with patch this time.
#57
Thanks for debugging this, @beejeebus!
So this patch should actually prevent that from happening in the first place. dfac() is supposed to be able to fully re-initialize and build or rebuild Drupal into a state in which it can fully operate. Regardless of whether there currently is bogus or outdated information or not.
The actual cause and problem is the static variable in module_list().
Apparently, module_list() calls into system_list(), which uses a drupal_static() already, so the system list cache is fully flushed and reset by dfac(), but module_list() is not.
That can obviously call for trouble only. So I'd suggest to fix that static instead.
Interdiff is against #50.
#58
The last submitted patch, drupal8.flush-rebuild.57.patch, failed testing.
#59
Thankfully, #1541958: Split setUp() into specific sub-methods made the operations being performed in setUp() much more clear. To explain the fatal errors and quote from there:
#60
The last submitted patch, drupal8.flush-rebuild.59.patch, failed testing.
#61
So #59 resolved a lot of failures. Now dfac() only needs to ensure that all modules that are supposed to be enabled are actually loaded. Added in this patch.
#62
Victory! :)
That said, if we don't want to introduce the new drupal statics, then we could as well reset them manually within dfac(). However, I think that adding them is the proper change. Especially because of the module_list() vs. system_list() difference.
#63
i want to set this to RTBC, but i guess fago should have a look first?
#64
If #1541958: Split setUp() into specific sub-methods lands first, this patch will need to be re-rolled. To do so, just skip/remove the hunk in drupal_web_test_case.php.
#65
True. Ok, let's do so.
+++ b/core/includes/module.inc@@ -444,10 +461,10 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+ _system_update_bootstrap_status();
system_list_reset();
module_list(TRUE);
module_implements_reset();
- _system_update_bootstrap_status();
Why do we re-order that in module_enable()/disable()?
That looks problematic, as updating the bootstrap status relies upon module_implements() which relies upon module_list(), thus both caches need to flushed before we call this function. Otherwise, the new module might be missed my _system_update_bootstrap_status().
That said, clearing all that different module-related function and their interdependencies is quite a mess too. This would deserve its own clean-up issue.
Else, patch looks good to me.
#66
The concern in #65 sounds valid to me. Thus, I've reverted all changes to _system_update_bootstrap_status() in attached patch.
#67
Good!
#68
Hm. I'd prefer the change for setUp() to land with the follow-up patch in #1541958: Split setUp() into specific sub-methods instead of this one, because the follow-up fix really belongs over into that issue/patch.
#69
Committed to 8.x. Big clean-up. Thanks for all the careful reviews.
#70
Thanks. This is a huge API change and obviously needs a change notice. :)
#71
Created two change notices:
http://drupal.org/node/1561490
http://drupal.org/node/1561492
#72
Nice patch, but the name menu_router_rebuild() is misleading. We are rebuilding two subsystems here - menu router and menu links. Conflating the two is a step backward IMO. Hopefully we can fix that in a followup.
#73
Awesome!
Is there a followup issue for #72? If so, can we link it here, and if not, should we file one?
#74
Hm. I'm not sure whether we need that follow-up - the Web services initiative will rewrite the router system anyway. It's possible that these changes will remove the double notion.
#75
Automatically closed -- issue fixed for 2 weeks with no activity.