Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
-
$modules = system_data('modules'); $themes = system_data('themes');
-
$modules = system_module_data(); $themes = system_theme_data();
Comment | File | Size | Author |
---|---|---|---|
#173 | 147000-system-duplicate-key-error-theme-data-173.patch | 1.17 KB | ELC |
#166 | protect_theme_rebuild-147000-166.patch | 8.11 KB | jbrauer |
#164 | protect_theme_rebuild-147000-164.patch | 6.65 KB | jbrauer |
#123 | 147000-106-nocvstag.patch | 8.72 KB | TR |
#119 | 147000-106-nocvstag-D6.patch | 8.72 KB | TR |
Comments
Comment #1
dwwThis cleanup/unification would also be a good place to add the hook_alter_info() I'm proposing over at http://drupal.org/node/152926.
Comment #2
drewish CreditAttribution: drewish commentedsubscribing. personally i like #1 better.
Comment #3
dwwmaybe we should call it
drupal_system_data()
to defend ourselves from system.module needing to implement hook_data() at some point in the future?Comment #4
dwwor 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. ;)
Comment #5
drewish CreditAttribution: drewish commenteddigging 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.
Comment #6
dwwyeah, 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.
Comment #7
Dave ReidI'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.
Comment #8
webchickSmall 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.Comment #9
Dave ReidHere'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().
Comment #10
Dave ReidHere'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.
Comment #11
dwwVery cool, thanks. #10 sounds especially promising, though I haven't had a chance to look at the patch. Keep up the great work!
Comment #12
arhak CreditAttribution: arhak commentedsubscribing
Comment #13
Dave ReidMaking 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.
Comment #14
pillarsdotnet CreditAttribution: pillarsdotnet commentedBackported to version 6.4 here: http://drupal.org/node/307756
Comment #15
Dave ReidRerolled and bumped for review.
Comment #16
Dave ReidRe-rolled for HEAD, but the patch is causing a errors on install, so I need to take a look at what's going wrong.
Comment #17
pillarsdotnet CreditAttribution: pillarsdotnet commentedCan'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
Comment #18
Dave ReidFixed some comment coding standards, will need to test if this still applies. Too tired right now...
Comment #19
Dave ReidRe-rolled for head. I also did some actual ab performance testing. I like the results, and I bet you will too!
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!
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe. 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.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe patch looks good by itself.
A couple of issues:
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.
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.
Two words: file system.
A space is missing after foreach.
This is badly formatted.
Dot missing at the end of the sentence.
I guess that's "Read info files for each module". Plus the final dot is missing.
Do we still need this? I thought we removed the notion of bootstrap module.
Same remark as above.
Comment #23
Dave ReidAwesome 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.
Comment #24
Dave ReidComment #25
Dave ReidComment #26
mathieu CreditAttribution: mathieu commentedSubscribe.
Comment #27
EvanDonovan CreditAttribution: EvanDonovan commentedAnyone working on this? Looks like it would be a critical performance improvement...
Comment #28
BerdirRe-roll... that was.. erm... fun... NOT ;)
About DamZ's review..
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.
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..
Comment #29
Dries CreditAttribution: Dries commentedFrom 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:
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.
Comment #30
Berdir- Changed $files_updated to $files
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:
Patch without static cache:
Path with static cache and #429132: Remove unecessary module_rebuild_cache() call from _registry_rebuild() reverted:
Comment #32
BerdirForgot to remove a debug call...
Comment #33
BerdirBad update.module fooled my :)
Comment #34
BerdirPassed: 149 passes..
That's not exactly enough ;) I assume the tests don't like the static cache..
Comment #35
BerdirI 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()
Comment #36
Dries CreditAttribution: Dries commentedLooks 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.
Comment #37
Dave ReidThanks Berdir for picking this up where I left off. It was much needed.
Comment #38
webchickThis was not actually marked "needs work" :)
Comment #39
cburschkaI'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?
Comment #40
BerdirI'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.
Comment #41
yched CreditAttribution: yched commentedAlso, the committed patch contains
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 ?]
Comment #42
Dave Reid@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().
Comment #43
Berdir@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() ?
Comment #44
BerdirComment #45
brianV CreditAttribution: brianV commentedTested Berdir's latest patch, and it seems to do the trick.
Comment #46
Dries CreditAttribution: Dries commentedCommitted. Thanks Berdir, and thanks for testing brianV.
Comment #47
BerdirAdded a short note about the renamed functions: http://drupal.org/node/224333#system_get_module_data
Comment #48
JohnAlbinThe 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?
Comment #49
BerdirSorry 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.
Comment #51
Dave ReidThe 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.
Comment #52
BerdirRe-roll with the remaining changes.
Comment #53
JohnAlbinBerdir, sorry about the conflicting patches! :-)
This patch is RTBC!
Comment #54
Dries CreditAttribution: Dries commentedGood catch, John. Committed to CVS HEAD. Thanks.
Comment #56
pwolanin CreditAttribution: pwolanin commentedIs there a bugfix here that should be backported to D6?
Comment #57
AdrianB CreditAttribution: AdrianB commentedSubscribing.
Comment #58
Jerome F CreditAttribution: Jerome F commentedSubscribing
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 ?
Comment #59
srobert72 CreditAttribution: srobert72 commentedSubscribing
Comment #60
jannalexx CreditAttribution: jannalexx commentedSubscribing
Comment #61
mikeytown2 CreditAttribution: mikeytown2 commentedSubscribe from #307756: Backport of #147000: Unify and rewrite module_rebuild_cache() and system_theme_data()
Which patch is the correct one?
Comment #62
tim.plunkett@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
Comment #63
Danny_Joris CreditAttribution: Danny_Joris commentedSubscribing. I was following the issue at #307756: Backport of #147000: Unify and rewrite module_rebuild_cache() and system_theme_data() . (for D6)
Comment #64
pwolanin CreditAttribution: pwolanin commentedI'm working on a Drupal 6 patch, which will include two elements and build off the existing patch from mikeytown2
Comment #65
pwolanin CreditAttribution: pwolanin commentedFor reference - here is the last patch from mikeytown2
Comment #66
pwolanin CreditAttribution: pwolanin commenteddiscussed briefly with Gabor, simplifying this condition:
since it's pretty clear that
$file->old_filename
can never be set unless$file->status
is set.new, lightly tested patch.
Comment #67
dstolApplied the patch in Red Hat SELinux and immediately started seeing this error in my messages.
Comment #68
bleen CreditAttribution: bleen commentedsubscribing ... some have indicated that this issue should solve #477550: "User warning: Duplicate entry ... " ... Themes
Comment #69
CrookedNumber CreditAttribution: CrookedNumber commentedI'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().
Comment #70
febbraro CreditAttribution: febbraro commentedsubscribe
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)
Comment #71
jbrauer CreditAttribution: jbrauer commentedThis has been tested in several places I'm aware of and all have seen positive results.
Comment #72
Gábor HojtsyGreat, thanks, committed.
Comment #73
Gábor HojtsyOh, 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.Comment #74
jbrauer CreditAttribution: jbrauer commentedWe're working on seeing if we can't get this for the 6.20 release....
Comment #75
pwolanin CreditAttribution: pwolanin commentedThis 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().
Comment #76
dstol@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().
Comment #77
dstolAh bingo... line 836 of system.module.
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
Comment #78
jbrauer CreditAttribution: jbrauer commented@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.
Comment #79
dstolShould resolve the
Comment #80
pwolanin CreditAttribution: pwolanin commentedI think the last patch is missing the new lock-install.inc
Comment #81
pwolanin CreditAttribution: pwolanin commentedHere's the patch with that added.
Comment #82
david.a.king CreditAttribution: david.a.king commentedsubscribe - is this safe to try? having multiple problems daily..
Comment #83
jbrauer CreditAttribution: jbrauer commented@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.
Comment #84
KerriO CreditAttribution: KerriO commentedSubscribing.
Comment #85
Lars Toomre CreditAttribution: Lars Toomre commentedI 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
Comment #86
calefilm CreditAttribution: calefilm commentedSubscribing
Comment #87
mcurry CreditAttribution: mcurry commentedSubscribing.
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?
Comment #88
fourmi4x CreditAttribution: fourmi4x commentedSuscribing
Comment #89
calefilm CreditAttribution: calefilm commentedHi, 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:
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..
Comment #90
dstol@calefilm, http://drupal.org/patch/apply You want to download the patch into the root of Drupal.
Comment #91
calefilm CreditAttribution: calefilm commentedThank you dstol. After patching I got:
root/install.php.orig
root/includes/lock-install.inc
no error as of yet
Comment #92
adamcadot CreditAttribution: adamcadot commentedSub
Comment #93
Wan CreditAttribution: Wan commentedSubscribing
Comment #94
calefilm CreditAttribution: calefilm commentedI 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:
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.
Comment #95
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch in #81 has a slight bug that causes the following PHP warning:
Corrected patch and interdiff attached.
Comment #96
dstolSpelled mode wrong.
Powered by Dreditor.
Comment #97
dstolForgot to update status.
Comment #98
pillarsdotnet CreditAttribution: pillarsdotnet commentedOops!
Comment #99
Shadlington CreditAttribution: Shadlington commentedI 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.
Comment #100
RobbM CreditAttribution: RobbM commentedSubscribing.
Comment #101
kybermanSubscribing.
Comment #102
jvinci CreditAttribution: jvinci commentedsubscribing... Thanks.
Comment #103
sonicthoughts CreditAttribution: sonicthoughts commentedsubscribing
Comment #104
Chemtox CreditAttribution: Chemtox commentedidem
Comment #105
pillarsdotnet CreditAttribution: pillarsdotnet commentedRerolled with "git format-patch"
No other changes.
Comment #106
pillarsdotnet CreditAttribution: pillarsdotnet commentedGrr... forgot to add includes/lock-install.inc.
Trying again...
Comment #107
apratt CreditAttribution: apratt commentedSo 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.
Comment #108
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch 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
Comment #109
apratt CreditAttribution: apratt commentedSo 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.
Comment #110
sonicthoughts CreditAttribution: sonicthoughts commentedsubscribing - driving me nuts!
Comment #111
ikeigenwijs CreditAttribution: ikeigenwijs commentedsubscribe
Comment #112
alexbk66- CreditAttribution: alexbk66- commentedsubscribing
Comment #113
Saoirse1916 CreditAttribution: Saoirse1916 commentedSame problem here...subscribing.
Comment #114
dstolSince 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.
Comment #115
pillarsdotnet CreditAttribution: pillarsdotnet commentedTagging, as this is a backport of code already accepted into 7.x.
Comment #116
alexbk66- CreditAttribution: alexbk66- commentedShould the title of this issue change to something like "Duplicate entry...."?
HobbyBlob.com
Comment #117
bcobin CreditAttribution: bcobin commentedsubscribing
Comment #118
candelas CreditAttribution: candelas commentedi 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
Comment #119
TR CreditAttribution: TR commentedPatch 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:
Comment #120
candelas CreditAttribution: candelas commentedi 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
Comment #121
Shane Birley CreditAttribution: Shane Birley commentedDrupal 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.
Comment #122
Gábor HojtsyComment #123
TR CreditAttribution: TR commentedI'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.
Comment #124
Shane Birley CreditAttribution: Shane Birley commentedI attempted to patch a fresh download of 6.22 just to be clear.
Comment #126
dstol@Shane, the patch should apply against the 6.x branch.
Comment #127
dstol#123: 147000-106-nocvstag.patch queued for re-testing.
Comment #129
Shane Birley CreditAttribution: Shane Birley commented@dstol, I tried and it failed again. Does this not mean something is wrong with the patch?
Comment #130
dstol6.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.
Comment #131
Shane Birley CreditAttribution: Shane Birley commentedShould 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...
Comment #132
pillarsdotnet CreditAttribution: pillarsdotnet commentedYes, we should.
Perhaps the definition of the word we?
Comment #133
BerdirIt 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.
Comment #134
Shane Birley CreditAttribution: Shane Birley commentedThis 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.
Comment #135
Shane Birley CreditAttribution: Shane Birley commented@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.
Comment #136
webchickShane, 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.
Comment #137
Shane Birley CreditAttribution: Shane Birley commented@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.
Comment #138
xmacinfoSorry 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.
Comment #139
TR CreditAttribution: TR commentedIf 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?
Comment #140
Shane Birley CreditAttribution: Shane Birley commented@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...
Comment #141
candelas CreditAttribution: candelas commentedthanks 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.
Comment #142
dstolI'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.
Comment #143
Fabianx CreditAttribution: Fabianx commentedI 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
Comment #144
Shane Birley CreditAttribution: Shane Birley commented@Fabianx: No, I tried it on a completely new installer download. I must be doing something incorrectly.
Comment #145
Fabianx CreditAttribution: Fabianx commented@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
Comment #146
Shane Birley CreditAttribution: Shane Birley commented@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
install.php.rej
system.module.rej
Comment #147
candelas CreditAttribution: candelas commentedthanks 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:
i dont get the message when:
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)?
:)
Comment #148
Danny EnglanderSubscribing
Comment #149
candelas CreditAttribution: candelas commentedi 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 :)
Comment #150
candelas CreditAttribution: candelas commentedsince 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... :)
Comment #151
Juc1 CreditAttribution: Juc1 commentedJust to recap -
# 139 Posted by TR on June 4, 2011 at 1:15am
# 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?
Comment #152
calefilm CreditAttribution: calefilm commentedI 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.
Comment #153
mattcasey CreditAttribution: mattcasey commentedsubscribing
Comment #154
alexharries CreditAttribution: alexharries commentedSubscribing...
Comment #155
dww@alexharries (and everyone else): Stop subscribing, start following! Thanks.
Comment #156
RaulMuroc CreditAttribution: RaulMuroc commentedWell, 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.
Comment #157
greg.harveyI'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? =/
Comment #158
greg.harveyAnd I do stand corrected - suddenly it's clear:
Marching on!
Comment #159
dstol@greg.harvey, the installer isn't a fully bootstrapped so calling lock_acquire() fatals. See #73.
Comment #160
greg.harvey@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.
Comment #161
greg.harveyOh, and we can lose the "regression" - that was for D7 and fixed ages ago.
Comment #162
jbrauer CreditAttribution: jbrauer commentedTested 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.
Comment #163
jbrauer CreditAttribution: jbrauer commentedthe 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
Comment #164
jbrauer CreditAttribution: jbrauer commentedOK 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.
Comment #166
jbrauer CreditAttribution: jbrauer commentedURG .. lock file ...
Comment #167
jbrauer CreditAttribution: jbrauer commentedand status... churn away testbot
Comment #168
dstolConfirming 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.
Comment #169
greg.harveyWill test #166 myself this week. =)
Comment #170
greg.harveyApplied 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. =)
Comment #171
jbrauer CreditAttribution: jbrauer commentedUpdating category to reflect that this fixes a major bug (i.e. all themes disabled)
Comment #172
Gábor HojtsyThanks, committed, pushed.
Comment #173
ELC CreditAttribution: ELC commentedI 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
The issue seems to be in system_theme_data() in the following condition:
It is always running the INSERT code instead of the update code because
.. 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
.. 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)?
Comment #174
xmacinfo@ELC: You should open a new issue (and reference here) instead of reopening this issue.
Comment #175
lort CreditAttribution: lort commentedI 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
Comment #176
jmuessig CreditAttribution: jmuessig commentedELC #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.
Comment #177
Dane Powell CreditAttribution: Dane Powell commentedI 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!)
Comment #178
greg.harveyI 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.
Comment #179
ELC CreditAttribution: ELC commented@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.