i blame devel module for making me wonder 'WTF is causing admin/just/about/anything to run about 50 more queries than any non-admin page?'.
for core packages, we don't define 'project' in our info files:
justin@ubuntu:~/code/drupal/7/clean$ grep project modules/*/*.info
we do define 'package' as 'Core' in our info files:
justin@ubuntu:~/code/drupal/7/clean$ grep package modules/*/*.info modules/aggregator/aggregator.info:package = Core ... you get the idea ... modules/user/user.info:package = Core
unfortunately, we have this in update_get_project_name:
function update_get_project_name($file) {
$project_name = '';
if (isset($file->info['project'])) {
$project_name = $file->info['project'];
}
elseif (isset($file->info['package']) && (strpos($file->info['package'], 'Core -') !== FALSE)) {
$project_name = 'drupal';
}
elseif (in_array($file->name, array('bluemarine', 'chameleon', 'garland', 'marvin', 'minnelli', 'pushbutton'))) {
// Unfortunately, there's no way to tell if a theme is part of core,
// so we must hard-code a list here.
$project_name = 'drupal';
}
return $project_name;
}
as 'Core - ' doesn't match 'Core', the caching of this expensive data always fails. maybe it never worked, i can't be bothered looking.
so, the 2-characters-to-save-47-queries patch is attached.
this patch may be a bit disconcerting, because it causes many admin pages to feel, well, quick, which is distinctly un-drupal like.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 334238-update-core-package-D7.patch | 3.47 KB | dave reid |
| #10 | 334238-update-core-package-D7.patch | 3.33 KB | dave reid |
| #8 | 334238-update-core-package-D7.patch | 1.03 KB | dave reid |
| wtf.admin_.is_.fast_.patch | 873 bytes | Anonymous (not verified) |
Comments
Comment #1
damien tournoud commentedGood catch!
We changed the package name of Core modules in #293223: Roll back "Hide required core modules" but failed to update that function.
Comment #2
dries commentedI committed this to CVS HEAD but ideally there would have been a test for this. Marking this 'code needs work' so we can follow-up with a test.
Comment #3
catchThis will also need to be backported to D6.
Comment #4
damien tournoud commentedI think 6 is good, and it's quite difficult to test this.
Comment #5
catchOh you're right, I completely failed to read #1.
Comment #6
Anonymous (not verified) commentedi'll work up a test in the next couple of days unless someone beats me to it.
Comment #7
dww"update system" == update.php + DB updates.
"update.module" == update status in core.
Just found this while reading through "update system" issues, looking for misclassified ones. Looks like this was only a D7 bug that's already fixed, so all that remains is a test.
Comment #8
dave reidTrying to rack my brain coming up with tests for this, but I can't figure out a reliable way to do it.
Also discovered that we're using the Core - fields package for field modules that are required, so they never show up in the modules page. Also, this was going to exclude seven.theme. Changed the strpos to look if the package *begins* with "Core".
Comment #9
Anonymous (not verified) commentedlooks good to me, i like the stpos change, i think its a bit tighter than what i added.
Comment #10
dave reidActually here's an even better plan. Let's stop hardcoding the theme inclusions that no one can ever find when we add themes, and just add
package = Coreto our core themes' .info files.Comment #11
dave reidFixes the extra blank line in stark.info.
Comment #12
Anonymous (not verified) commenteddo we need a doc update somewhere so contrib themes know about this? or is that going to be just an upgrade entry?
Comment #13
dave reidIt's only going to apply to our core themes, so we don't need to mention it anywhere.
Comment #14
dwwLove it. Defining
Package = Corefor core themes is a fine solution to this crappy hack. It will prevent bugs like #490562: Hard-coded list of core themes for update module is now stale in the future. There was another issue where we needed to know if a theme was part of core or not, but I can't find it right now. Drat. Anyway, still not sure about a test, but this is an improvement nonetheless. We can always add more tests later, but let's get this in. Thanks!Comment #15
webchickThat looks like a nice solution.
Committed to HEAD. Thanks!
Comment #16
dries commentedAlways good to remove a hack. Committed to CVS HEAD. Thanks.
Comment #18
jacineThis needs docs. Wasted a good 1/2 hour googling and looking through commit logs trying to figure this out what on earth this line was doing in core themes, what it does if anything, and if it's needed in other themes. On it.
Comment #19
jacineDidn't mean to remove the "needs tests" tag. Adding it back.
I've added docs to the theming update page:
http://drupal.org/update/theme/6/7#package-core