[meta] system info cache writing and registry rebuilding is broken without a full bootstrap

great, subscribing :)

Hm, this might want to go into update.php? Esp since that has access control and I feel like some access control would be great for this...

Setting to CNW then. I agree access control is needed.

Added the registry_rebuild() to update.php. Not sure where it's best to put the menu_rebuild() and drupal_flush_all_caches(), but this works for the registry recovery anyway

Pretty quickly, visiting update.php is going to be as slow as modules page. I think we should add a recovery button to the current first page of update.php. that submit handler can flush caches and rebuild menu.

I think it's fine for update.php to be slow, not averse to a 'fix my site' button either though.

I don't like running the update process to rebuild the registry so I favor Moshe's idea of a submit handler on a button. But update.php having the use of a "recovery" or rebuild is misleading.

Well, update.php needs to handle rebuilding the registry anyway, because a module adding or removing a hook implementation can hose your site completely.

Sure, so to clarify the point of this issue is the ability for someone to just run registry_rebuild() because something got hosed. update.php does that, I'm just stating that I think update.php has been and should stay about updating modules and something new created for this issue. But, I understand not duplicating code and not adding another top-level file so perhaps Moshe's idea is the route to go.

What's the status of this?

The suggestions seem to be

1. Add recovery.php that will rebuild all registries and flush all caches.
2. Add a recovery button to update.php.
3. Automatically flush caches whenever update.php is called.

Number 3 has the drawback of being hard to find and incurring a performance cost. Number 2 may also be hard to find, while number 1 introduces a new top-level file when we already have three (authorize.php).

We still also really need a "fix my site" button (and may also a function that can be called by drush and the like)

This is really impeding development for D7.

Just got bitten by this #1134798: EntityCacheTaxonomyTermController not found.

Committed to 7.x and 8.x. Thanks!

Still needs to go in D7.

#5: registry_rebuild.patch queued for re-testing.

I pulled 7.x and this made it in, not sure why David thought this hadn't been applied - maybe git log isn't handling commits to both branches so well?

Unfortunately the comment in the patch dates back to when we had the function registry, now we don't.

Reasons not to commit patches from 'code needs review' part 293.

Ok, committed the rollback to 8.x and 7.x. This broke (as in fatal error, destroy-your-site kind of broke) sites with CTools module, at least.

Discussed this with beejeebus, merlinofchaos and webchick in irc and this whole issue is even more of a snake pit than we thought.

http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/regist...

registry_rebuild() depends on system module.

system_rebuild_module_data() invokes hook_system_info_alter() - but there is no guarantee that all modules are loaded when that hook runs.

_registry_update() invokes hook_registry_files_alter(), but again there is no guarantee that all modules are loaded when that runs.

This patch added a call to registry_rebuild() before a full bootstrap.

ctools implements hook_registry_files_alter(), and the classes it is registering are used all over the place.

Boom!

At least for Drupal 7, I think we need to do something like the following. For Drupal 8 this will require dismantling most of system module and having a module system that doesn't depend on itself and a class autoloader that either doesn't depend on the module system or doesn't pretend to be useful for anything other than classes in modules.

But for now:

1. registry_rebuild() should add a full bootstrap right inside, possible we should even move it to system.module or common.inc to enforce this slightly more.

2. No class that can ever be used before full bootstrap can use the registry for autoload, that includes core classes, otherwise we can never, ever move them safely.

3. No hook_boot() implementation can rely on the registry for autoload, that's the same as #2, hmm.

Another possibility is to add bootstrap_alter(), make both hook_registry_files_alter() and hook_system_info_alter() bootstrap hooks, and then registry_rebuild() might work at a lower bootstrap level, but that won't remove the recursive dependency, it'll just be adding a (quite ugly) bandaid to it.

None of these solutions are good so it is a case of which makes us throw up least.

Modules that own classes that want to move them, will have to add registry_rebuild() in a hook_update_N() until this is sorted out, this worked for #1134798: EntityCacheTaxonomyTermController not found.

Re-titling slightly.

Not enough docs but I wonder if this is enough to make it work.

