in working on http://drupal.org/node/146910, i noticed that module_rebuild_cache() and system_theme_data() do almost exactly the same thing, but have wildly different names. we should clean this up. a couple of options:

  1. $modules = system_data('modules');
    $themes = system_data('themes');
    
  2. $modules = system_module_data();
    $themes = system_theme_data();
    
CommentFileSizeAuthor
#173 147000-system-duplicate-key-error-theme-data-173.patch1.17 KBELC
#166 protect_theme_rebuild-147000-166.patch8.11 KBjbrauer
#164 protect_theme_rebuild-147000-164.patch6.65 KBjbrauer
#123 147000-106-nocvstag.patch8.72 KBTR
#119 147000-106-nocvstag-D6.patch8.72 KBTR
#106 147000-106-D6.patch8.73 KBpillarsdotnet
#105 147000-105-D6.patch7.15 KBpillarsdotnet
#98 147000-protect-rebuild-98-D6.patch8.09 KBpillarsdotnet
#95 147000-protect-rebuild-95-D6.patch8.09 KBpillarsdotnet
#95 interdiff-D6.patch723 bytespillarsdotnet
#81 147000-protect-rebuild-81-D6.patch8.96 KBpwolanin
#79 147000-protect-rebuild-79-D6.patch6.7 KBdstol
#75 147000-protect-rebuild-74-D6.patch8.97 KBpwolanin
#66 147000-protect-rebuild-66-D6.patch6.24 KBpwolanin
#65 drupal-307756-133-D6.patch5.86 KBpwolanin
#52 system-get-data-147000-D7_another_fix2.patch638 bytesBerdir
#49 system-get-data-147000-D7_another_fix.patch959 bytesBerdir
#44 system-get-data-147000-D7_fix.patch652 bytesBerdir
#35 system-get-data-147000-D7_5.patch21.92 KBBerdir
#33 system-get-data-147000-D7_3.patch23.14 KBBerdir
#32 system-get-data-147000-D7_3.patch23.11 KBBerdir
#30 system-get-data-147000-D7_2.patch23.15 KBBerdir
#28 system-get-data-147000-D7_1.patch21.9 KBBerdir
#19 system-get-data-147000-19-D7.patch22.39 KBDave Reid
#18 system-get-data-147000-D7.patch22.05 KBDave Reid
#16 system_get_data-147000-D7.patch22.19 KBDave Reid
#15 147000.module_system.patch25.19 KBDave Reid
#10 147000-2.patch25.18 KBDave Reid
#9 147000.patch25.12 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

This cleanup/unification would also be a good place to add the hook_alter_info() I'm proposing over at http://drupal.org/node/152926.

drewish’s picture

subscribing. personally i like #1 better.

dww’s picture

maybe we should call it drupal_system_data() to defend ourselves from system.module needing to implement hook_data() at some point in the future?

dww’s picture

or maybe "system_get_data($type)"? too vague? maybe my original plan for "system_theme_data()" and "system_module_data()" (which would obviously share lots of the same helper functions) is the way to go? i'm too exhausted now to think clearly.... someone please tell me the right name(s) to use. ;)

drewish’s picture

digging to the two functions i'm having a bit of trouble figuring out how to unify them into a single system_data() function. i think shared helper functions might be a better way to go.

dww’s picture

yeah, i agree. sad to say it, but i think s/module_rebuild_cache/system_module_data/ is about the best we can do here. there are a few things that could be shared, but not many. :( it's possible we could reorganize where all the code lives, etc, but that doesn't seem particularly urgent. i think i'm just going to put this on hold and worry about http://drupal.org/node/152926 first.

Dave Reid’s picture

Version: 6.x-dev » 7.x-dev
Assigned: Unassigned » Dave Reid

I'm going to take this up as I'm also working on rewriting system_theme_data in #289369: Deleted modules are not removed from system table.

webchick’s picture

Small request: Let's get a verb in that function name ({scope}_{verb}_{thing_that's_being_verbed}()). I don't like either 1. or 2. for that reason, although I agree that unifying their names is a good move.

Dave Reid’s picture

Status: Active » Needs review
FileSize
25.12 KB

Here's my first effort on this. Summary of changes:
- module_rebuild_cache() has been moved from ./includes/module.inc to ./system/system.module for consistency
- module_rebuild_cache() has been renamed to system_get_module_data()
- system_theme_data has been renamed to system_get_theme_data()
- Created helper function _system_get_module_data
- Renamed helper function _system_get_theme_data
- Refactored system_get_files_database(&$files, $type) to system_get_files_database(&$files, $type, $update = FALSE)
- When $update = TRUE, the function will update the system table based on the information provided in the $files database (insert new records for new discovered modules/themes in filesystem, updates changed records, deletes records for modules/themes no longer found in the filesystem)

I tested install, update, module enabling/disabling/installing/deleting, theme switching, basically anything involving these functions. Tests all pass for me. The only thing I'm unsure of is if I should split the update functionality out of system_get_files_database() and make a new function called system_update_files_database().

Dave Reid’s picture

Title: unify module_rebuild_cache() and system_theme_data() » Unify and rewrite module_rebuild_cache() and system_theme_data()
FileSize
25.18 KB

Here's my second revision. I split my original system_get_files_database into two functions. The original system_get_files_database and system_update_files_database. I updated the logic so only changes found in module or theme information will be actually updated into the database instead of blindly updating every single record every time. After testing compared to the original methods, the amount of insert/update/delete queries to the database were reduced to barely nothing with no side effects or issues:

Page: admin/build/modules
Task: viewing page, no changes being made
Original D7 code: 136 write queries
Patched D7 code: 0 write queries

Page: admin/build/modules
Task: after uninstalling and removing devel module from file system
Original: 121 write queries
Patched: 5 write queries

Page: admin/build/modules
Task: after installing devel module to file system
Original: 136 write queries
Patched: 5 write queries

Page: admin/build/themes
Task: viewing page, no changes being made
Original: 57 write queries
Patched: 0 write queries

I'll try to gather some data on read queries.

dww’s picture

Very cool, thanks. #10 sounds especially promising, though I haven't had a chance to look at the patch. Keep up the great work!

arhak’s picture

subscribing

Dave Reid’s picture

Making a note to myself to keep an eye out for when #259412: Total module system revamp lands in case I need to re-roll patch.

pillarsdotnet’s picture

Backported to version 6.4 here: http://drupal.org/node/307756

Dave Reid’s picture

FileSize
25.19 KB

Rerolled and bumped for review.

Dave Reid’s picture

Status: Needs review » Needs work
FileSize
22.19 KB

Re-rolled for HEAD, but the patch is causing a errors on install, so I need to take a look at what's going wrong.

pillarsdotnet’s picture

Can't test for D7, but my backport to D6 also caused errors, which I fixed. Maybe you can compare with my (working) D6 patch at http://drupal.org/node/307756

Dave Reid’s picture

Fixed some comment coding standards, will need to test if this still applies. Too tired right now...

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
22.39 KB

Re-rolled for head. I also did some actual ab performance testing. I like the results, and I bet you will too!

