Setting a default theme in install profile causes WHACK errors [#585012]

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
#20
#18	585012-18.patch	1.85 KB	jpmckinney
#18
#9	profile-theme-585012-9.patch	1.95 KB	David_Rothstein
#9

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 22 September 2009 at 21:31

All 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 commented 22 September 2009 at 22:13

Priority:

Normal

» Critical

This 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

sun

German

Karlsruhe

CreditAttribution: sun commented 22 September 2009 at 22:13

subscribing

Comment #4

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 22 September 2009 at 22:44

Issue tags:

+DrupalWTF

A 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 commented 9 October 2009 at 07:49

this is the error that is actually occurring, and a fix for it that needs to be updated.

Comment #6

adrian CreditAttribution: adrian commented 9 October 2009 at 08:02

sorry : 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 commented 12 January 2010 at 17:54

There 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 16 January 2010 at 12:56

Comment #9

David_Rothstein CreditAttribution: David_Rothstein commented 16 January 2010 at 17:55

Priority:	Critical	» Normal
Status:	Active	» Needs review

File	Size
profile-theme-585012-9.patch	1.95 KB

I 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 commented 31 January 2010 at 23:39

Very 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 commented 22 February 2010 at 19:25

This 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 Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 1 March 2010 at 09:37

The 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

sun

German

Karlsruhe

CreditAttribution: sun commented 1 March 2010 at 14:36

Priority:

Normal

» Critical

Regressions are critical, AFAIK.

Comment #14

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 1 March 2010 at 16:48

Agreed with @sun.

Comment #15

yoroy CreditAttribution: yoroy commented 15 March 2010 at 00:00

Status:

Needs review

» Needs work

I *think* this is 'needs work' as per #11. What's the next step?

Comment #16

yoroy CreditAttribution: yoroy commented 4 April 2010 at 22:03

Status:	Needs work	» Needs review
Issue tags:	-DrupalWTF

#9: profile-theme-585012-9.patch queued for re-testing.

Comment #17

4 April 2010 at 22:20

Status:	Needs review	» Needs work
Issue tags:		+DrupalWTF

The last submitted patch, profile-theme-585012-9.patch, failed testing.

Comment #18

jpmckinney CreditAttribution: jpmckinney commented 6 April 2010 at 01:43

Status:

Needs work

» Needs review

File	Size
585012-18.patch	1.85 KB

Fix patch to patch install.core.inc, not install.php

Comment #19

6 April 2010 at 02:00

Status:

Needs review

» Needs work

The last submitted patch, 585012-18.patch, failed testing.

Comment #20

jpmckinney CreditAttribution: jpmckinney commented 6 April 2010 at 04:06

Status:

Needs work

» Needs review

File	Size
585012-20.patch	1.84 KB

Oops, forgot to set --no-prefix.

Comment #21

sun

German

Karlsruhe

CreditAttribution: sun commented 9 April 2010 at 22:59

Patch looks RTBC. Anyone able to confirm via manual testing?

Comment #22

yoroy CreditAttribution: yoroy commented 9 April 2010 at 23:04

Need instructions on how then :) Needs a custom install profile to test?

Comment #23

moshe weitzman CreditAttribution: moshe weitzman commented 10 April 2010 at 04:12

I 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 10 April 2010 at 12:42

@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 commented 10 April 2010 at 22:55

Priority:

Critical

» Normal

As 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 commented 11 April 2010 at 08:16

Well whats holding you back from making a patch :D?

Comment #27

David_Rothstein CreditAttribution: David_Rothstein commented 11 April 2010 at 12:04

Uh... the fact that I already did? :)

We just need to get the existing patch here committed.

Comment #28

Bojhan CreditAttribution: Bojhan commented 11 April 2010 at 12:25

Its failing tests.

Comment #29

David_Rothstein CreditAttribution: David_Rothstein commented 11 April 2010 at 13:38

No, 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:

Create an empty directory profiles/minimal/themes and move the entire themes/stark directory into there (so you get profiles/minimal/themes/stark).

Edit the profiles/minimal/minimal.install file and put this code at the very bottom of the minimal_install() function:

  theme_enable(array('stark', 'seven'));
  variable_set('theme_default', 'stark');
  variable_set('admin_theme', 'seven');

Then install Drupal with the minimal profile and check that with the patch, both Stark and Seven are listed as enabled on the admin/appearance page and working correctly (with Stark as main site theme, Seven as admin theme)...