One of the issues I've had with install profiles is the fact that packages (ie: themes/modules) stored within the profiles/$profile folder are often not available during install time, leading to lots of nasty hacks from within install profiles.

An example (in Drupal 6) would be that if you stored your theme in profiles/hostmaster/themes/eldir, the theme_list() would be generated by drupal_maintenance_theme before you could load the install_profile table, and you would not be able to set the theme to eldir, because after completing the system there would be no system table entry for your theme.

This issue also crops up in drush, when we want to provide a listing of all the commands that are active for the site. We are unable to find the commands until we have a full Drupal bootstrap.

The plugin manager would also need to be able to find out which sites are running a specific install profile that is going to be updated, without needing to connect to each individual site's database.

The reason this is a problem, is because this information needs to be available before bootstrap.

The attached patch adds an additional line to the default.settings.php file, which defines the $installed_profile global, from the installer. The global was already defined in the bootstrap.inc, but wasn't being used anywhere.

I was also able to remove some of the conditional code from module_listing, which merely existed as a workaround to the fact that the profile value needs to be available already.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Generally, this patch looks like it is trying to fix the wrong issue. While putting the installation profile in the settings.php file does make a lot of sense, none of the issue listed here should happen in the current architecture.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

We need this for all the reasons adrian mentions ... I don't really understand Damien's comment .

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

adrian’s picture

adrian’s picture

Status: Needs work » Needs review
FileSize
5.31 KB

here's an updated version of this patch.

adrian’s picture

FileSize
8.06 KB

Here is an updated version of the patch with an upgrade path.

adrian’s picture

Priority: Normal » Critical
Issue tags: +DrupalWTF
FileSize
8.64 KB

minor change. don't save the install profile after install anymore. Also some documentation about the fact that the value being written tot he config file has been validated.

i'm marking this critical because it is a blocker for #585012: Setting a default theme in install profile causes WHACK errors , which is critical and a DRUPALWTF

moshe weitzman’s picture

Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community

Back to rtbc ... when bot is up again, please wait for green.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This seems fairly straight-forward (and I'm guessing that testbot would be throwing up if it broke anything too severely..) but it no longer applies after the other stuff that was committed tonight.

It looks like 6.x upgraders just all get "standard" as their profile, is that correct? What are Open Atrium folks going to do to get around this issue?

adrian’s picture

there's an if there.

upgraders only get set to standard IF the current profile is default.

webchick’s picture

Oh. Then the upgrade path for Open Atrium is "open your settings.php file and add this line"? Or a variable_set() in the update path?

adrian’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.66 KB

here's a new patch.

adrian’s picture

webchick: at the moment open atrium is drupal 6 only.

and changing install profiles is tricky for _ANY_ drupal site, on _ANY_ drupal version.

the issue is especially problematic for drupal 6 install profiles, but this will help with it for d7.

When you have an install profile, all of the components of that profile will be stored in profiles/$profilename.
This means that you can't change the $profilename from within the update hooks in any of the .install files in your project, because they won't be found by the drupal_system_listing() function.

To change an install profile you need to modify the profile that will be loaded BEFORE the update.php process even starts.

If this information is not stored inside the settings.php, but in the database, it means that you will need to bootstrap the site
to a higher level, where it will be barely functional (ie: the themes and modules used for the site are not found because they are not present anymore, and can't be detected in the search paths).

The obvious answer to this is to use drush, but not everyone can use drush. Storing this in the settings.php makes a lot more sense.

Drupal core gets away with this _only_ because it's modules and update code aren't in the profile space but in modules/.
Packaged install profiles on Drupal.org enfore the use of the profile search path.

I still think it was a terrible idea to rename the profiles to begin with, but so be it. At least in drupal core it doesn't kill us.

Keep in mind, that this is all not even the primary reason or one of the primary reasons why this is a drupalwtf. The lack of this causes several problems in Drush, and in developing install profiles.

Status: Reviewed & tested by the community » Needs review
Issue tags: -DrupalWTF

Re-test of store_settings_php_4.diff from comment #12 was requested by adrian.

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

The last submitted patch, store_settings_php_4.diff, failed testing.

adrian’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.73 KB

here's a re-rolled version of the patch. too late for the alpha sadly =(

dww’s picture

Status: Reviewed & tested by the community » Needs work

Not having closely studied the issue (and related problems), this seems on the surface to be a good move. However, #16 has some (mostly cosmetic) problems:

A) Do we actually want to document $installed_profile in the default settings.php? No one should ever set that manually, right? If we do document it, we need to be more explicit about why an end-user cares about this setting, and under what circumstances it should ever change...

B) It seems slightly weird that update_fix_d7_requirements() always just appends the setting (without the comment) to the settings.php file. I guess there's no way to tell where in the settings.php file to put it since we're upgrading from a D6 site that doesn't define it, so appending \n and the line is perhaps the best we can do. But, it seems like that'd be a good place to actually have a comment explaining why this was added and again, if/why/how a site admin would ever want to change it.