admin/build/modules before patch:
Requests per second:    0.96 [#/sec] (mean)
Time per request:       1042.654 [ms] (mean)
Time per request:       1042.654 [ms] (mean, across all concurrent requests)
Transfer rate:          34.40 [Kbytes/sec] received

admin/build/modules after patch:
Requests per second:    1.01 [#/sec] (mean)
Time per request:       988.747 [ms] (mean)
Time per request:       988.747 [ms] (mean, across all concurrent requests)
Transfer rate:          36.27 [Kbytes/sec] received

admin/build/themes before patch:
Requests per second:    3.48 [#/sec] (mean)
Time per request:       287.024 [ms] (mean)
Time per request:       287.024 [ms] (mean, across all concurrent requests)
Transfer rate:          71.66 [Kbytes/sec] received

admin/build/themes after patch:
Requests per second:    3.82 [#/sec] (mean)
Time per request:       261.681 [ms] (mean)
Time per request:       261.681 [ms] (mean, across all concurrent requests)
Transfer rate:          78.60 [Kbytes/sec] received

With only a bit more processing, we're getting a huge decrease in the hits to the database (see # 10) and a very useful performance improvement!

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

subscribe. i got to looking at module_rebuild_cache when wondering how to make tests run faster.

its a beast, and we call it too often.

Damien Tournoud’s picture

The patch looks good by itself.

A couple of issues:

+  // Need to make a safe, modifiable copy of the $files array since PHP
+  // automatically makes references to objects instead of copies.
+  $files = array();
+  foreach($files_updated as $key => $file) {
+    $files[$key] = clone $file;
+    $files[$key]->info = serialize($files[$key]->info);
+  }

I don't buy it. If the only difference is serialize() the info key or not... we can easily do better then doubling memory usage.

+      // Scan fields to find only the updated values.
+      $file->updated_fields = array();
+      foreach ($file as $key => $value) {
+        if (isset($files[$file->name]->$key) && $files[$file->name]->$key != $value) {
+          $file->updated_fields[$key] = $files[$file->name]->$key;
+        }
+      }
+
+      // Update the record.
+      if (count($file->updated_fields)) {
+        db_update('system')
+          ->fields($file->updated_fields)
+          ->condition('filename', $file->old_filename)
+          ->execute();
+      }

It would probably be faster to just look if *at least one* field has changed, pass the whole $file to db_update, and let the database deal with it.

+      // File is not found in filesystem, so delete record from the system table.
+      db_delete('system')
+        ->condition('filename', $file->filename)
+        ->execute();
+    }

Two words: file system.

+  // All remaining files are not in the system table, so we need to add them.
+  foreach($files as $file) {

A space is missing after foreach.

+    db_insert('system')
+      ->fields(array(
+        'filename' => $file->filename,
+        'name' => $file->name,
+        'type' => $type,
+        'owner' => isset($file->owner) ? $file->owner : '',
+        'bootstrap' => isset($file->bootstrap) ? $file->bootstrap : 0,
+        'info' => $file->info)
+      )
+      ->execute();

This is badly formatted.

+  // Set defaults for module info

Dot missing at the end of the sentence.

+  // Read info files for each theme
+  foreach ($modules as $key => $module) {

I guess that's "Read info files for each module". Plus the final dot is missing.

+    // Log the critical hooks implemented by this module.
+    $modules[$key]->bootstrap = 0;
+    foreach (bootstrap_hooks() as $hook) {
+      if (module_hook($modules[$key]->name, $hook)) {
+        $modules[$key]->bootstrap = 1;
+        break;
+      }
     }

Do we still need this? I thought we removed the notion of bootstrap module.

+    // Set defaults for theme info

Same remark as above.

Dave Reid’s picture

Awesome review DamZ! I think this was one of my first core patches (seeing as the bootstrap stuff is still there), and it obviously showed. :) I'll have to revisit this to get it back in tip-top shape.

Dave Reid’s picture

Dave Reid’s picture

Issue tags: +Performance, +Patch Spotlight
mathieu’s picture

Subscribe.

EvanDonovan’s picture

Anyone working on this? Looks like it would be a critical performance improvement...

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.9 KB

Re-roll... that was.. erm... fun... NOT ;)

About DamZ's review..

I don't buy it. If the only difference is serialize() the info key or not... we can easily do better then doubling memory usage.

Changed that function quite a bit, removed whole additional $files array, beneath others things. There is some special casing now for the info attribute, but it's quite simple and the whole function is shorter.

It would probably be faster to just look if *at least one* field has changed, pass the whole $file to db_update, and let the database deal with it.

Not sure about that. how would we do that? a break after the first mis-match?

Changed the other things...

One thing that I don't like is filepath vs. filename. We use filename in {system} but filepath everywhere else. Imho, that's quite confusing. and filepath is the proper name, because the path is included too. We should rename the column in {system} imho..

Dries’s picture

From an API point of view, this looks like a nice clean-up.

- @param $files_updated should be $files.

- The following code looks a little funny:

+  system_get_files_database($themes, 'theme');
+  system_update_files_database($themes, 'theme');

The same pattern exist for modules. It's funny because you seem to be reading and writing the same data -- that feels like a null operation.

Berdir’s picture

- Changed $files_updated to $files

The same pattern exist for modules. It's funny because you seem to be reading and writing the same data -- that feels like a null operation.

Yes it does, but it does make sense.. that is the workflow (same for themes):
1. Read the file system and looks what's there (_system_get_module_data())
2. Extend that information with things that are only in the database, for example, status and schema_version (drupal_get_files_database)
3. Update/Insert/Delete {system} based on the new information we have. (drupal_update_files_database())

Maybe we could combine step #2 and #3, but I tried and failed (resulted in the ~10th drupal re-install for that patch ;)) or rename step #2 to extend instead of get or something like that.

- Introduced a static cache for system_get_module_data(). It is currently called twice, and we did remove a call from the registry (#429132: Remove unecessary module_rebuild_cache() call from _registry_rebuild()). If we have a static cache, we can revert that change, because using the statically cached information is obviously faster. It does need a bit more memory, but we save that atleast on admin/build/modules because we don't have to load and parse all those .info files three times. The static cache can currently not be reset except of using drupal_static_reset() directly..

Some devel "benchmarks"

Without patch:

Executed 919 queries in 152.26 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 990.35 ms.
Executed 919 queries in 170.06 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 1116.68 ms.
Executed 920 queries in 164.28 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 1034.99 ms.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.79 MB, PHP peak usage=27 MB.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.79 MB, PHP peak usage=27 MB.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.79 MB, PHP peak usage=27 MB.

Patch without static cache:

Executed 796 queries in 134.05 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 946.37 ms.
Executed 798 queries in 135.79 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 931.35 ms.
Executed 798 queries in 137.66 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 919.3 ms.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.17 MB, PHP peak usage=25.5 MB.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.14 MB, PHP peak usage=25.5 MB.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.17 MB, PHP peak usage=25.75 MB.

Path with static cache and #429132: Remove unecessary module_rebuild_cache() call from _registry_rebuild() reverted:

Executed 796 queries in 137.8 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 880.64 ms.
Executed 795 queries in 136.18 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 855.15 ms.
Executed 795 queries in 138.14 milliseconds. Queries taking longer than 5 ms and queries executed more than once, are highlighted. Page execution time was 875.1 ms.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.56 MB, PHP peak usage=24.75 MB.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.56 MB, PHP peak usage=24.5 MB.
Memory used at: devel_boot()=0.78 MB, devel_shutdown()=15.56 MB, PHP peak usage=24.5 MB.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
23.11 KB

Forgot to remove a debug call...

Berdir’s picture

Bad update.module fooled my :)

Berdir’s picture

Status: Needs review » Needs work

Passed: 149 passes..

That's not exactly enough ;) I assume the tests don't like the static cache..

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.92 KB

I don't think I can get the static cache working without harming kittens, removed those things again. Maybe we can thing about this again after #481498: Update simpletest module to use drupal_static()

Dries’s picture

Status: Needs review » Fixed

Looks good. Committed to CVS HEAD. I'm marking this 'code needs work' because we'll want to update the upgrade documentation. Please mark as fixed once the handbook is updated.

Dave Reid’s picture

Thanks Berdir for picking this up where I left off. It was much needed.

webchick’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation

This was not actually marked "needs work" :)

cburschka’s picture

-  // Set defaults for module info
-  $defaults = array(
-    'dependencies' => array(),
-    'dependents' => array(),
-    'description' => '',
-    'package' => 'Other',
-    'version' => NULL,
-    'php' => DRUPAL_MINIMUM_PHP,
-    'files' => array(),
-  );
+  // Set defaults for module info.
+  $defaults = array(
+    'dependencies' => array(),
+    'dependents' => array(),
+    'version' => NULL,
     'php' => DRUPAL_MINIMUM_PHP,
   );
Notice: Undefined index: package in system_modules() (line 648 of modules/system/system.admin.inc).

I'd guessed that this is a design change, but when I mentioned it on IRC Heine said it might have been an accidental regression.

Is it intended that modules must now set a package in their .info files, and the 'Other' package no longer exists?

Berdir’s picture

I'd say that's a bug. For themes, there were 2 defaults missing too, but these caused errors and I corrected them.

I will provide a fix tomorrow.

yched’s picture

Also, the committed patch contains

-    $themes = _system_theme_data();
-    $modules = module_rebuild_cache();
+    $themes = _system_get_theme_data();
+    $modules = system_get_module_data();

Shouldn't _system_get_theme_data() be system_get_theme_data() ?
[edit: actually _system_get_theme_data() is called directly in a few other places - not sure that is intended ?]

Dave Reid’s picture

@yched/41: No not at all, since we don't want to load the cached data. That's why the original call is _system_theme_data() instead of system_theme_data().

Berdir’s picture

@yched
_system_get_theme_data() is called at a few places when the database shouldn't be updated. That function does just return what's on the file system while system_get_theme_data() does extend that with the additional information from the database and stores there what has changed.

IIRC, for themes, this lead to a bug when system_get_theme_data() disabled all themes when it is called in update.php

Edit: Maybe a different name would help to indicate this? Something like system_scan_theme_data() ?

Berdir’s picture

Status: Needs work » Needs review
FileSize
652 bytes
brianV’s picture

Status: Needs review » Reviewed & tested by the community

Tested Berdir's latest patch, and it seems to do the trick.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks Berdir, and thanks for testing brianV.

Berdir’s picture

Added a short note about the renamed functions: http://drupal.org/node/224333#system_get_module_data

JohnAlbin’s picture

Title: Unify and rewrite module_rebuild_cache() and system_theme_data() » Regression: Unify and rewrite module_rebuild_cache() and system_theme_data()
Status: Fixed » Active

The re-roll done in comment #28 broke #351487: Remove default values for stylesheet and scripts includes from system module. It looks like an 8-month-old, out-of-date patch was used to move code around instead of actually moving the up-to-date code to the new spots.

From a quick glance at the patch, I also see "comment_user_verification" is gone from the theme settings defaults.

What else was broken?

Berdir’s picture

Status: Active » Needs review
FileSize
959 bytes

Sorry for breaking all these things, you start to rely too much on tests if you have them ;) And it probably went in a bit too fast....

The attached patch re-adds comment_user_verifcation and removes mission and css/js stuff. This was all I could find.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

The script regressions part of the regression has already landed in #351487: Remove default values for stylesheet and scripts includes from system module.

Also marking #497442: Module uninstall leaves system table row as a duplicate of this issue.

Berdir’s picture

Status: Needs work » Needs review
FileSize
638 bytes

Re-roll with the remaining changes.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Berdir, sorry about the conflicting patches! :-)

