| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | update.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs tests |
Issue Summary
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:
<?php
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| wtf.admin_.is_.fast_.patch | 873 bytes | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
Good catch!
We changed the package name of Core modules in #293223: Roll back "Hide required core modules" but failed to update that function.
#2
I 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.
#3
This will also need to be backported to D6.
#4
I think 6 is good, and it's quite difficult to test this.
#5
Oh you're right, I completely failed to read #1.
#6
i'll work up a test in the next couple of days unless someone beats me to it.
#7
"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.
#8
Trying 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".
#9
looks good to me, i like the stpos change, i think its a bit tighter than what i added.
#10
Actually 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.#11
Fixes the extra blank line in stark.info.
#12
do we need a doc update somewhere so contrib themes know about this? or is that going to be just an upgrade entry?
#13
It's only going to apply to our core themes, so we don't need to mention it anywhere.
#14
Love 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!#15
That looks like a nice solution.
Committed to HEAD. Thanks!
#16
Always good to remove a hack. Committed to CVS HEAD. Thanks.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.
#18
This 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.
#19
Didn'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
#20
Automatically closed -- issue fixed for 2 weeks with no activity.