C) Hrmm...

+  // The $_GET['profile'] here has been validated against a file_exists() call and filtered
+  // to remove possible exploits. 
+  // It will not allow an attacker to write malicious values into the config file.

It'd be nice if the spot where this validation and filtering happens is commented, too (I'm not sure off the top of my head where that happens). If we're now relying on that to prevent PHP code execution exploits, we better say so where we do it. Also, any reason this comment has each sentence on its own line? It should just wrap (to 80 chars) as a single inline block...

[begin pedantic nit picking] ;)

D)

-    // Include the default profile.
-    variable_set('install_profile', 'standard');
+    // Include the default profile
+    $installed_profile = 'standard';

Inline comments should end with period.

E)

+ * This value is stored in the settings.php file generated during installation.
+ *
+ * Because we need to have this information before the database connection is initialized,
+ * to correctly determine where to search for modules and themes, we can not use the variable
+ * system to store this value.
+ *
+ * If this function is called during the initial installation process, the global
+ * variable will be initialized to the profile requested by the user and then
+ * stored in the settings file for future use.

Very useful comment, but it doesn't wrap to 80 chars.

F) The $update_rewrite_profile variable has a misleading name. We're not rewriting anything. ;) We're just writing it. Maybe $update_write_profile or $update_record_profile would be better.

G) Typos and doesn't wrap:

+    // We modify the installed_profile global at, so that future calls to drupal_get_profile() will have return the correct value, 
+    // because the settings.php file has already been loaded at this point.

("global at", "will have return the")?

H) Typo:

+    // Remove the uneccesary variable.

p.s. Please don't set your own patches to RTBC. ;)

David_Rothstein’s picture

Reviewing this issue, I don't think I understand the motivation for this patch. Can you explain further?

For example:

i'm marking this critical because it is a blocker for #585012: Setting a default theme in install profile causes WHACK errors , which is critical and a DRUPALWTF

This patch might fix that bug as a side effect, but I don't believe it's the root cause. I've just posted a simple patch at #585012: Setting a default theme in install profile causes WHACK errors which fixes that independently.

This issue also crops up in drush, when we want to provide a listing of all the commands that are active for the site. We are unable to find the commands until we have a full Drupal bootstrap.

What is the reason Drush can't just bootstrap to DRUPAL_BOOTSTRAP_VARIABLES whenever it needs to know the install profile?

The plugin manager would also need to be able to find out which sites are running a specific install profile that is going to be updated, without needing to connect to each individual site's database.

Similar question.. I haven't been following the plugin/update manager that closely, but I don't understand why it would be bad for that to connect to the site's database?

***

In addition, this patch currently has one side effect which I think is not so good. Right now, it's possible in Drupal to install a site without ever having to make the settings.php file writable. As long as you put the database credentials in settings.php beforehand, you can have a system where you do not need to mess around with file permissions in order to get Drupal installed. At least with the current implementation, it seems like this patch would break that?

For example, I tried making settings.php not writable with this patch (and putting my DB credentials in there, but not filling in the $installed_profile) and the installer died with this error:

    *  Notice: Undefined property: stdClass::$uri in system_update_files_database() (line 2096 of /home/droth/web/drupal/modules/system/system.module).
    * Notice: Undefined property: stdClass::$name in system_update_files_database() (line 2097 of /home/droth/web/drupal/modules/system/system.module).
    * PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1: SELECT * FROM {menu_router} WHERE path IN () ORDER BY fit DESC LIMIT 0, 1; Array ( ) in menu_get_item() (line 414 of /home/droth/web/drupal/includes/menu.inc).

If I do fill in $installed_profile, it works a little better, but then there is no guarantee that the profile which was actually installed matches the value in settings.php, leading to a variety of bizarre bugs (sort of as hinted at by @dww above).

***