This patch is RTBC!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Good catch, John. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)

Is there a bugfix here that should be backported to D6?

AdrianB’s picture

Subscribing.

Jerome F’s picture

Subscribing

The duplicate entry error with themes in Drupal 6 is really annoying.
Would this patch ported to D6 get rid of this problem ?
I've got this error on each page load when using CCK, views, panels, system themes, etc. This means all my page loads are heavy, and I have to scroll down on each page, each content I add to my panes and so on... I've searched the web and found no definitive solution. There were many duplicates, in several languages, as well in core and modules issue queues, and some of them were 4 years old (2007).

The patch in http://drupal.org/node/307756#comment-2551466 seems to have done the trick for me. I know that means patching drupal core, but at least I got rid of this error.

Please, do you have any insights about this ?

srobert72’s picture

Subscribing

jannalexx’s picture

Subscribing

mikeytown2’s picture

tim.plunkett’s picture

@mikeytown2 the patch at #307756: Backport of #147000: Unify and rewrite module_rebuild_cache() and system_theme_data() is not a straight backport of the d7 solution, and the patch for this issue is d7, so there is no "correct" d6 patch yet

Danny_Joris’s picture

pwolanin’s picture

I'm working on a Drupal 6 patch, which will include two elements and build off the existing patch from mikeytown2

  1. Use the lock API to avoid having multiple rebuilds in parallel
  2. For existing rows in the system table, update them instead of deleting all and re-inserting to that in the absence of transactions parallel threads doing a select will get valid data
pwolanin’s picture

FileSize
5.86 KB

For reference - here is the last patch from mikeytown2

pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.24 KB