+++ b/modules/system/system.module
@@ -2383,6 +2383,7 @@ function _system_rebuild_module_data() {
+    module_load_all();

IMHO we'll only introduce more weird and unexpected behaviors by automatically loading all modules here.

Instead, the calling code should have to ensure that the environment for executing a command is properly setup before executing the command.

In turn, I'd rather like to see a module_load_all(NULL) condition here (or at the beginning of registry_rebuild()) that merely checks whether all modules have been loaded, but doesn't autoload them. In case they were not, we throw an exception or do whatever else that halts processing and ensures the command is not executed and the caller gets to know of the bogus invocation attempt.

Powered by Dreditor.

I'm not sure that will work for system_rebuild_module_data(), but there's an easy way to find out.

However throwing the error doesn't deal with the original point of this issue, is that moving classes around completely hoses installs, maybe my hook_update_N() (and a handbook page with instructions on how to hack index.php) is enough though? Then docs, then checking we don't use autoload for anything in /includes.

@sun, this is broken beyond repair in D7 so we need something to fix it now, not how we'd like it to eventually work (which is going to require very large API changes), I'm marking back to CNR to give some other people the chance to review that approach.

Subscribe

Re-setting.

@justinrandell - for cases that can't be fixed with loading all modules, then that seems like all we can do for classes that are needed prior to full bootstrap in Drupal 7.

This is an issue where things are broken either way, so if we can handle both kinds of broken in this issue as much as is possible that sounds good to me!

For testing this, if we go with the module_load_all() and/or force a bootstrap, we could do the following:

Two testing modules, one implements hook_boot() and calls registry_rebuild().

The other implements hook_system_info_alter() and hook_registry_files_alter() - just does drupal_set_message('something'); in each.

Then enable both modules from the test, hit a page, check for existence of both of the messages, ensure they don't show up when the hook_boot() module is disabled, that'd be it.

If we can hit update.php from simpletest (can't remember if we can or not) we could add the same check for that, minus the hook_boot() module.

If instead we went for the module_load_all(NULL) check that sun suggested, we'd be enforcing that every one of these functions can only be called after a full bootstrap:

http://api.drupal.org/api/drupal/modules--system--system.module/function...

That's a lot of functions, like I said just uploading the patch that does that should tell whether that's possible or not.

As an example. I don't think that update.php allows to invoke registry_rebuild() - only system.module is loaded.

sub.

Changing the title.

This is why #38 breaks:

function install_profile_modules(&$install_state) {
  $modules = variable_get('install_profile_modules', array());
  $files = system_rebuild_module_data();
  variable_del('install_profile_modules');

So I'm re-uploading #28 with an extra comment. Still needs a test but running by the test bot.

If we want the install system to not depend on system module, and I think we do, then we can work on that as part of the overall effort at #679112: Time for system.module and most of includes to commit seppuku, but that is not a critical Drupal 7 issue.

Will add tests later this evening or tomorrow if it's doable.

With tests.

+++ b/modules/system/system.testundefined
@@ -1868,10 +1868,21 @@ class UpdateScriptFunctionalTest extends DrupalWebTestCase {
   }
+ ¶
+  /**
+   * Confirm that the call to registry_rebuild() on update.php fires successfully.

Trailing whitespace.

Nice test :)

Powered by Dreditor.

Fixed the whitespace.

This problem got me yesterday...
Fixed it my self, else I could test this.

rfay posted http://drupal.org/project/registry_rebuild

However we still need registry_rebuild() / _system_rebuild_module_data() to either handle loading all modules or not allow itself to run even if we were to completely give up on fixing this problem in core for D7.

looks reasonable, but need to review further.

Just spent over an hour discussing this with chx and beejeebus in irc, chx may have come up with a way around this that kills at least some of the circular dependencies:

- Add a variant of system_rebuild_module_data() that doesn't invoke any hooks (i.e. hook_system_info_alter()) - but just finds module files etc.
- Deprecate files[] completely.
- Remove hook_registry_files_alter()
- registry_rebuild() scans all modules for .module and .inc files, and adds them to the registry regardless of whether the module is enabled or disabled.
- the registry no longer differentiates between enabled or disabled modules - classes are added regardless.
- registry rebuilds will be more expensive, but we can likely avoid doing so as often as we currently do, so there might be a performance trade-off but it won't be a straight hit, and it'll only really be on rebuilds (the table size is not going to grow so much that runtime registry queries get noticeably slower, they'll all indexed, and registry rebuilds shouldn't block processes from executing hopefully).

- Remove hook_registry_files_alter()

Libraries API needs this alter hook to register additional PHP libraries in */libraries directories.

I'll regret this later but... sub.

@sun: so if we wanted to account for that and still remove the hook, we'd need to scan the libraries directory if it exists and also scan .php files?

That's a tough question and I'm not familiar enough with fundamental principles of PHP OO auto-loading. Libraries need to be considered as external code; "PHP soup" if you will. They may contain code that's incompatible or shouldn't be used. Modules that use a certain library may only need a subset of what the library provides. (custom integration example) Lastly, note that libraries may exist in /libraries (distros), /profiles/*/libraries (installation profiles), and /sites/*/libraries (sites). Overrides are supposed to behave like module and theme overrides, but I guess/hope the current registry file parsing implements that already. In the end, we could simply try to hard-code the additional libraries paths to scan, even though it feels slightly sub-optimal to me.

OK, I'm moving this back to CNR. I wasn't aware that libraries module was using this hook, and that's a different use-case to the ctools one (which is just trying to avoid putting things in files[]), so we need more discussion.

We could force libraries module to implement it's own autoloader, but I imagine you wouldn't be overly keen on that.

I think that's going to break too much. If simpletest cannot use autoloading, let's just rework it, and not break the rest of core for that edge case.

cross posted - back to CNR

I think I agree with pwolanin though I'm not sure what his comment was responding too. We should make the registry behave as sanely as possible. The alter hook isn't being abused by ctools/libraries but the simpletest exceptions seem to be complicating things. We should optimise for core and the rest of contrib and solve simpletest separately.

subscribe

sub

Ironically, I didn't even know that this issue existed and was involved with the problems that caused the creation of Registry Rebuild. I thought it was just an entity/rules thing coming around to bite us. Thanks to all for your work on this.

I'm interested that the rollback happened about June 1, but I got bit only about June 20. And it seemed like there were piles of people affected in that timeframe. Could be that it came with the Entity/Rules/Commerce updates that forced the old version of this, and that I hadn't updated. Could be I guess.

So entity/rules/commerce may have been hitting the original bug this issue was intended to help with - which is that if you move/rename a class, you can end up in an unrecoverable position.

The patch that got committed to 7.2 and was rolled back, would have fixed this except for hook_registry_files_alter(), which ctools was using to register classes on behalf of other modules, and the fact this hook wasn't run from the registry_rebuild() on update.php meant that this also left people in an unrecoverable position.

I haven't checked the code of the registry_rebuild() project, but it sounds like the two rebuilds approach may cover both of these cases, I assumed you'd seen this issue when I saw that project, otherwise I'd have pinged you about it..

The current patch here does two things:

1. Enforces that hook_system_info_alter() and hook_registry_files_alter() actually get invoked when their parent functions are called, currently they are some of the time, but other times not - by forcing a module_load_all(). This was a new discovery that was only tracked down by Justin after 7.2 had hosed some sites. An earlier patch by sun caused them to throw an exception if they were called during early bootstrap, but core itself does this deep in the installer so IMO it's not happening for D7.

2. Adds the registry_rebuild() back to update.php again in the hope that D7 itself can provide roughly the same functionality as registry_rebuild project does when you hit update.php (which you should after updating modules). Randy if you're able to review that based on having done a similar thing in registry_rebuild that'd be really handy.

#45 is still valid, but if we wanted to just ensure the ctools issue can't happen again, here's a minimal patch that enforces modules are loaded when invoking hook_system_info_files_alter() and hook_registry_files_alter().

I really think it is a bad pattern for random parts of the codebase to do the job of the bootstrap. not a fan of that module_invoke_all().

@moshe. It's a bad pattern for functions available (and currently required) at any bootstrap level to be dependent on all modules being loaded. The patch names describe my feelings about the issue, perhaps you could suggest an alternative? Note that sun's patch to throw the exception would require refactoring large parts of the installer and modules system to break all these circular dependencies. That is something that desperately needs to happen in D8 but won't get backported.

As long as we fix it with the exception in D8 then I'm OK with hackola for D7 (i.e. module_invoke_all).

subscribing

I think we should go forward with #45, as long as that avoids the ctools breakage.

However, here's a re-roll of the last patch without the test part which is not relevant.

Tagging issues not yet using summary template.

removing issue summary initiative tag

Adding issue summary as part of http://groups.drupal.org/node/167534

Since there's no good solution for this in D7 (although I would still go for #45) and http://drupal.org/project/registry_rebuild exists, I'm demoting this to major.

Subscribe

subscribing

I'm not sure whats the motivation of the #68 patch, but it doesn't fix the issue in the D7. After moving the modules from /modules to /modules/contrib the site hangs and there is no standard way how to rebuild the registry cache (admin/modules, update.php, drush cc - none of them works).

www# drush cc
WD php: Warning:                                                                                            [warning]
require_once(/www/sites/dev/css1-001-dev/www/sites/all/modules/entity/includes/entity.inc): failed to open
stream: No such file or directory in _registry_check_code() (line 2913 of
/www/sites/dev/css1-001-2-dev/www/includes/bootstrap.inc).
Drush command terminated abnormally due to an unrecoverable error.                                          [error]
Error: require_once(): Failed opening required
'/www/sites/dev/css1-001-2-dev/www/sites/all/modules/entity/includes/entity.inc'
(include_path='.:/usr/local/share/pear') in /www/sites/dev/css1-001-2-dev/www/includes/bootstrap.inc, line
2913
Exit 255

@wojtha - you'd need the patch from #45 and visit update.php to actually repair a broken registry.

This issue now incorporates two bugs:

1. Can't rebuild a broken registry.

2. registry_rebuild() can easily create an incomplete registry if called before all modules are loaded.

Moving back to needs review since we have patches that try to fix both of those and mainly need to decide on whether we're fixing either.

I though the #68 by pwolanin was the same as the #45, just with the tests stripped. Now I see that pwolanin forgot to add registry_rebuild() in the update.php ... I'll try again.

#1327958: D8 upgrade path before /core move is completely hosed leaves unrecoverable, so I'm bumping this back to critical.

Attached just rerolls #68 for core/. Can someone clarify what is missing from this patch? I compared it with #45 and did not see anything that was removed other than tests.

It's the update.php hunk that's different:

diff --git a/update.php b/update.php
index a777920..c85f4c3 100644
--- a/update.php
+++ b/update.php
@@ -370,6 +370,9 @@ if (empty($op) && update_access_allowed()) {
   // in updated code are picked up.
   module_implements('', FALSE, TRUE);
 
+  // Rebuild the registry so that any classes moved by modules are rescanned.
+  registry_rebuild();
+
   // Set up $language, since the installer components require it.
   drupal_language_initialize();
 

Dave Reid reminded me about #1049284: Running update.php to flush caches no longer works unless there are updates pending, we could leave the registry building to that issue possibly (at least, for that issue to get fixed we need to correctly rebuild the registry on update.php as well as flushing other caches and registries).

Doing a full cache clear for #1049284: Running update.php to flush caches no longer works unless there are updates pending early enough for this issue makes update.php blow up in entertaining ways. So, I think the solution is separate. Attached is #79 with the registry rebuild from #80 added back in.

So, for the record, #81 will break update.php with both hunks, but not with just the one or the other.

Drupal.org hates me and I've tried to post this like 80 times. Anyway. Drupal is not sufficiently bootstrapped by this point in update.php for module_load_all(). I haven't yet determined how it was bootstrapped enough back in June in #28. Also, catch's patch names resonate. Also too,

registry_rebuild() should add a full bootstrap right inside, possible we should even move it to system.module or common.inc to enforce this slightly more.

Edit: And gettin' our bad selves bootstrapped might also let us move up the general cache clear action in #1049284: Running update.php to flush caches no longer works unless there are updates pending for consistency and general not-clearing-cached-stuff-piecemeal-in-5-places-in-update.php.

This is the difference between 85 and 86, for reference.

Does the registry_rebuild() need to happen before update_check_requirements() to resolve this issue? If not, we can move the registry rebuild a little later, when we're flushing a couple other caches, and avoid the need to manually bootstrap inside registry_rebuild(). Attached patch does that.

We still need a failing test for the bug that broke ctools as well as what's described in #75. Honestly I am not positive these patches even resolve it. Hopefully we can extend @beejeebus's test case for that.

Moving registry_rebuild() to system.module also seems plausible to me... here's what that would look like. No idea if this will blow stuff up, but hey, the upgrade tests pass.

+++ b/core/modules/system/system.moduleundefined
@@ -2391,6 +2416,10 @@ function _system_rebuild_module_data() {
+    // system_rebuild_module_data() is often called before a full bootstrap,
+    // such as in the installer. So to allow hook_system_info_alter() to run
+    // successfully, ensure all modules are loaded before invoking the hook.
+    module_load_all();

Okay, also. This comment doesn't even seem to be correct anymore. module_load_all() doesn't get you around needing a full bootstrap, because it itself goes boom without a full bootstrap. What has changed since June on this point?

Why does module_load_all() go boom without a full bootstrap? It should work as long as module.inc is loaded no?

I think this is well-earned.

So the code in field_system_info_alter() was added in #943772: field_delete_field() and others fail for inactive fields which I RTBC'ed, and itself fixed a critical.

We could try to do that without calling l() or t(), but this boils down to hook_system_info_alter() also needing everything available.

Discussed a bit with beejeebus in irc. If we do a full bootstrap (i.e. call hook_boot() or hook_init()) then we risk calling code that relies on the registry (which might be broken), so Drupal can fatal before we try to fix it. However a middle-ground is for probably _system_rebuild_module_data() to do a mini-full-bootstrap - i.e. include as much code as possible but don't invoke any hooks. This is going to look extremely ugly, but that's what we're dealing with.

we risk calling code that relies on the registry (which might be broken), so Drupal can fatal before we try to fix it.

Latest discussion on this issue makes it appear very similar to #996236: drupal_flush_all_caches() does not clear entity info cache, in which we're trying to solve the info/cache/registry/rebuild interdependency nightmare. There's at least one working patch already, which only has one remaining issue with the installer. I'd strongly suggest to solve that challenge in aforementioned issue.

And, given that #1049284: Running update.php to flush caches no longer works unless there are updates pending already deals with the invocation from update.php, I'm actually no longer sure what exactly this issue is about. The issue summary doesn't clarify either - can we clarify and update that?

Updated issue summary. Fixed broken link and updated API changes to none.

I've updated the summary.

Looks like #98 was against D7. Here's a D8 reroll. I checked the DRUPAL_ROOT but I think they are correct as-is since it is defined in update.php by getcwd() (and update.php itself is inside core/). If I'm wrong, testbot will tell us. :)

Okay I was wrong. Adding core/; will post reroll shortly.

--- a/core/update.php
+++ b/core/update.php
@@ -348,6 +348,54 @@ function update_check_requirements($skip_warnings = FALSE) {
   }
 }
 
+function update_rebuild_registry() {

Should this function be in includes/update.inc rather than update.php? That way it would be available to Drush also.

so what happens if you put your site in maintenance mode before you update?
The update destroys the registry and now registry_rebuild() won't be called because we are in maintenance mode...
And we can't disable maintenance mode because the site is broken.

Maybe we need to use drupal_maintenance_theme()?

The test looks good to me. Attached includes a bit of minor comment cleanup from #113, and I've uploaded the tests separately to expose the fails.

Looking at update_rebuild_registry() it seems to duplicate a LOT of code from the first part of _registry_update()... I'm not sure if it's worth the effort to make them share some of the code, but at the very least I feel like we need to explain what the differences are. Otherwise it's very hard to tell what this code is doing. It seems like the main differences are that it's a "hard rebuild" (forces all database entries in the registry to be wiped and regenerated), and that it skips stuff like hook_register_files_alter(). So basically, the bare minimum rebuild to get the site up and running. And that explains why it needs to set the 'registry_rebuild' variable at the end, so you can get a "real" rebuild on the next page load.

But the code comments currently don't tell me any of that, so I was left wondering if that was actually the case and which of the differences are critical or not.

For example, there are a bunch of other smaller differences: It uses RecursiveDirectoryIterator rather than file_scan_directory(), it uses drupal_parse_info_file() rather than relying on the data in the {system} table (is that because the data in the {system} table might be out of date?), etc. I'm not sure if those are just unintentional differences or if they actually matter because we're early in the bootstrap on a potentially broken site.

Beyond the missing info, this looks really good though. It's the kind of patch where if it works we shouldn't ask too many questions - I just want to make sure we capture what we do know as code comments :)

A minor issue is that the test is missing a comma after 'group' => 'System'.

Nice, that looks a lot better to me.

My only remaining comment is that it would be a good idea to add a @see _registry_update() to the PHPDoc at the top of the function, as well as one additional sentence there - maybe something like:

"Since this function does not result in a fully-built registry (only the minimum needed for Drupal to bootstrap), it also sets a variable which will trigger a complete rebuild during the next full bootstrap after it runs."

That way anyone reading the PHPDoc gets the essential information about this function without having to read through the more detailed comments in the code.

I was going to RTBC this patch, but then played around with it for a while and came up with a number of questions:

1. I noticed that even just with Drupal core, the pseudo-rebuilt registry which results from this patch has fewer files in it than a normal rebuilt registry. I tracked this down to missing files from includes/database and includes/filetransfer. It seems that the RecursiveDirectoryIterator isn't actually recursing. I'm not sure if this matters in practice (I'm somewhat surprised the missing database stuff doesn't crash the whole site but I'm sure there's a good reason). Nonetheless, it's probably worth fixing. Other places in core use entertaining code like the following in this situation:
   

   new RecursiveIteratorIterator(new SkipDotsRecursiveDirectoryIterator($path), RecursiveIteratorIterator::SELF_FIRST)
   

   So maybe something similar is required here.

2. I think the variable_set('registry_rebuild', TRUE) line should go inside the database transaction, rather than after it? We probably want to ensure that no one acts on the rebuild request before the transaction is committed.
3. Once that variable is set, is there a potential for a race condition during the next bootstrap, if a bunch of different processes try to kick off a registry rebuild at once? Perhaps the bootstrap code needs to acquire a lock before proceeding with the rebuild?
4. The pseudo-rebuild is very destructive (since it blows away the whole registry and starts over). And it happens every time an administrator visits update.php, even if they aren't actually updating their site. If visitors hit the site in the middle of this rebuild (e.g. when the registry tables are mostly empty), will it whitescreen? I'm not sure how serious of a concern this is in practice. But I'm also not sure it's actually necessary to blow away the registry tables. If instead, the same logic as in _registry_update() were used, but this part were left out:
   

   // Add the hash for those files we have already parsed.
   if (isset($files[$filename])) {
     $files[$filename]['hash'] = $file['hash'];
   }
   

   wouldn't that also force each file to be rescanned, without having to destroy the tables beforehand?

   Additionally, perhaps the call to cache('bootstrap')->set('lookup_cache', array()) should happen at the end of the rebuild process initiated by update_rebuild_registry(), rather than the beginning. On a healthy site, that allows other requests to keep using the cache even while the major surgery is going on. That would seem to be more in line with how a normal rebuild works also, where the cache isn't rewritten until the end of the page request.

5. If I visit update.php after updating my filesystem, then it looks like the registry is going to be rebuilt based on outdated data from {system}, since system_rebuild_module_data() hasn't been called. I believe it's then possible for me to go through the entire update.php process with the "wrong" registry? The registry will eventually be rebuilt correctly at the end, but I wondered if there are scenarios where update functions can fail as a result of the registry being wrong. It would be great if as soon as you are redirected to "update.php?op=info" a real registry rebuild happened right then. #106 says it's not safe to rebuild the registry when MAINTENANCE_MODE is defined, but I don't think it's that simple because after actual updates are run, update.php flushes all caches which results in a registry rebuild anyway, and MAINTENANCE_MODE is defined then too. So maybe there actually is a way to run it earlier also?

FYI, @aspilicious's concern in #109 is a bit related to my last point above, and I was worried about it too. But then I realized that when your site is in maintenance mode, the MAINTENANCE_MODE variable is not actually defined. (Go figure.) So that part is actually OK; even sites in maintenance mode will still get the full registry rebuild once someone tries to hit the main site. The only concern is over update.php itself, as I described above.

Anyway, sorry for all the questions, but given this issue's history we probably need to be pretty careful with this patch.

i think some of what you point out raises again the question of whether we should have an explicit 'rebuild' idea, rather than just 'if you hit update.php, we rebuild'.

Have to say the same thought crossed my mind also. But from a user experience standpoint, it's so much better if this just works on its own. I would say that if all the issues I raised can be fixed (including #5) we are probably OK leaving it as an automatic step in update.php.

4. i think that will be alright, but we need to make sure that what is rebuilt isn't still hosed, so there's an attraction to blowing it all away. as for the WSODs, should be fine on systems that support transactions - no other mysql threads will see the truncated tables until the transaction is committed.

Right, I forgot to mention that the concern in #4 is only for databases that don't support transactions, which does lessen it a bit.

explicit 'rebuild' idea

Right on spot: #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding

In light of #1541674: Remove the registry is this still relevant in D8?

No, it won't be relevant anymore I think. Not sure if we want to wait with moving back to 7.x until the remove registry patch is actually commited.

If anyone thinks its best to postpone it, feel free, but I think this is fine.

I'd be so happy to move this back to Drupal 7, but system_rebuild_module_data() is still going to suffer from the same problem even when the registry is removed. That's less critical than the registry but it could still leave sites in an inconsistent state.

The registry is gone now, so we need to figure out what to do here.

system_rebuild_module_data() still needs all modules loaded because it fires hook_system_info_alter(), but I'm not aware of any fatal errors caused by this in either 7.x or 8.x so that's really more of a major bug. However the fix is going to be similar to one of the patches already posted here, so we should probably keep things together in one issue, and the 7.x patch will just need to ensure the registry is taken care of as well.

system_rebuild_module_data() is called in a controlled fashion though. It is only invoked from callers that operate in a DRUPAL_BOOTSTRAP_FULL environment, AFAICS. I don't see what would need to be fixed there.

That said, that's (vastly) less of a case for system_rebuild_theme_data(), which apparently depends on loaded modules + hook_system_info_alter(), too, but I'm already trying hard to fix that nightmare over in #1067408: Themes do not have an installation status

I'd recommend to move this issue to 7.x.

Major based on #127.

I wonder if we should just make hook_system_info_alter() a bootstrap hook.

This is major for 8.x now, but it's still critical for 7.x unfortunately.

Whether we can completely fix it or not for 7.x it'll need to be tracked as such (although I still think my earlier patches on this issue were fine for a 7.x fix and just need proper review).

I considered splitting this into 8.x and 7.x issues but the alter hook is still fired in the same place under the same conditions so that'd just mean less context and more issues at the moment.

With 8.x, we no longer write the info to the {system} table. So while some alters might affect the stuff that gets saved to config, we could at least just throw away the system info cache if we're not at full bootstrap.

Untested patch for that.

#130: system_info.patch queued for re-testing.

Rerolled for #1831702: CacheArray Implements / Overrides documentation fix.

Oops, patch.

#134: 534594-133.patch queued for re-testing.

Updated issue summary.

added link to patch to the issue summary.

As discussed above, this is no longer relevant to Drupal 8, but is still a problem for D7. Updated the issue summary, and setting back to D7.

No this is still an 8.x bug per #130.

Oops. Sorry!

added "Update 12 Aug 2013: The registry has been removed from Drupal 8 so this issue is no longer valid. Setting back to D7."

a weak tidy up

#134: 534594-133.patch queued for re-testing.

Tagging for reroll.

Oh, hang on. The patch does still apply :)

This likely shouldn't do anything in resolveCacheMiss(), but instead avoid writing to cache if !$this->persistable in destruct().

Hmm actually patch ought to work, sending for re-test.

#134: 534594-133.patch queued for re-testing.

#134 looks good to me, previous test failure was a bot with no space. Marking RTBC.

Let's see if we can write a test for this using an event that fires pre bootstrap finishing.

So the test would be have an event listener that tries to get system info

- ensure the cache entry is empty
- request a page.
- make sure it's still empty

To go further, could also check that the cache entry is valid when the event listener doesn't run.

+++ b/core/lib/Drupal/Core/Utility/ModuleInfo.php
@@ -22,11 +22,23 @@ class ModuleInfo extends CacheArray {
+  /**
+   * Overrides CacheArray::__construct().
+   */
+  function __construct($cid, $bin) {
+    parent::__construct($cid, $bin);
+  }

No need for this now

That's nicer.

Whoops, still no tests...

-

CNW for tests.

All bootstrap hooks have gone. And there are either few or none pre-module events (see https://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/d...)

So... I think this is a 7.x issue only now, moving there.

This looks like it will work...

Nope.

Didn't really review logic but functinoality: There is a } out of place in your class. Also, the $data in resolveCacheMiss doesn't resolve to anything.

I was looking for the out of place } but still haven't found it. Where is it.

Also not sure where that $data should come from. Mostly I was trying to just backport what others had done.

+++ b/includes/bootstrap.inc
@@ -2972,17 +2972,35 @@ class SchemaCache extends DrupalCacheArray {
     $this->storage[$offset] = $value;
-    $this->persist($offset);
+    if ($this->persistable()) {
+      $this->storage[$offset] = $data;
+      $this->persist($offset);
+    }

This should be:

+++ b/includes/bootstrap.inc
@@ -2972,17 +2972,35 @@ class SchemaCache extends DrupalCacheArray {
-    $this->storage[$offset] = $value;
-    $this->persist($offset);
+    if ($this->persistable()) {
+      $this->storage[$offset] = $value;
+      $this->persist($offset);
+    }

+++ b/includes/bootstrap.inc
@@ -2972,17 +2972,35 @@ class SchemaCache extends DrupalCacheArray {
+  protected function persistable() {
+    if (is_null($this->persistable)) {
+      $this->persistable = drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL;

While it is great to cache this, I think this function should take a parameter so that the drupal_bootstrap_full can actually set that the schema cache now can be persisted.

Also shouldn't this fully go to DrupalCacheArray directly so that all cache arrays only persist if they are used within DRUPAL_BOOTSTRAP_FULL?

I've re-rolled the patch based on your first comment.

The second though seems like your suggesting to go further than we did in D8, right?

In which case that should be a new D8 issue that we could bring back.

+++ b/includes/bootstrap.inc
@@ -2972,17 +2972,35 @@ class SchemaCache extends DrupalCacheArray {
     $this->storage[$offset] = $value;
-    $this->persist($offset);

The first line will also need to be removed.

What first line? protected $persistable; or $this->persist($offset);

@Fabianx more clarification please.

$this->persist($offset); needs to be removed else you do set it then set it again in the if, but you want to only do it in the if case not in the other case.

Ok, thanks for the clarification.

This might not fail so badly.

@mgifford that patch didn't pass Mike - maybe needs a reroll by now too.

The code applies too, but now getting this error:

( ! ) Fatal error: Call to undefined method SchemaCache::persistable() in /DRUPAL7/includes/bootstrap.inc on line 2993

It definitely needs to be rerolled..

I am splitting of the latest patch into a new sub-issue:

#2450533: SchemaCache is prone to persistent cache corruption / drupal_get_complete_schema() is not robust against recursion

Because this one is for more services in Drupal 7, than just the schema cache ...

Making this one a meta.

Got here from #3028130: Class [warning] RulesPluginFeaturesIntegrationInterface does not exist and could not be loaded faces.inc:233.

Updating to rules-7.x-2.12 just caused several sites to crash.

It's understandable that there are technical reasons why rebuilding the registry cannot be done as part of the update process.

But isn't there some way that module maintainers can warn site maintainers that updating a certain module will require a registry rebuild, failing which their sites will crash?

(disregard, bumping to fix issue indexing)