If you have an install profile that ships with a custom theme (/drupal7/profiles/awesomeprofile/themes/dingus), and you want to enable that theme by default in the install profile, I would expect the following would have worked just perfectly, since we do the same thing for the admin theme in our default install profile:
...
// Enable our awesome profile theme 'dingus'.
db_update('system')
->fields(array('status' => 1))
->condition('type', 'theme')
->condition('name', 'dingus')
->execute();
variable_set('theme_default', 'dingus');
// This chunk is the same as the default.install:
// Enable the admin theme.
db_update('system')
->fields(array('status' => 1))
->condition('type', 'theme')
->condition('name', 'seven')
->execute();
variable_set('admin_theme', 'seven');
...
But once the install finishes (with no errors) and we hit the front page of the new Drupal site, we get unleashed with this:
* : in (line of ).
* : in (line of ).
* : in (line of ).
* : in (line of ).
* : in (line of ).
* : in (line of ).
* : in (line of ).
* Warning: Invalid argument supplied for foreach() in system_region_list() (line 2115 of /home/dave/Projects/www/drupal7dev/modules/system/system.module).
* Notice: Undefined index: dingus in system_region_list() (line 2121 of /home/dave/Projects/www/drupal7dev/modules/system/system.module).
* Warning: array_keys(): The first argument should be an array in block_page_build() (line 199 of /home/dave/Projects/www/drupal7dev/modules/block/block.module).
* Warning: Invalid argument supplied for foreach() in block_page_build() (line 199 of /home/dave/Projects/www/drupal7dev/modules/block/block.module).
* Notice: Undefined index: dingus in theme_get_setting() (line 1206 of /home/dave/Projects/www/drupal7dev/includes/theme.inc).
* Notice: Trying to get property of non-object in theme_get_setting() (line 1210 of /home/dave/Projects/www/drupal7dev/includes/theme.inc).
* Notice: Trying to get property of non-object in theme_get_setting() (line 1219 of /home/dave/Projects/www/drupal7dev/includes/theme.inc).
....?
Comment | File | Size | Author |
---|---|---|---|
#20 | 585012-20.patch | 1.84 KB | jpmckinney |
#18 | 585012-18.patch | 1.85 KB | jpmckinney |
#9 | profile-theme-585012-9.patch | 1.95 KB | David_Rothstein |
Comments
Comment #1
Dave ReidAll I've been able to debug with this is that when I visit the admin/appearance page, the theme works normally, but the checkbox for that theme is also unchecked (disabled). Sure enough, when I go to the {system} table the status value for dingus is 0 (disabled). It works fine for enabling the 'seven' theme...why not for anything else?
Comment #2
Bojhan CreditAttribution: Bojhan commentedThis is very critical to me, I wish to run a usability test this Saturday.
http://dl.getdropbox.com/u/29125/D7UXBeginner.rar - The Profile
Comment #3
sunsubscribing
Comment #4
Dave ReidA DrupalWTF for sure here. I copied the exact same themes/dingus folder over to the default profile and I added the exact same lines to enable the dingus theme in default.install. And no errors after the install. WTF.
Comment #5
adrian CreditAttribution: adrian commentedthis is the error that is actually occurring, and a fix for it that needs to be updated.
Comment #6
adrian CreditAttribution: adrian commentedsorry : http://drupal.org/node/545452
a simple explanation is the following
in install.php, system_maintenance_theme is called, which populates the theme list static.
it does this however BEFORE the profile is loaded or configured and before the database connection is made.
meaning the only place it can look for the profile being used is settings.php
Comment #7
adrian CreditAttribution: adrian commentedThere is a working and complete patch that will resolve this issue at #545452: Store install profile in the generated settings.php file
Comment #8
hass CreditAttribution: hass commented+
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedI think there is a relatively simple explanation for this bug.
During interactive installations, we get the name of the install profile from the URL (and put it into a variable). However, we don't populate this variable early enough in the page request, so some functions which try to access it very early on will find that it's empty (and fall back on assuming the standard install profile in that case).
So the correct fix seems to be to move the code which populates variables from the URL to an earlier place. The attached patch does that, and fixes the issue for me.
I'm also downgrading this from "critical" because in my testing of current HEAD, I didn't see any errors on the screen or any failure of the theme to actually work. I do however see the incorrect information on the admin/appearance page and in the database, as reported in #1. Please set back to critical if you see something else. (I think what happened is that since this bug was originally reported, we've added a fair amount of cache clearing to the installer, which helps a bit here. But the attached patch is needed to completely fix the bug.)
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedVery similar (and earlier) issue for Drupal 6 at #330297: Themes in profiles directory not discovered during install ... but it seems the bug is much more serious in Drupal 6 than Drupal 7. So for now, I'm just cross-linking them.
Comment #11
adrian CreditAttribution: adrian commentedThis is not just during install. it happens during all initializations ..
and i don't think install_state should even be used outside the install process. What about during upgrades ?
Comment #12
Gábor HojtsyThe Drupal 6 counterpart of this issue was committed at #330297: Themes in profiles directory not discovered during install, based on the general opinion that we should not hold down the current D6 install profiles push by a bug not actively fixed in D7. If this bug is not fix, we get a regression in Drupal 7.
Comment #13
sunRegressions are critical, AFAIK.
Comment #14
Gábor HojtsyAgreed with @sun.
Comment #15
yoroy CreditAttribution: yoroy commentedI *think* this is 'needs work' as per #11. What's the next step?
Comment #16
yoroy CreditAttribution: yoroy commented#9: profile-theme-585012-9.patch queued for re-testing.
Comment #18
jpmckinney CreditAttribution: jpmckinney commentedFix patch to patch install.core.inc, not install.php
Comment #20
jpmckinney CreditAttribution: jpmckinney commentedOops, forgot to set --no-prefix.
Comment #21
sunPatch looks RTBC. Anyone able to confirm via manual testing?
Comment #22
yoroy CreditAttribution: yoroy commentedNeed instructions on how then :) Needs a custom install profile to test?
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedI really think "regressions are critical" makes little sense. Bugs should be evaluated on their own merit, not on their longevity (we had this in d6 so it can't be critical) or presence in prior version (this is a new bug so its critical). I'd like for another person to agree and then downgrade this bug.
Comment #24
Bojhan CreditAttribution: Bojhan commented@moshe I am not sure, why wouldn't this be a critical bug? Clearly install profiles, should be able to ship with themes. That it was an oversight on Drupal 6, doesn't mean its not critical.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedAs per #9, this wasn't thought to be critical anymore (before it turned into a regression from D6), so based on that it shouldn't be critical now.
However, why don't we just fix it? :) The patch is very straightforward, and would be a good idea even if it didn't fix this particular bug (it certainly makes sense to fully populate the variable as early as possible, before anyone tries to use it). I haven't tested it now, but when I tested it a few months ago, it definitely fixed the bug then.
Would be nice to be able to write tests for this, but don't think that's possible without #630446: Allow SimpleTest to test the non-interactive installer...
Comment #26
Bojhan CreditAttribution: Bojhan commentedWell whats holding you back from making a patch :D?
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedUh... the fact that I already did? :)
We just need to get the existing patch here committed.
Comment #28
Bojhan CreditAttribution: Bojhan commentedIts failing tests.
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedNo, the reroll in #20 is OK, right?
BTW, to test this without bothering to create new themes or profiles, I think the simplest way is to do something like:
profiles/minimal/themes
and move the entirethemes/stark
directory into there (so you getprofiles/minimal/themes/stark
).profiles/minimal/minimal.install
file and put this code at the very bottom of the minimal_install() function:Comment #30
Bojhan CreditAttribution: Bojhan commentedAhh, wierd it didn't work for me - does now. Well RTBC then?
Comment #31
klausiStill applies.
Comment #32
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #34
kenorb CreditAttribution: kenorb commentedThe same problem for 6.x: #957282: Errors on update.php