discussed briefly with Gabor, simplifying this condition:

  if (isset($file->status) || (isset($file->old_filename) && $file->old_filename != $file->filename)) {

since it's pretty clear that $file->old_filename can never be set unless $file->status is set.

new, lightly tested patch.

dstol’s picture

Applied the patch in Red Hat SELinux and immediately started seeing this error in my messages.

Jun 29 16:16:12 dev kernel: type=1400 audit(1277842572.957:309): avc:  denied  { getattr } for  pid=15827 comm="httpd" path="/var/www/vhost/drupal/includes/module.inc" dev=dm-0 ino=458849 scontext=user_u:system_r:httpd_t:s0 tcontext=user_u:object_r:tmp_t:s0 tclass=file
bleen’s picture

subscribing ... some have indicated that this issue should solve #477550: "User warning: Duplicate entry ... " ... Themes

CrookedNumber’s picture

I'm probably going to take a beating for this, but I felt obliged to mention it. Namely: there is a difference between a) consciously deleting module files and b) apache, for whatever reason, not being able to immediately find those files. But, to system_update_files_database(), there isn't.

One ill-advised, ill-timed permission change to sites/all will seriously hose your site. That was true in drupal5 and drupal6 as well. But in 5 and 6, disaster relief was simply a matter of fixing the permission problem (and requesting the modules page). However, in drupal7, you'll also have to remember which modules you were running, re-install them, and then deal with any consequences of re-installing them.

Again, I know the solution is not messing with file permissions in the first place. And I too have been down the road (in 5 and 6) where I end up with a long, cruft-filled system table that I had to clean out by hand.

I just wanted to make note of the potentially severe consequences of system_update_files_database().

febbraro’s picture

subscribe

also, @dstol, since the patch changes module.inc try running restorecon to repair the SELinux context on the that file and your error should go away. (I realize this response comes months later)

jbrauer’s picture

Status: Needs review » Reviewed & tested by the community

This has been tested in several places I'm aware of and all have seen positive results.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks, committed.

Gábor Hojtsy’s picture

Status: Fixed » Needs work

Oh, well. The bad news is that if you attempt to install with this patch applied, all you get is Fatal error: Call to undefined function lock_acquire() in ............../modules/system/system.module on line 811 Not really good to start off an install. Rolling back until this is made to actually work.

jbrauer’s picture

We're working on seeing if we can't get this for the 6.20 release....

pwolanin’s picture

Status: Needs work » Needs review
FileSize
8.97 KB

This patch allows Drupal to install successfully, but for some reason theme data is not getting saved in the {system} table, even if I also add a call to system_theme_data() in install.php.

I'm confised about that - suggests there is some other flaw with the changes to system_theme_data().

dstol’s picture

@pwolanin, I'm seeing 100% exactly the same thing. There isn't a result from select * from system where type='theme';

Everything 'fixes' itself when I hit admin/build/themes, which just calls system_theme_data().

dstol’s picture

Ah bingo... line 836 of system.module.

if (isset($theme->status)) {
 db_query("UPDATE {system} SET owner = '%s', info = '%s', filename = '%s' WHERE name = '%s' AND type = '%s'", $theme->owner, serialize($theme->info), $theme->filename, $theme->name, 'theme');
}

It's running an update against a record that doesn't exist in system yet. Upon second glance, it looks more like the problem is in system_get_files_database

jbrauer’s picture

Status: Needs review » Needs work

@pwolanin's patch in #75 looks good and resolves the issue with the install.

However the logic in system.module is failing on the installation. The result of the failure is that the system table is never populated with themes on the install.

dstol’s picture

Status: Needs work » Needs review
FileSize
6.7 KB

Should resolve the

However the logic in system.module is failing on the installation. The result of the failure is that the system table is never populated with themes on the install.
pwolanin’s picture

I think the last patch is missing the new lock-install.inc

pwolanin’s picture

Here's the patch with that added.

david.a.king’s picture

subscribe - is this safe to try? having multiple problems daily..

jbrauer’s picture

@David King ... it should be in pretty good shape. The main body of the patch is well tested. We'd love to have more people test installing new sites with this patch in place and report back.

KerriO’s picture

Subscribing.

Lars Toomre’s picture

I have just upgraded to 6.20 and I can confirm that this frustrating duplicate entry of themes error still exists.

This has been an intermittent problem for me over the past two years and always has been very frustrating. Unfortunately, it is popping up almost all of the time this weekend. The only clue that I can offer is that I see the problem whenever I enable a module that even in the smallest of ways is off with one of its definitions that are incorporated into the menu-router table. From reading above, I am guessing now that as the menu_router entry attempts to get resolved, there must be some type of timeout that causes in turn this issue.

I am very much looking forward to when the patch in #81 is committed so that this frustrating problem finally goes away. In the meantime, does anyone have a good strategy for tracking down errant menu-router entries?? LOL

calefilm’s picture

Subscribing

mcurry’s picture

Subscribing.

Is this patch intended to go into an official Drupal 6 maintenance/point release anytime soon?

I'll start testing as soon as possible but I'm not sure how to trigger the problem behavior (IOW, how will I know if the race condition is fixed...?) and I'm not sure what else to look for given the scope of the patch. Can anyone provide testing guidance?

fourmi4x’s picture

Suscribing

calefilm’s picture

Hi, I have the same problem as described in #85, " user warning: Duplicate entry 'themes/pushbutton/pushbutton.info' for key 'PRIMARY' query: INSERT INTO system....."

I just upgraded to 6.20... Am I supposed to try to apply the patch in #81? would I place it into sites/modules/system/ ?

When I do I get this message:

can't find file to patch at input line 19
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|? .cvsignore
|? 147000-protect-rebuild-74-D6.patch
|? 147000-protect-rebuild-81-D6.patch
|? log.txt
|? sites/8888.127.0.0.1
|? sites/all/apachesolr-6x-2x
|? sites/all/modules
|? sites/all/themes
|? sites/default/files
|? sites/default/files2
|? sites/default/settings.php
|Index: install.php
|===================================================================
|RCS file: /cvs/drupal/drupal/install.php,v
|retrieving revision 1.113.2.13
|diff -u -p -r1.113.2.13 install.php
|--- install.php	6 Dec 2010 06:50:56 -0000	1.113.2.13
|+++ install.php	16 Dec 2010 05:33:25 -0000
--------------------------
File to patch: 

lock-install.inc file shows up in my sites/modules/system folder. Is this what is supposed to happen?

Because in my terminal (using Mac) I still am asked, "File to patch: "

Obviously I I'm way behind all you guys.. thanks for any assistance. I get this error intermittently, especially now that I updated my DATE, Calendar and jQuery modules..

dstol’s picture

@calefilm, http://drupal.org/patch/apply You want to download the patch into the root of Drupal.

calefilm’s picture

Thank you dstol. After patching I got:

root/install.php.orig
root/includes/lock-install.inc

no error as of yet

adamcadot’s picture

Sub

Wan’s picture

Subscribing

calefilm’s picture

I apologize if this isn't where I should be posting this issue--It's hard to find anywhere else that experts are working on this problem other than here.

I had been receiving the infamous "Duplicate entry error" as stated in #89. After applying pwolanin's patch in #81, I stopped receiving the duplicate entry error as it seemed to only apply to Themes.

I stopped receiving the error for a couple weeks as I am building a rather large, modular site and am using up to 120+ modules. I have been using panels a great deal and views a great deal. Can't say I've made many changes to my modules folder as of late or data other than adding the JQuery update module, JQuery Plug-in, and views_slideshow.

I was editing a panel page, and received another type of duplicate error that went on for ages. It's so long I'd be booted off of Drupal if I posed it.

It starts out like this:

user warning: Duplicate entry 'fivestar/preview/color' for key 'PRIMARY' query: INSERT INTO menu_router (path, load_functions, to_arg_functions, access_callback, access_arguments, page_callback, page_arguments, fit, number_parts, tab_parent, tab_root, title, title_callback, title_arguments, type, block_callback, description, position, weight, file) VALUES ('fivestar/preview/color', '', '', 'user_access', 'a:1:{i:0;s:29:\"administer site configuration\";}', 'fivestar_preview_color', 'a:0:{}', 7, 3, '', 'fivestar/preview/color', '', 't', '', 4, '', '', '', 0, 'sites/all/modules/fivestar/fivestar_color.inc') in /Users/Me/Sites/acquia-drupal/includes/menu.inc on line 2460.

user warning: Duplicate entry 'admin/content/node2' for key 'PRIMARY' query: INSERT INTO menu_router (path, load_functions, to_arg_functions, access_callback, access_arguments, page_callback, page_arguments, fit, number_parts, tab_parent, tab_root, title, title_callback, title_arguments, type, block_callback, description, position, weight, file) VALUES ('admin/content/node2', '', '', 'views_access', 'a:1:{i:0;a:2:{i:0;s:16:\"views_check_perm\";i:1;a:1:{i:0;s:16:\"administer nodes\";}}}', 'views_page', 'a:2:{i:0;s:13:\"admin_content\";i:1;s:4:\"page\";}', 7, 3, '', 'admin/content/node2', '', 't', '', 4, '', '', '', 0, '') in /Users/Me/Sites/acquia-drupal/includes/menu.inc on line 2460.

user warning: Duplicate entry 'admin/user/user2' for key 'PRIMARY' query: INSERT INTO menu_router (path, load_functions, to_arg_functions, access_callback, access_arguments, page_callback, page_arguments, fit, number_parts, tab_parent, tab_root, title, title_callback, title_arguments, type, block_callback, description, position, weight, file) VALUES ('admin/user/user2', '', '', 'views_access', 'a:1:{i:0;a:2:{i:0;s:16:\"views_check_perm\";i:1;a:1:{i:0;s:16:\"administer users\";}}}', 'views_page', 'a:2:{i:0;s:11:\"admin_users\";i:1;s:6:\"page_1\";}', 7, 3, '', 'admin/user/user2', '', 't', '', 4, '', '', '', 0, '') in /Users/Me/Sites/acquia-drupal/includes/menu.inc on line 2460.

There are thousands and thousands of Duplicate Entry Errors. I could spend all week copying and pasting every entry topic. Here are a few:

user warning: Duplicate entry 'fivestar/preview/color'
. . . 'admin/content/node2' (this page sends me to my admin/content/node)
'admin/user/user2' (this page sends me to admin/user/user)
'group'/all/feed' ('group' is an organic group - group node)
'admin/settings/availability-calendars' ('Configure global settings for availability calendars module.')
'admin/content/backup_migrate' ('Backup/restore your database or migrate data to or from another Drupal site.', '', 0, '')
'admin/settings/email ( 'Administer flood control settings for email contact forms', '', 0, '')
'admin/settings/comment_notify'
'admin/content/comment'
'admin/settings/connectiv'
'admin/build/contact'
'admin/content/privacy'
'email/%/%'
'admin/build/imagecache'
'admin/og/og_access'
'admin/build/pages'
'node/%/panel_content'

they all end in.... in /Users/Me/Sites/acquia-drupal/includes/menu.inc on line 2460.

I can provide anyone here with specifics as to what's in the errors if this might help.

pillarsdotnet’s picture

Patch in #81 has a slight bug that causes the following PHP warning:

Use of undefined constant MAINTENANCE_MODE - assumed 'MAINTENANCE_MODE' in modules/system/system.module on line 847.

Corrected patch and interdiff attached.

dstol’s picture

+++ modules/system/system.module
@@ -833,7 +833,7 @@
+      if (isset($theme->status) && (defined('MAINTENANCE_MODDE') && MAINTENANCE_MODE != 'install')) {

Spelled mode wrong.

Powered by Dreditor.

dstol’s picture

Status: Needs review » Needs work

Forgot to update status.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
8.09 KB

Oops!

Shadlington’s picture

I tried out #98 because I too am getting these duplicate entry errors.
I haven't been using my test system long enough since I applied the patch to know if the usual errors have stopped, but I have noticed that since applying the patch there are (at least!) two places where I now get the errors, always: admin/build/modules, admin/reports/updates and admin/reports/status.

That said, there is only around 6 errors showing up on each of these pages. Previously, the sporadic ones I would get would arrive in the 100s.

RobbM’s picture

Subscribing.

kyberman’s picture

Subscribing.

jvinci’s picture

subscribing... Thanks.

sonicthoughts’s picture

subscribing

Chemtox’s picture

idem

pillarsdotnet’s picture

FileSize
7.15 KB

Rerolled with "git format-patch"

No other changes.

pillarsdotnet’s picture

FileSize
8.73 KB

Grr... forgot to add includes/lock-install.inc.

Trying again...

apratt’s picture

So the patch isn't working for me. Reading back through the thread it isn't clear to me whether I can apply the patch retroactively or if I need a clean install. The site I'm applying has been running for about six months. Any page that calls for system theme related information seems to trigger the message. I'm also seeing the "not able to keep track of enabled themes." bug mentioned in some of the other threads. Pretty clearly something in the locking code isn't working properly. I'm trying to find the time to do some more in-depth troubleshooting.

pillarsdotnet’s picture

Patch in #106 applies to Drupal 6.x-dev as checked out from the git repository.

If you don't want to deal with git, you can grab a snapshot from here: http://drupalcode.org/project/drupal.git/snapshot/refs/heads/6.x.tar.gz

apratt’s picture

So I got frustrated and just used the simple patch in http://drupal.org/node/307756#80 and commented out the patch above. This worked to solve the problem. When I get some time I'll inspect the code more in-depth.

sonicthoughts’s picture

subscribing - driving me nuts!

ikeigenwijs’s picture

subscribe

alexbk66-’s picture

subscribing

Saoirse1916’s picture

Same problem here...subscribing.

dstol’s picture

Assigned: Dave Reid » Unassigned
Status: Needs review » Reviewed & tested by the community

Since pwolanin and I worked on this in comments 70-80ish the patch in 106 has changed very little, just becoming a git patch. We've been running this in prod for months and months since it was last committed and reverted. RTBC again.

pillarsdotnet’s picture

Issue tags: +backport

Tagging, as this is a backport of code already accepted into 7.x.

alexbk66-’s picture

Should the title of this issue change to something like "Duplicate entry...."?

HobbyBlob.com

bcobin’s picture

subscribing

candelas’s picture

i come from
http://drupal.org/node/477550 that it is closed as duplicate of this one.

i get the same error the first time i load the first page of the site as admin.
it gives an warning with all themes installed (core and zen).
if i reload the page, the error disapears.
i am in local server for constructing the site so i rebuild the theme registry all the time.
php 5.3.2 mysql 5.1.41

TR’s picture

Patch in #106 adds a CVS $Id$ tag to the top of the new includes/lock-install.inc file. That shouldn't be there.

Here's a copy of that patch with the CVS tag removed:

candelas’s picture

i tried the solution in http://drupal.org/node/839184#comment-3152044 and the problem was gone for a while but it has come back.
if i go to the theme page, always my theme has the "default" radio button selected next to it, but the theme enabled check box has been unselected. If i select it the problem goes for a while.
this weekend i have to work, but next i try the patch to see if anything changes.
thanks for the patch :)
i am in a 6.20 and next week will upgrade to 6.22

Shane Birley’s picture

Drupal 6.22

joetheadmin@ubuntu:~/Desktop/drupal-6.22$ patch < 147000-106-nocvstag-D6.patch
patching file lock-install.inc
patching file module.inc
Hunk #1 FAILED at 93.
Hunk #2 FAILED at 119.
2 out of 2 hunks FAILED -- saving rejects to file module.inc.rej
patching file install.php
patching file system.module
Hunk #1 FAILED at 805.
1 out of 1 hunk FAILED -- saving rejects to file system.module.rej

Something has changed in 6.22, I would think. I will look but don't see much difference between 6.20 and 6.22.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
TR’s picture

FileSize
8.72 KB

I'm running Drupal 6.x head straight out of the Git repository, and the patch applies cleanly for me. Renaming and reposting the patch to get the testbot to recognize it and try it out.

Shane Birley’s picture

I attempted to patch a fresh download of 6.22 just to be clear.

Status: Needs review » Needs work

The last submitted patch, 147000-106-nocvstag.patch, failed testing.

dstol’s picture

@Shane, the patch should apply against the 6.x branch.

dstol’s picture

Status: Needs work » Needs review
Issue tags: -Performance, -Patch Spotlight, -backport, -Needs documentation

#123: 147000-106-nocvstag.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +Patch Spotlight, +backport, +Needs documentation

The last submitted patch, 147000-106-nocvstag.patch, failed testing.

Shane Birley’s picture

@dstol, I tried and it failed again. Does this not mean something is wrong with the patch?

dstol’s picture

6.22 is a release tag, while 6.x is a development branch. If you visit http://drupal.org/node/3060/git-instructions/6.x and clone from git and attempt to patch, it should apply.

Shane Birley’s picture

Should we not roll this back to 6.22 as well? It doesn't make sense to patch the latest dev branch when most people with this bug are not using the dev branch. Or am I missing something fundamental here which could be the case...

pillarsdotnet’s picture

Should we not

Yes, we should.

Or am I missing something fundamental here

Perhaps the definition of the word we?

Berdir’s picture

Status: Needs work » Needs review

It is not possible to change fixed releases. Changes and bugfixes can only ever go into the dev branch which will then become the next stable version once released.

Also, AFAIK, the testbot for 6.x does not work correctly. You can place a "d6" in the file name of the patch to have the testbot ignore it.

Shane Birley’s picture

This is exactly the reason a lot of non-technical people don't come to the forums on Drupal.org because we have some members in the community that continually post unhelpful and rude posts like the one above.

@pillarsdotnet, not very Christian of you and quite insulting. You should be ashamed and your attitude in these forums is unfortunate. I pray you think again before posting such comments.

Shane Birley’s picture

@bredir, I am not suggesting that we do anything to change the fixed release beyond having a patch that is able to parse correctly on a current production 6.22 install. I don't see why this patch fails on 6.22, I looked at the script and, from what I can tell, it should apply without an issue. There must be something simple I am missing.

I will see if I can figure that out but, honestly, @pillarsdotnet kind of made me feel like I shouldn't help at all and we wonder why people jump to Wordpress or other CMS systems.

webchick’s picture

Shane, please don't let one person having a bad day in the issue queue get you down. pillarsdotnet, please consider going outside for awhile when you are frustrated. :) *hugs*

Patches always go against the latest dev version, because that's where the next version (6.23) will come from. That's a good one to point your dev server at. Once this patch is tested and ready to go, though, then you could always provide a backported one against 6.22 for the benefit of people who need a fix before 6.23 is released.

Shane Birley’s picture

@webchick: Yah, I am just getting back into the documentation team and catching up on some of my issue queue and this particular patch is getting to me. I have a few hundred Drupal sites and this error doesn't bother most of them. It is the three or four that slow to a crawl because they are waiting for dozens of errors getting pumped into the log by watchdog. I also have a sensitive spot to developers bashing on non-devs, such as myself who spend most of my time in documentation and training land. I used to code a mean Pascal, though.

Also, I am think I am a little sensitive because my holiday time quashed my Drupal London plans. Can't go until Denver. By the way, I just read the article in @linuxjournal the other day - kick ass.

xmacinfo’s picture

Sorry if I take too much time here. :-)