It does occur to me that allowing the profile to be stored in settings.php has a (potential) interesting application - it seems like you could have a distribution of Drupal that cleanly bypassed the install profile selection screen (currently the only part of the installer that you can't modify without hacking core)? But that's not what this patch is doing, and doesn't seem like we'd need to require it to be stored in settings.php in order to achieve that.

David_Rothstein’s picture

Also, this part of the patch won't work with command-line installations:

+  $settings['installed_profile'] = array(
+    'value' => $_GET['profile'],
+    'required' => TRUE
+  );

We can't take the profile directly from $_GET here, but rather would need to get it from $install_state['parameters']['profile'].

adrian’s picture

David :

the motivation of this patch is exactly the same as this patch -
#585012: Setting a default theme in install profile causes WHACK errors

We are in need of having access to the $profile variable earlier in the process than is currently possible.
But unlike the patch in that thread, which only handles the specific case of setting the theme during install time, this patch is about solving other cases I have discovered.

The other cases are especially triggered when using external tools to script Drupal, such as Drush. An example being, that Drush allows modules to extend it by placing a .drush.inc inside the directory. Drush also allows commands to run before Drupal has even been bootstrapped.

Drush - database rollback

A good example of this would be something like a backup / restore command for manipulating the database of the Drupal site. These commands would only need to bootstrap Drupal to the level where it has read the site configuration file to find the database details.

However, if you want to create an install profile that includes this module, Drush will be unable to find this module and load it, until you have already made the connection. This is not a problem when making the backup, but when doing the restore it can cause severe issues, as you will be loading up Drupal, and then swapping out it's database from underneath it.

The basic gist of that case is, that the functionality of drush extensions is compromised by causing them to be invisible to us until too late in the installation process.

Open atrium - changing install profiles

This very recently became a problem for us with open atrium, where we changed from using atrium_installer to 'open_atrium' as the install profile.

Because the install profile is in the database, the only way to modify it is to bootstrap Drupal to have a database connection, and then send a variable_set('install_profile'). Or , you can set the $GLOBALS['conf']['install_profile'] variable (which is what this patch wants to standardize).

Because all our modules are now stored in profiles/open_atrium/modules, update.php would not be able to run until the install profile variable had been changed _AND_ system_module_listing/theme_listing had been called to rebuild the system table.

Before that point , the modules containing the updates are completely invisible to the update process.

Our (drush based) solution was to code the install_profile into the settings.php, and to modify the drush updatedb command to regenerate the system table before figuring out which updates to run.

http://drupalcode.org/viewvc/drupal/contributions/profiles/openatrium/UP...

moshe weitzman’s picture

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

#16: store_settings_php_5.diff queued for re-testing.

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

The last submitted patch, store_settings_php_5.diff, failed testing.

David_Rothstein’s picture

#20 is a good explanation, thanks. The Open Atrium case seems like what you wound up doing would not be so different than what you would do here (they have to modify settings.php either way) - in other words, it works just as well if storing the profile in settings.php is optional rather than required? However, for the other use cases, I can see why requiring it would be helpful.

However, I think the use case I mentioned in #18 is important too (being able to install Drupal without ever making the settings.php file writable). So it seems like the best thing this patch could do would be to enable that also - i.e., if it finds that the global $installed_profile is already defined in settings.php, then it should always use that, (a) bypassing the initial profile selection screen in the installer, and (b) not trying to (re)write it to settings.php?

arianek’s picture

subscribe

tnightingale’s picture

subscribe

arianek’s picture

and by subscribe, we mean, this is making us crazy. is there things that we (thegreat and/or other affinity bridge crew) can do to help out with this issue?

arianek’s picture

just linking to the other issue that has a good description of what's been happening for us: #578144: Main theme lost basically the same as in this comment http://drupal.org/node/578144#comment-2982044 - the fix of setting the 'install_profile' variable in settings.php works *mostly* and in cases where that didn't do it, disabling update status temporarily stops this from happening. (but obviously that's a temporary solution).

webchick’s picture

Well. A good first step would be making this patch pass tests. :)

arianek’s picture

hmmm. good point. i am not sure whether we should even try to improve on one of adrian's patches, but we may as well take a look!

adrian’s picture

Status: Needs work » Needs review
FileSize
6.89 KB

here is an update version of the patch for HEAD.

this has been simplified after feedback from angie, in that it doesnt handle the rewrite of settings.php again after the
db changes have been made, because we dont support alpha to alpha upgrades.

anarcat’s picture

just a quick note there will be a conflict between this patch and #323477: Increase simpletest speed by running on a simplified profile which should be fairly easy to resolve, but that is to keep in mind anyways.

anarcat’s picture

small reroll of the patch to write the settings.php only once. patch otherwise looks syntactically correct to me, but i haven't tested it yet.

adrian’s picture

we're appending , so this is the previous patch again, which i think is cleaner.

anarcat’s picture

Please ignore that latter reroll, it's irrelevant: we *append* and not rewrite the whole thing so there's no need to optimize here.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I read this over too and it fixes a WTF for sure. Wait for green before commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 545452_profile_in_settingsphp.diff, failed testing.

aspilicious’s picture

David_Rothstein’s picture

I also don't see any indication in the patch file that the middle part of comment #18 was addressed (unless I missed it?). We need to deal with the situation where someone has an unwritable settings file but has already put their DB credentials in it. And specifically I think we need to preserve their ability to do that.

adrian’s picture

Status: Needs work » Needs review
FileSize
7 KB

I spoke to webchick about this, and the different db credentials were added as part of the new db api, which is part of the alpha releases. We specifically do not support the head to head upgrade path.

The patch is also far uglier when you need to take that into account.

Attached is the latest version of the patch against the latest head, including the test case modification of making the simpletest profile configurable.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

wait for green before commit

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 545452_profile_in_settingsphp_2.diff, failed testing.

David_Rothstein’s picture

adrian, not sure what you mean? I'm not talking about the upgrade path - I'm talking about fresh D7 installations.

adrian’s picture

david: we store the profile in the same step we write the database settings into the settings.php

if you can't write to the file , you can't install at all.

adrian’s picture

david:

ok. thinking about it , i get what you mean.

the issue is this

+  if (isset($install_profile)) {
+    $profile = $install_profile;

When you edit the settings.php by hand, and change only the db. the 'install_profile' will be ''.
what we'd need is 'isset && !empty'

would that satisfy your requirements ?

David_Rothstein’s picture

Nope, it is currently perfectly possible to install Drupal without ever making the settings.php file writable.

(I'll post more detailed steps to reproduce in an hour or two - don't have time right now.)

David_Rothstein’s picture

Crosspost :) I'll look at that suggestion later too.

anarcat’s picture

Status: Needs work » Needs review

Right: you fill in the db details and drupal picks them up. What that means is that you need to fill in the install profile too. Doesn't this just work?

adrian’s picture

Here's a new patch using just '!empty($install_profile)' , which should work fine (instead of isset && !empty)

adrian’s picture

fwiw. what this means is that if you leave the $install_profile = ''; and dont edit it.

it will still default to 'standard'.

even though new install and upgrades will populate it with 'standard' correctly.

I was confused because aegir already generates the settings.php and doesnt require write access to the web server,
but we also always populate the variable anyway.

Status: Needs review » Needs work

The last submitted patch, 545452_profile_in_settingsphp_3.diff, failed testing.

adrian’s picture

Status: Needs work » Needs review
FileSize
7.01 KB

ok. here's another patch.

setting the profile to 'standard' by default.

this was causing system_requirements to cause an exception during testing.

Status: Needs review » Needs work

The last submitted patch, 545452_profile_in_settingsphp_4.diff, failed testing.

David_Rothstein’s picture

OK, so to clarify the use case where the problems are (just so everyone is on the same page) it's this:

  1. Start with a fresh Drupal 7 codebase.
  2. Before installing Drupal, edit the settings.php file to put your database credentials in it (you never need to make the file writable by the web server if you don't want to).
  3. Visit install.php and install.

With the previous patches the main bug there was that you could wind up with a Drupal site installed that never had an install profile recorded (it would stay empty in the settings file).

Adrian's latest patch solves that part by having 'standard' be the default, but there's still a remaining bug: If I follow the above steps but then choose the Minimal profile on the first screen of the installer, there is a mismatch between what I selected and what's in my settings.php file, and Drupal explodes. (The immediate error is Fatal error: Call to undefined function rdf_mapping_save() but obviously that's just a symptom.)

One way to solve that might be:

  1. Go back to having $install_profile default to an empty string in settings.php.
  2. On the first page of the installer, do a quick load of settings.php and check if $install_profile is set to something valid. If so, automatically pre-select that profile and skip the first screen of the installer.
  3. On the Verify Requirements stage of the installer, again check if there is a valid $install_profile in settings.php, and if not, require that settings.php be writable by the webserver.

Not sure if there's a better way to do it, but that would probably work, and would allow people to independently set $install_profile or $databases in their settings.php file depending on their use case.

Otherwise this all seems pretty good.

moshe weitzman’s picture

Anyone willing to push this along according to David's plan?

adrian’s picture

Status: Needs work » Needs review

Re-roll of this patch for testing.

adrian’s picture

FileSize
7.06 KB

aaand the file.

adrian’s picture

FileSize
10.24 KB

Here is the patch, with david's input added.

The profile is empty by default and the step is skipped if it has already been set in the config.

David_Rothstein’s picture

FileSize
12.7 KB

Rerolled to remove system_update_7049() entirely rather than leaving it as an empty stub function (as per #909272: Remove stub update functions and renumber the remaining ones).

I'll try to do a real review of this at some point tomorrow.

David_Rothstein’s picture

Status: Needs review » Needs work
FileSize
12.69 KB

OK, I reviewed this and rerolled with some minor code style cleanup (no substantial changes, although one of the code style issues was an extra space in front of $install_profile in default.settings.php which apparently confused drupal_rewrite_settings() and made it always append $install_profile to the bottom of settings.php rather than overwriting the existing line; that bug is now fixed in the attached).

I think this is almost ready (whether for D7 or D8 I'm not sure at this point) but there are still a couple issues, one somewhat major. With these things fixed it should be RTBC.

  1. It seems like my third point in #53 is still not addressed, meaning that the "Verify requirements" stage of the installer does not ensure that settings.php is writable in all cases where it needs to be. Specifically, if the database credentials are present but $install_profile is not present (or not valid) we still need to make sure settings.php is writable.

    The current code uses $install_state['settings_verified'] to decide whether to check settings.php writability, which only checks for a valid database connection. I think fixing this might be as simple as changing the if() statement in install_check_requirements() to also check drupal_get_profile() and see if it's valid or not, but not positive.

  2.  function drupal_get_profile() {
    -  global $install_state;
    +  global $install_profile;
     
    -  if (isset($install_state['parameters']['profile'])) {
    -    $profile = $install_state['parameters']['profile'];
    +  if (!empty($install_profile)) {
    +    $profile = $install_profile;
       }
       else {
         $profile = variable_get('install_profile', 'standard');
    

    Why is this function still using variable_get('install_profile')? Unless it's needed by update.php during the D6-D7 update (which if that's the case, should be documented with a @todo to remove in Drupal 8), it seems to me like this function could just return the global $install_profile always, or NULL if it's not set?

  3. @@ -239,7 +239,10 @@ function install_begin_request(&$install
       drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);
     
       // This must go after drupal_bootstrap(), which unsets globals!
    -  global $conf;
    +  global $conf, $install_profile;
    +  if (!empty($install_profile)) {
    +    $install_state['parameters']['profile'] = $install_profile;
    +  }
    

    I found this a little confusing at first and think it could benefit from a code comment stating that even if the profile was already requested via the URL, we are explicitly overriding it with whatever value was set in settings.php.

  4. -  global $update_rewrite_settings, $db_url, $db_prefix;
    +  global $update_rewrite_settings, $db_url, $db_prefix, $install_profile;
       if (!empty($update_rewrite_settings)) {
         $databases = update_parse_db_url($db_url, $db_prefix);
         $salt = drupal_hash_base64(drupal_random_bytes(55));
         file_put_contents(conf_path() . '/settings.php', "\n" . '$databases = ' . var_export($databases, TRUE) . ";\n\$drupal_hash_salt = '$salt';", FILE_APPEND);
    +
    +    // The install profile was renamed from 'default' to 'standard'.
    +    // This change was originally handled in system_update_7049(), 
    +    // but because we no longer use a variable to store the profile name,
    +    // it is no longer necessary.
    +    $profile = variable_get('install_profile', 'standard');
    +    if ($profile == 'default') {
    +      $profile = 'standard';
    +    }
    

    The $update_rewrite_settings variable used here is set in update_prepare_d7_bootstrap() based only on whether settings.php already has $databases in the correct format. So it seems update_prepare_d7_bootstrap() should be modified to check for $install_profile too?

    This is similar to my first point, essentially, only concerning update.php rather than install.php (the bug is less likely to occur during updates I guess, but still should be an easy fix).

  5. @@ -591,7 +591,7 @@ class DrupalUnitTestCase extends DrupalT
        * method.
        */
       protected function setUp() {
    -    global $conf;
    +    global $conf, $install_profile;
    

    For DrupalUnitTestCase, does this change actually do anything? Doesn't look to me like it is needed.

moshe weitzman’s picture

Version: 7.x-dev » 8.x-dev

sigh

olli’s picture

Status: Needs work » Closed (duplicate)
somar-assaad’s picture

Version: 8.x-dev » 7.23

hi
i am new in openatrium
i install open atrium ,when i get to the site i get blank page,
how i make apache recognice to my atrium installation??

David_Rothstein’s picture

Version: 7.23 » 8.x-dev
somar-assaad’s picture

hi,
i need help please, i tried to install openatrium-7.x-2.0-beta4 and i have this message error during the install

<>