Backport of #147000: Unify and rewrite module_rebuild_cache() and system_theme_data()
pillarsdotnet - September 12, 2008 - 22:44
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
Description
Backport of patch in http://drupal.org/node/147000#comment-999711 to Drupal 6.4
Patch based on Drupal 6.4 official release.
Untested as of yet...
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| 147000_backport-to-6.4.diff | 18.45 KB | Ignored | None | None |
#1
Patch failed to apply. More information can be found at http://testing.drupal.org/node/14307. If you need help with creating patches please look at http://drupal.org/patch/create
#2
Okay...
Hopefully testbot will like this version?
#3
I am not sure that this could get in, because only bug fixes are accepted for D6..
#4
Re: Pasqualle "only bug fixes are accepted"
There are race conditions caused by two or more webserver processes calling the named functions.
The race condition caused errors in my installation; my bug report got marked "wontfix".
This patch should greatly reduce (but not eliminate) the chance of such a race condition.
it is my understanding that if the D7 patch lands then the D6 backport has a good chance of being accepted also.
Should a backport of a performance enhancement which reduces but does not completely eliminate a known bug condition be marked as a feature request or a bug report?
#5
Checked out cvs branch DRUPAL-6 (currently 6.5-dev), applied the patch, and tested. Found several errors and fixed them.
Corrected patch created via "cvs diff -u -p" attached
#6
More errors fixed; re-rolled. Seems to be working well, now.
#7
Re-rolled against current CVS checkout. To apply:
cvs -z6 -d:pserver:anonymous:anonymous@cvs.drupal.org:/cvs/drupal co -r DRUPAL-6 drupalwget -O - http://drupal.org/files/issues/147000_backport-to-6.x.diff | patch -p0 -d drupal
#8
On a code style review, many comment lines should end with a period.
Also,
+ // Keep the old filename from the database incase the file has moved"incase" -> "in case"
#9
Oh, give me a break! You're marking this "code needs work" based on the fact that I didn't correct grammatical errors in the comments???
Go bother the original developers; I'm just maintaining a backport.
#10
Comments that I added have been revised in 147000#comment-1050505. Comments already in core, that's a quite different story...
#11
Incorporated Dave Reid's six lines of comment correction.
Re-rolled patch; apply as before.
#12
Re-rolled patch aganst current CVS checkout; should also apply against 6.8.
#13
Couple of no-no's here:
<?php
+ db_query('UPDATE {system} SET ' . implode(',',$sql_sets) . ' WHERE filename=\'%s\'',$sql_args);
+ }
- db_query("INSERT INTO {system} (name, owner, info, type, filename, status, throttle, bootstrap) VALUES ('%s', '%s', '%s', '%s', '%s', %d, %d, %d)", $theme->name, $theme->owner, serialize($theme->info), 'theme', $theme->filename, isset($theme->status) ? $theme->status : 0, 0, 0);
+ // Remove the file so that we don't try to insert it later.
+ unset($files[$file->name]);
+ }
+ else {
+ // File is not found in filesystem, so delete record from the system table.
+ db_query('DELETE FROM {system} WHERE filename=\'' . $file->filename .'\'');
?>
please re-write this to be
<?php
+ db_query("UPDATE {system} SET ". implode(', ', $sql_sets) ." WHERE filename='%s'",$sql_args);
+ }
- db_query("INSERT INTO {system} (name, owner, info, type, filename, status, throttle, bootstrap) VALUES ('%s', '%s', '%s', '%s', '%s', %d, %d, %d)", $theme->name, $theme->owner, serialize($theme->info), 'theme', $theme->filename, isset($theme->status) ? $theme->status : 0, 0, 0);
+ // Remove the file so that we don't try to insert it later.
+ unset($files[$file->name]);
+ }
+ else {
+ // File is not found in filesystem, so delete record from the system table.
+ db_query("DELETE FROM {system} WHERE filename = '%s'", $file->filename);
?>
also please replace all tabs with 2 spaces.
#14
Sorry, but this is a duplicate of #147000. Don't open duplicate issues for different versions. Let's get#147000 in, and then think whether we should backport it or not.
#15
Anybody running 7.x has bigger problems than the ones cured by this patch. If-and-when #147000 gets integrated, any 6.x users who ask for a backport will no doubt be told to upgrade to 8.x instead. I'm okay with this, but it doesn't mean that I'm going to stop running, fixing, and sharing code.
I'm sorry if that offends you.
#16
You would use your time much better if you helped the original patch get in. We never open two issues for the same bug/improvement in two versions of Drupal. Please play with the team, don't try to go solo.
#17
Applied trivial changes to comments and formatting and re-rolled for today's checkout of DRUPAL-6 branch.
#18
does this patch still need review? or it's been committed to more recent versions of drupal 6?
#19
What's the status on this?
#20
I'm getting those errors once in a while. Will the patch here remove those? It was from last year, so I guess that was dropped? Do you need people to check it?
user warning: query: INSERT INTO system (name, owner, info, type, filename, status, throttle, bootstrap) VALUES ('pushbutton', 'themes/engines/phptemplate/phptemplate.engine', 'a:13:{s:4:"name";s:10:"Pushbutton";s:11:"description";s:52:"Tabled, multi-column theme in blue and orange tones.";s:7:"version";s:4:"6.13";s:4:"core";s:3:"6.x";s:6:"engine";s:11:"phptemplate";s:7:"project";s:6:"drupal";s:9:"datestamp";s:10:"1246481719";s:7:"regions";a:5:{s:4:"left";s:12:"Left sidebar";s:5:"right";s:13:"Right sidebar";s:7:"content";s:7:"Content";s:6:"header";s:6:"Header";s:6:"footer";s:6:"Footer";}s:8:"features";a:10:{i:0;s:20:"comment_user_picture";i:1;s:7:"favicon";i:2;s:7:"mission";i:3;s:4:"logo";i:4;s:4:"name";i:5;s:17:"node_user_picture";i:6;s:6:"search";i:7;s:6:"slogan";i:8;s:13:"primary_links";i:9;s:15:"secondary_links";}s:11:"stylesheets";a:1:{s:3:"all";a:1:{s:9:"style.css";s:27:"themes/pushbutton/style.css";}}s:7:"scripts";a:1:{s:9:"script.js";s:27:"themes/pushbutton/script.js";}s:10:"screenshot";s:32:"themes/pushbutton/screenshot.png";s:3:"php";s:5:"4.3.5";}', 'theme', 'themes/pushbutton/pushbutton.info', 0, 0, 0) in .../drupal/modules/system/system.module on line 821.
Thank you.
Alexis Wilke
#21
subscribing
#22
subscribing and bumping. Tempted to set as critical, as is happening on several websites..
#23
I'm getting these intermittently on nearly every one of my sites. A client just saw it today while my boss was training them - not good. #147000: Regression: Unify and rewrite module_rebuild_cache() and system_theme_data() has already been fixed so this is no a valid backport.
#24
I'm getting this error also - what's the status here? Has the fix been backported to D6? I'm on 6.13 - what shall I do? Thanks much!
#25
Patch no longer applies. This leads to the loss of active themes on cron runs and there are a string of warnings about duplicate entries in the theme section of the system table.
#26
In my (very brief) testing this seems to solve the problem reported with cache rebuilding in #477550: "User warning: Duplicate entry ... " ... Themes and elsewhere. This is a re-roll of the patch in #17 to apply to 6.13.
#27
I get the following error message:
patching file install.phpcan't find file to patch at input line 18
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: includes/theme.maintenance.inc
|===================================================================
|--- includes/theme.maintenance.inc (revision 96)
|+++ includes/theme.maintenance.inc (working copy)
--------------------------
File to patch:
Any ideas? Thanks!
#28
bcobin, to patch the file, you'll need to type 'patch -p0 < 307756-26-rebuild-cache-unification-backport.patch' inside the root drupal directory. For this example, the patch is also downloaded to the root drupal directory.
#29
Thank you, japerry - I was able to successfully apply the patch both on my development sandbox and on the production site. Woo hoo!
One note: On the production site, the patch changed permissions on update.php so it was not accessible, nor was I able to change permissions from FTP (I wasn't sure what the permissions should be anyway, though I'm sure I could have done it from command line.)
So I renamed update.php to update_old.php, copied update.php from the development install, and was able to run it, no prob.
I'll keep an eye out, but I am hopeful this squashes the nasty, nasty "theme error" bug! Thank you VERY much!
(The Drupal community ROCKS!)
#30
Successfully applied to latest D6 release and so far it's looking good.
#31
Several positive reports and no negative reports yet. Advancing to RTBC.
#32
We cannot change APIs in the Drupal 6 version. The patch would break any modules that use this data, and there are some modules that do.
#33
Dave,
Which modules? What API is broken? It seems to me that all the old functions are kept. So far, I have not seen even on of the 100+ modules that I use having any kind of problem since I applied the patch!
So please, give us substance, who's the victim?! Without proof of otherwise it works!
Thank you.
Alexis Wilke
#34
Changing function names = changing API. The only times API can change are in extreme cases (absolutely necessary for security) and during major Drupal versions. For example, this patch changes module_rebuild_cache() to system_get_module_data(). Here's an example of just the DRUPAL-6--1 CVS modules that call that function:
grep -Rni 'module_rebuild_cache' ../module_supports/module_supports.module:14: $files = module_rebuild_cache();
./smartlist_manager/smartlist_manager.install:139: module_rebuild_cache();
./patterns/components/system.inc:172: $modules_info = module_rebuild_cache();
./patterns/components/system.inc:267: $modules_info = module_rebuild_cache();
./patterns/patterns.module:845: $modules_info = module_rebuild_cache();
./patterns/patterns.module:1351: module_rebuild_cache();
./patterns/patterns.module:2600: $modules_info = module_rebuild_cache();
./node_import/node_import.admin.inc:1246: foreach (module_rebuild_cache() as $name => $module) {
./authcache/authcache.module:124: module_rebuild_cache();
./simpletest_automator/simpletest_automator.module:405: module_enable(array_keys(module_rebuild_cache()));
./xmlsitemap/xmlsitemap.install:414: module_rebuild_cache();
./xmlsitemap/xmlsitemap.install:493: module_rebuild_cache();
./mailman_manager/mailman_manager.install:116: module_rebuild_cache();
./util/system_module.module:17: $modules = module_rebuild_cache();
./testing_server_setup/testing_server_install/patches/core.patch:32: $files = module_rebuild_cache();
./module_filter/module_filter.module:98: $files = module_rebuild_cache();
./module_filter/module_filter.module:101: foreach (module_rebuild_cache() as $id => $module) {
./configuration/configuration.module:2026: module_rebuild_cache();
./configuration/configuration.module:2060: $modules_info = module_rebuild_cache();
./drush_mm/drush_mm.inc:114: module_rebuild_cache();
./drupal_notifier/drupal_notifier.module:297: $files = module_rebuild_cache();
./fivestar/fivestar.install:148: module_rebuild_cache();
./fivestar/fivestar.install:152: module_rebuild_cache();
./moduleinfo/moduleinfo.module:34: $files = module_rebuild_cache();
./sitedoc/sitedoc.module:1144: $files = module_rebuild_cache();
./luceneapi/luceneapi.admin.inc:30: $files = module_rebuild_cache();
./module_browser/module_browser.module:68: return module_rebuild_cache();
./features/features.admin.inc:12: module_rebuild_cache();
./plugin_manager/plugin_manager.module:164: $files = module_rebuild_cache();
./plugin_manager/plugin_manager.admin.inc:385: module_rebuild_cache();
./plugin_manager/plugin_manager.admin.inc:424: $files = module_rebuild_cache();
./services/services/system_service/system_service.inc:67: $modules = module_rebuild_cache();
./provision/platform/install.php:243: $files = module_rebuild_cache();
./provision/platform/provision_drupal.module:270: $data['modules'] = _provision_drupal_get_cvs_versions(module_rebuild_cache());
./provision/platform/provision_drupal_clear.php:23:module_rebuild_cache();
./dtools/wsod/wsod.module:111: module_rebuild_cache(); // rebuild paths in system table
./dtools/wsod/wsod.module:137: module_rebuild_cache();
./dtools/wsod/wsod.module:285: module_rebuild_cache();
./dtools/wsod/wsod.module:294: module_rebuild_cache();
./dtools/wsod/README.txt:31: - run in emergency mode to execute module_rebuild_cache() to refresh module paths
./dtools/wsod/wsod_emergency.php:32: module_rebuild_cache(); // so we need to rebuild path of modules
./webservices/wsservice_dcore/wsservice_dcore.system.inc:58: $modules = module_rebuild_cache();
./simpletest/drupal_unit_tests.php:140: module_rebuild_cache(); // Rebuild the module cache
./simpletest/drupal_test_case.php:290: module_rebuild_cache();
./simpletest/drupal_test_case.php:316: module_rebuild_cache();
./simpletest/drupal_test_case.php:453: module_rebuild_cache();
./date/date_api.install:123: module_rebuild_cache();
#35
Well... the module_rebuild_cache() function is still available. Now why he renamed them, maybe because in D7 it was renamed? And he used those from D7? But it is still D6 compatible because the old functions are still there. So, yes, we could spend time copying the function system_get_module_data() inside module_rebuild_cache() to make you happy, but I don't really see the point in wasting more time?! Another one that will break your code for real?
<?phpfunction module_rebuild_cache() {
- // Get current list of modules
- $files = drupal_system_listing('\.module$', 'modules', 'name', 0);
+ return system_get_module_data();
+}
?>
#36
subscribing
#37
Seems like #35 should satisfy the issue. Any reason we can't just add that to the patch and commit it?
#38
After consulting with folks this needs to be re-rolled using the existing functions where possible.
#39
Has any of this been implemented in 6.14? To wit; does 6.14 need this patch and, if so, will it apply correctly? Thanks...
#40
The last patch above should apply to Drupal 6.14. However it will not be committed as is as it should be re-factored to use the existing functions instead of introducing new functions. For it to be included in Drupal 6.15 this needs to be done.
#41
I've tried to apply the patch on drupal 6.14 and the patch does not apply to the following files :
Could we have a patch which work with Drupal 6.14 please ?
#42
Just wanted to confirm I just received errors on my site that lead me to searching and finding this issue. Would really appreciate this being committed.
#43
To clarify there is nothing here to commit. There is a patch that needs work. Updates to the patch are welcome so it can get reviewed and be considered for being committed.
The attached patch resolves the API issue and seems to resolve the issue with themes being disabled on cron. It does not, however, fix the issue of themes being disabled on update.php.
#44
Very sorry, but I've become confused after going through this thread, so I have a stupid question if you don't mind...
Is this latest patch, #43, intended to address (among other details) the "duplicate entry..." errors people are getting re: themes (enabled and disabled), as described in a number of places including here [#367966]?
If so, is the patch intended to be applied to Drupal 6.14 or the latest dev release (or...)?
(@jbrauer #43) Are you saying applying this patch will or may result in themes being disabled when update.php is run? (If so, can we simply re-enable the theme without additional problems? Or... is that part of what needs to be tested? or...?)
I'm completely on board with helping test this or other solutions, just want to make sure I understand what's up.
Thanks so much for your patience.
#45
It has been effective for this.
It is built against 6.14 but should apply to head as well.
This is correct. This is also why the patch is "needs work" because we cannot say the problem is 'fixed' but users after updating will have to go re-enable the themes. As a practical matter in the interim this will work but it needs to be a complete solution before it gets committed.
#46
@45 -- Thank you very much, your responses are perfect!
I will be trying the patch out later today most likely and will certainly comment again if anything unexpected happens -- and I'll be stopping by again when new comments are posted in case there's anything I can do to help with this. Thanks again, everyone!
#47
subscribe. I'm getting what I expect are the same errors also noted in #477550: "User warning: Duplicate entry ... " ... Themes, after upgrading to 6.14.
user warning: Duplicate entry 'sites/all/themes/contrib_themes/amity_island/amity_island.info' for key 1 query: INSERT INTO system (name, ...#48
As a side note, because it is probably not directly related to this issue although the result was quite similar, the other day I moved a site from one server to another and somehow new themes were added (I use multi-site installs and there are more themes on the new server) and those addition did NOT include the proper type (i.e. the Type column was NULL!)
I simply ran an SQL order to fix the column, but your code would still fail because the DELETE expects the column to be proper (I would think it should be too!) and thus the INSERT would attempt to duplicate the path which is what the error is about, generally.
UPDATE system SET type = 'theme' WHERE type IS NULL;
IMPORTANT: I checked my table and could tell only themes had that problem, modules were properly defined.
Thank you.
Alexis Wilke
#49
Subscribing
#445052: Race Condition in system_theme_data() function. Duplicate entry error for every theme.
#50
Subscribing. I'm getting the Duplicate entry error as well.
#51
subscribing
#52
Subscribing. It's been over a year for this "critical" bug (according to the current priority setting), I'm really looking forward to seeing this fixed!
-Josh
#53
subscribing...
#54
subscribing....when clients see this they get really hinky. will apply 6.14 patch today/tomorrow and try to test, but this happens routinely on the three sites I'm building at the moment. #45 was very helpful clarification.
#55
I have the same issue, BUT I don't have cron task configured (as I'm still developing and I don't need cron to run automatically for now), and also never happened before when cron runs or update.php, it just happens. Was more notorious when content editing, but this also happens viewing content, or doing everything else, suddenly, all my themes are disabled and every time, all the site gets in a particular theme, the custom one, so at least visitors will always see custom theme while this bug gets fixed.
About time, doesn't happen in regular basis.
I checked #48 tip, but all my system table records are properly declared.
I discovered this bug days ago, but since thursday hadn't happen, but today, it happened again.
I'll try to apply patch when I find time to do so.
My setup:
Drupal 6.14 over WAMP server 2.0 (apache 2.2.8, php 5.2.6, mysql 5.0.5b) in dev server, but this happens too in future production server (LAMP I don't have versions at this time)
Modules installed:
ad, ad_flash, ajax, background, backup_migrate, better_formats, cck, cck_fieldgroup_tabs, chart, customized (Nothing fancy on this one), date, devel, email, emfield, filefield,
imageapi, imagecache, imagefield, jquery_ui, jquery_update, link, pathauto, smtp, spamspan, swftools, tabs, taxonomy_menu, token, transliteration, views, webform, xmlsitemap
Themes enabled:
Garland (for admin users) and custom theme (for visitor users)
#56
I applied the patch at #45 to three sites, and the problem seemed to go away. However, just yesterday the client reported seeing it on one of the patched sites. I confirmed via the log that it had happened.
So the patch apparently doesn't solve the problem, just (possibly) reduces the frequency of occurrence.