@Shane Birley: I know it can be confusing, but the proper way to fix a release (here Drupal 6.22 is a release), is to release a new version that contains a fix as well as a number of other fixes. So, as with any other software, a new version number is needed (here we would need 6.23).

What you need to do is to do a copy of your 6.22 installation and update it to the latest 6.x-dev (currently dated May 25). Dev version (dev branch) is where all the patch are tested. Once a patch is RTBC and committed, a new dev version (dated more recently) will be available with the fix.

It may take a long time before a new Drupal version is released (6.23 might not be released quickly enough for you). So you may need to run 6.x-dev a while if you need the fix (provided it as been committed, or that you applied a patch).

If you still need to run a released version of Drupal, but need a simple fix, you would need to reroll the patch yourself against Drupal 6.22. That patch will most probably be different that the patch applied to the dev release.

However, I advise against applying patches to a released version of Drupal. For a neophyte it can be very hard to manage.

TR’s picture

If I can reset the discussion back to the issue at hand ...

@Berdir (#133): The patch in #119 was named 147000-106-nocvstag-D6.patch to keep the testbot from running it. #121 claimed the patch didn't apply, so I reposted the exact same patch as 147000-106-nocvstag.patch to force the testbot to apply it. The testbot was able to apply the patch cleanly, which should serve as the definitive test that the patch as written applies to D6.x head.

@Shane Birley (#121): I don't know why the patch didn't apply for you. However, I notice if I try to apply the patch to Drupal 7.x (not 6.x), I get the exact same errors as you posted. That suggests to me that you might have accidentally tried to apply the patch to a Drupal 7 site. Regardless, the testbot results show that the patch does apply cleanly, so I think this is a problem on your end, not a problem with the patch. I'd be happy to help you figure out what's going wrong, but let's do it over IRC or by PM so it doesn't derail this thread.

The current patch under consideration is #123, which is exactly the same as #106 but with the CVS $Id$ tag removed. #106 was published on 19 March 2011, well before Drupal 6.22. It applied cleanly back then, and it applies cleanly now in 6.x-dev. In fact, I just checked out 6.22 and it applies cleanly in 6.22. As I said above, the problems applying the patch that were raised in #121 are clearly not a problem with the patch.

A number of people above have stated that they're experiencing the problem raised in this issue. Can we get a review from one of you as to whether this patch works to solve the problem?

Shane Birley’s picture

@TR: Sounds good. I applied the patch to a fresh install of 6.22, which is why I was concerned I was doing something wrong back in #131. I will hit you up in IRC.

Time to drink beer...

candelas’s picture

thanks a lot for the patch :)

this weekend i upgrade a site to 6.22 and i will try it.
i will report how it goes.

dstol’s picture

Status: Needs review » Reviewed & tested by the community

I'm not exactly sure why Gabor marked it as needs review in #122 anyways. I'm going to assume it was because of the comment that the patch didn't apply in #121, which isn't the case against 6.x as tested by TR in #139 and myself in #126.

Back to RTBC for now, unless my previous assumption is wrong.

Fabianx’s picture

I can confirm: Applies fine for all versions from: 6.0 to 6.22 and 6.x. ( No, I did not test that manually. )

@Shane Birley: Not sure what problem you had, perhaps you had another core patch applied when trying to test the patch?

Best Wishes,

Fabian

Shane Birley’s picture

@Fabianx: No, I tried it on a completely new installer download. I must be doing something incorrectly.

Fabianx’s picture

@Shane Birley: Can you still reproduce the problem?

If yes, can you post the contents of the *.rej files to pastebin.com or so?

That would help us see where your problem could be.

Thanks and Best Wishes,

Fabian

Shane Birley’s picture

@Fabianx: Yes, I just tried again on both 6.x and 6.22 - each failed. It must be something I am doing but it is pretty hard to screw up applying a patch.

Here are the .rej files from the 6.x attempt.

module.inc.reg

--- a/includes/module.inc
+++ b/includes/module.inc
@@ -93,6 +93,14 @@
  *   The array of filesystem objects used to rebuild the cache.
  */
 function module_rebuild_cache() {
+  $write_database = TRUE;
+  // If lock not acquired, return $files data without writing to database.
+  if (!lock_acquire('module_rebuild_cache')) {
+    $write_database = FALSE;
+    // Wait for the parallel thread to be done so we are more likely
+    // to get updated and consistent data.
+    lock_wait('module_rebuild_cache');
+  }
   // Get current list of modules
   $files = drupal_system_listing('\.module$', 'modules', 'name', 0);
 
@@ -119,32 +127,39 @@
       unset($files[$filename]);
       continue;
     }
-    // Merge in defaults and save.
-    $files[$filename]->info = $file->info + $defaults;
 
     // Invoke hook_system_info_alter() to give installed modules a chance to
     // modify the data in the .info files if necessary.
     drupal_alter('system_info', $files[$filename]->info, $files[$filename]);
 
-    // Log the critical hooks implemented by this module.
-    $bootstrap = 0;
-    foreach (bootstrap_hooks() as $hook) {
-      if (module_hook($file->name, $hook)) {
-        $bootstrap = 1;
-        break;
+    // Merge in defaults and save.
+    $files[$filename]->info = $file->info + $defaults;
+  }
+
+  // If lock not acquired, return $files data without writing to database.
+  if ($write_database) {
+    foreach ($files as $filename => $file) {
+      // Log the critical hooks implemented by this module.
+      $bootstrap = 0;
+      foreach (bootstrap_hooks() as $hook) {
+        if (module_hook($file->name, $hook)) {
+          $bootstrap = 1;
+          break;
+        }
       }
-    }
 
-    // Update the contents of the system table:
-    if (isset($file->status) || (isset($file->old_filename) && $file->old_filename != $file->filename)) {
-      db_query("UPDATE {system} SET info = '%s', name = '%s', filename = '%s', bootstrap = %d WHERE filename = '%s'", serialize($files[$filename]->info), $file->name, $file->filename, $bootstrap, $file->old_filename);
-    }
-    else {
-      // This is a new module.
-      $files[$filename]->status = 0;
-      $files[$filename]->throttle = 0;
-      db_query("INSERT INTO {system} (name, info, type, filename, status, throttle, bootstrap) VALUES ('%s', '%s', '%s', '%s', %d, %d, %d)", $file->name, serialize($files[$filename]->info), 'module', $file->filename, 0, 0, $bootstrap);
+      // Update the contents of the system table:
+      if (isset($file->status)) {
+        db_query("UPDATE {system} SET info = '%s', name = '%s', filename = '%s', bootstrap = %d WHERE filename = '%s'", serialize($files[$filename]->info), $file->name, $file->filename, $bootstrap, $file->old_filename);
+      }
+      else {
+        // This is a new module.
+        $files[$filename]->status = 0;
+        $files[$filename]->throttle = 0;
+        db_query("INSERT INTO {system} (name, info, type, filename, status, throttle, bootstrap) VALUES ('%s', '%s', '%s', '%s', %d, %d, %d)", $file->name, serialize($files[$filename]->info), 'module', $file->filename, 0, 0, $bootstrap);
+      }
     }
+    lock_release('module_rebuild_cache');
   }
   $files = _module_build_dependencies($files);
   return $files;

install.php.rej

--- a/install.php
+++ b/install.php
@@ -130,6 +130,12 @@
     if (!$verify) {
       install_change_settings($profile, $install_locale);
     }
+    // The default lock implementation uses a database table,
+    // so we cannot use it for install, but we still need
+    // the API functions available.
+    require_once './includes/lock-install.inc';
+    $conf['lock_inc'] = './includes/lock-install.inc';
+    lock_init();
 
     // Install system.module.
     drupal_install_system();

system.module.rej

--- a/modules/system/system.module
+++ b/modules/system/system.module
@@ -805,22 +805,48 @@
  *   Array of all available themes and their data.
  */
 function system_theme_data() {
+  $write_database = TRUE;
+  // If lock not acquired, return $files data without writing to database.
+  if (!lock_acquire('system_theme_data')) {
+    $write_database = FALSE;
+    // Wait for the parallel thread to be done so we are more likely
+    // to get updated and consistent data.
+    lock_wait('system_theme_data');
+  }
   // Scan the installation theme .info files and their engines.
   $themes = _system_theme_data();
+  foreach ($themes as $key => $theme) {
+    if (!isset($theme->owner)) {
+      $themes[$key]->owner = '';
+    }
+  }
 
   // Extract current files from database.
   system_get_files_database($themes, 'theme');
 
-  db_query("DELETE FROM {system} WHERE type = 'theme'");
+  // If lock not acquired, return $themes data without writing to database.
+  if ($write_database) {
+    $names = array();
 
-  foreach ($themes as $theme) {
-    if (!isset($theme->owner)) {
-      $theme->owner = '';
+    foreach ($themes as $theme) {
+      // Record the name of each theme found in the file system.
+      $names[] = $theme->name;
+      // Update the contents of the system table.
+      if (isset($theme->status) && (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE != 'install')) {
+        db_query("UPDATE {system} SET owner = '%s', info = '%s', filename = '%s' WHERE name = '%s' AND type = '%s'", $theme->owner, serialize($theme->info), $theme->filename, $theme->name, 'theme');
+      }
+      else {
+        $theme->status = ($theme->name == variable_get('theme_default', 'garland'));
+        // This is a new theme.
+        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, $theme->status, 0, 0);
+      }
     }
-
-    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);
+    // Delete from the system table any themes missing from the file system.
+    if ($names) {
+      db_query("DELETE FROM {system} WHERE type = 'theme' AND name NOT IN (". db_placeholders($names, 'varchar') .")", $names);
+    }
+    lock_release('system_theme_data');
   }
-
   return $themes;
 }
 
candelas’s picture

thanks a lot for the patch :)

i applied the patch and the site works better, but i still get the message in some pages.
now the line is 841
i have not clear which pages...
but i am lucky that, in pages that are like edit a node (or other that my client is going to use) or what a anonymous user will see, the message doesnt come. so when i put the site in production, no problem. thanks a lot :)

but in pages like admin/build/themes, admin/build/modules, admin/reports/dblog it comes.
but.. not always...

i will be checking to see if i found something in comun in those pages...

edit: since i had a little of time, i investigate and these are my results:

I get the message when:

  • admin/build/views/edit/taxonomia Referrer admin/build/views/edit/taxonomia
  • admin/build/path Referrer admin/build/path?page=1
  • admin/build/block Referrer admin/build/block/configure/views/taxonomia-block_2
  • admin/build/path/list Referrer taxonomy/term/16
  • admin/rules/rules/rules_boletin_promover_portada/edit Referrer admin/rules/trigger
  • admin/build/themes Referrer admin/reports/event/35913
  • admin/build/modules Referrer admin/build/modules
  • admin/reports/dblog Referrer admin/reports/dblog?page=1 (but if i change page by the pager it doesnt do it)
  • admin/content/taxonomy/5 Referrer admin/content/taxonomy

i dont get the message when:

  • i view pages that are not admin (not all admin give error)
  • i edit a node and save
  • i delete a taxonomy term
  • i create a view
  • i edit a view (sometimes)
  • i delete a view

did the people that tried the patch have this problems or it is only me (maybe something i have configured in drupal or a module or x)?

:)

Danny Englander’s picture

Subscribing

candelas’s picture

i dont know if it has to do with this, but now i cant install a module http://drupal.org/project/browscap that deals with theme nor one that works with cache http://drupal.org/project/search_files. i can install other modules. if i try in a fresh install, i can.

for me, this is too complicated... i hope it gets solved soon... thanks a lot for your generous work :)

candelas’s picture

since i suffer so much, i tell everywhere that i have been reading and trying patches.
i disable admin menu, and my problems are gone. no need of patches.
an alternative is http://drupal.org/project/simplemenu

thanks to everybody, and also to admin menu that i have been using for years... :)

Juc1’s picture

Just to recap -

# 139 Posted by TR on June 4, 2011 at 1:15am

A number of people above have stated that they're experiencing the problem raised in this issue. Can we get a review from one of you as to whether this patch works to solve the problem?

# 147 Posted by candelas on August 7, 2011 at 12:33pm

('duplicate entry' problem is not solved)

Does anyone agree with # 150 that this problem relates to admin menu?

calefilm’s picture

I did at first. But now I'm using Admin Menu again error free.

(NOTE: NO MORE ERRORS, but I'm still losing my enabled 'checked' themes, however I'm applying Page Theme to keep all themes running)

Maybe someone can answer this simple question. Am I supposed to be following this thread if I have a "Duplicate Entry" issue?

Mikeytown2 posted a patch that resolved my problem entirely.. as my errors have not come back.
http://drupal.org/node/307756#comment-3052424

I originally posted this "Duplicate entry" issue at Admin_Menu:
http://drupal.org/node/1033240#comment-4877510 (this is a history of changes I've made since getting this error)

I've been getting my feet wet with Drupal for about two years now but am very lost in threads like these. I apologize. So I don't even know if these patches apply to my issue... however, after Mikeytown2 posted his patch everyone said it was a duplicate issue and to come here! Well, his patch worked--but now I'm afraid no one knows about it and it won't be applied to future versions of drupal.

mattcasey’s picture

subscribing

alexharries’s picture

Subscribing...

dww’s picture

@alexharries (and everyone else): Stop subscribing, start following! Thanks.

RaulMuroc’s picture

Well, at the end situation is at least:

Last known patch for D6:

- deactivate themes after a while.
- it doesn't let activate modules. It was happening to me, i had to put again original drupal core files. I was not able, after a week of the patch, to activate modules. Now, with original files yes.

I am gonna continue working on that. Think we'all should be, anyway it is for D6, perhaps with D7 will happen same, cuz it is problems which rises after X time.

greg.harvey’s picture

Status: Reviewed & tested by the community » Needs work

I've been asked to review and test this patch here: #1331676: Update on cron appears to be disabling themes (which has been marked a duplicate by jbrauer)

So I guess the R&TBC label is not properly "acknowledged". First question - now there's a lock.inc as of Drupal 6.16 (March 4, 2010) what exactly is lock-install.inc actually doing in this patch? Correct me if I'm wrong, but it looks to me like lock-install.inc is just a faked-up copy of the same functions found in lock.inc but with no actual logic.

As I say, I prepare to stand corrected, but I really can't see what it's doing in there? =/

greg.harvey’s picture

Status: Needs work » Needs review

And I do stand corrected - suddenly it's clear:

+    // The default lock implementation uses a database table,
+    // so we cannot use it for install, but we still need
+    // the API functions available.
+    require_once './includes/lock-install.inc';
+    $conf['lock_inc'] = './includes/lock-install.inc';
+    lock_init();

Marching on!

dstol’s picture

Title: Rewrite module_rebuild_cache() and system_theme_data() » Regression: Unify and rewrite module_rebuild_cache() and system_theme_data()

@greg.harvey, the installer isn't a fully bootstrapped so calling lock_acquire() fatals. See #73.

greg.harvey’s picture

Status: Needs review » Needs work

@dstol, I think that was fixed in #75 with the addition of lock-install.inc and its inclusion in install.php - though I haven't tested an install, it *should* be fine now.

But there are other problems too. Applying this to 6.22 I get a pile of User warning: Duplicate entry messages, one for each theme, and the theme page no longer works (enabled/disabled checkboxes all empty except for the default theme and changes to that state don't stick - it's impossible to enable a theme).

So definitely "needs work".

I might be able to work on this a bit next week. I'll try.

greg.harvey’s picture

Title: Regression: Unify and rewrite module_rebuild_cache() and system_theme_data() » Rewrite module_rebuild_cache() and system_theme_data()

Oh, and we can lose the "regression" - that was for D7 and fixed ages ago.

jbrauer’s picture

Title: Regression: Unify and rewrite module_rebuild_cache() and system_theme_data() » Rewrite module_rebuild_cache() and system_theme_data()

Tested several patches here.

#81 appears to work in that it makes the Duplicate entry errors go away. However I'm not certain because of the failing conditional statement that it ever rebuilds them. (#156 seems to echo this possibility)

#106 and the related 123/119 resolve the undefined constant errors but cause errors about trying to insert duplicate theme entries, even in cases where those wouldn't otherwise be seen.

jbrauer’s picture

Priority: Normal » Major

the problem is in this line

if (isset($theme->status) && (defined('MAINTENANCE_MODDE') && MAINTENANCE_MODE != 'install')) {

Outside of install/update.php it's always

TRUE && (FALSE &&TRUE) ... patching momentarily

jbrauer’s picture

Status: Needs work » Needs review
FileSize
6.65 KB

OK here is what could be the lucky patch ...

With this I tested installation, manually deleting a theme from the theme table then visiting the themes page to see that it was properly rebuilt, submitting the theme page and making sure settings were saved.

Status: Needs review » Needs work

The last submitted patch, protect_theme_rebuild-147000-164.patch, failed testing.

jbrauer’s picture

URG .. lock file ...

jbrauer’s picture

Status: Needs work » Needs review

and status... churn away testbot

dstol’s picture

Confirming the results jbrauer mentions in #162 and #163.

The fatal Gabor mentions in #73 that caused a patch to be reverted out of 6.19 has been addressed in #81 and is fixed in #166.

Marking #166 RTBC.

greg.harvey’s picture

Status: Needs review » Reviewed & tested by the community

Will test #166 myself this week. =)

greg.harvey’s picture

Applied this to a project that was suffering theme disabling issues almost certainly related. Seems to work fine for now, no errors. I'll know in a few weeks if this also stops the theme disabling problem, but in any case, seems there is no functional problem with this latest patch, so +1 from me. =)

jbrauer’s picture

Category: task » bug

Updating category to reflect that this fixes a major bug (i.e. all themes disabled)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed, pushed.

ELC’s picture

Status: Fixed » Needs review
FileSize
1.17 KB

I have a multi-site install which was 6.22 before I updated to 6.24. The sites seem not to suffer any bad experiences (they still work). When running "drush -l site updatedb", or removing an overriding theme, I get a series of duplicate key errors

Duplicate entry 'themes/bluemarine/bluemarine.info' for key 'PRIMARY' [warning]
query: system_theme_data
/* admin : system_theme_data */ INSERT INTO tsk_system (name, owner, info, type, filename, status, throttle, bootstrap) VALUES
('bluemarine', 'themes/engines/phptemplate/phptemplate.engine',
'a:13:{s:4:\"name\";s:10:\"Bluemarine\";s:11:\"description\";s:66:\"Table-based multi-column theme with a
marine and ash color
scheme.\";s:7:\"version\";s:4:\"6.24\";s:4:\"core\";s:3:\"6.x\";s:6:\"engine\";s:11:\"phptemplate\";s:7:\"project\";s:6:\"drupal\";s:9:\"datestamp\";s:10:\"1328134267\";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/bluemarine/style.css\";}}s:7:\"scripts\";a:1:{s:9:\"script.js\";s:27:\"themes/bluemarine/script.js\";}s:10:\"screenshot\";s:32:\"themes/bluemarine/screenshot.png\";s:3:\"php\";s:5:\"4.3.5\";}',
'theme', 'themes/bluemarine/bluemarine.info', 0, 0, 0) database.mysqli.inc:134

The issue seems to be in system_theme_data() in the following condition:

if (isset($theme->status) && !(defined('MAINTENANCE_MODE') && MAINTENANCE_MODE != 'install')) {
        db_query("UPDATE {system} SET owner = '%s', info = '%s', filename = '%s' WHERE name = '%s' AND type = '%s'", $theme->owner, serialize($theme->info), $theme->filename, $theme->name, 'theme');
      }
      else {
        $theme->status = ($theme->name == variable_get('theme_default', 'garland'));
        // This is a new theme.
        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, $theme->status, 0, 0);
      }

It is always running the INSERT code instead of the update code because

  • status is set (either 0 or 1)
  • MAINTENANCE_MODE is defined
  • MAINTENANCE_MODE is set to "update"

.. which means that the INSERT is happening, even though the entry exists in the database leading to every already existing but disabled theme causing a duplicate key error.

As I said before, this seems to be somewhat restricted to drush updatedb, but it also occurs when a module which was previously in the normal location + an overridden location, and then the overridden location is removed. The database ends up with 2 themes with the same name and attempts to set the same filename field value for the one which no longer has any files present. I ran into this issue at the same time as the drush issue which made two separate errors turn up at the same time with almost the exact same symptoms. The fix below does not fix this issue; the duplicate keys will happen at least twice every cache flush.

The original version used to delete all the values from the system table and then rebuild them from scratch which meant that there was never going to be a duplicate set of identical things to enter into the database.

I fixed it by changing the condition to

if (isset($theme->status) && !(defined('MAINTENANCE_MODE') && (MAINTENANCE_MODE != 'install' && MAINTENANCE_MODE != 'update'))) {

.. but I'm afraid I don't have the in depth knowledge to know why the MAINTENANCE_MODE is checked, or if what I'm doing is remotely safe and maintains the original intent.

I've attached a patch of this for your amusement. Should this instead go over to drush and tell them they need to set MAINTENANCE_MODE to "install" when running updatedb (that doesn't really make sense in terms of english)?

xmacinfo’s picture

@ELC: You should open a new issue (and reference here) instead of reopening this issue.

lort’s picture

I can confirm your 'fix' works @ ELC #173

Thank you for taking the time post your findings.

EDIT: Issue started for me too when upgraded from 6.23 to 6.24

jmuessig’s picture

ELC #173 fix works for me, though I had to manually patch for some reason.

Issue started for me on upgrade from drupal core 6.23 to 6.24.

Dane Powell’s picture

Status: Needs review » Fixed

I propose that we continue discussion on #173 in #1425868: Duplicate entry of themes primary key in systems table of Drupal 6.24 (using Drush or Ægir). Note that the poster of that patch specifically mentions that the patch is 'for amusement only' and should not be considered a proper fix (though it's still certainly appreciated!)

greg.harvey’s picture

I took the liberty of moving the patch to comment #7 in #1425868: Duplicate entry of themes primary key in systems table of Drupal 6.24 (using Drush or Ægir) to continue this further issue there.

ELC’s picture

@xmacinfo cheers; I'll keep that in mind for future reference. It was late and git blame pointed me here ;)

After reviewing what I was looking at though, I think I put it in here because there was enough of problem that the entire function may need to be revisited. To deal with both errors, the behaviour needs change fairly significantly.

Anyway, I have continued in the other post.

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -Patch Spotlight, -backport, -Needs documentation

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