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.
Comment | File | Size | Author |
---|---|---|---|
#59 | profile_settings_4.patch | 12.69 KB | David_Rothstein |
#58 | profile_settings_3.patch | 12.7 KB | David_Rothstein |
#57 | profile_settings_2.patch | 10.24 KB | adrian |
#56 | profile_settings.patch | 7.06 KB | adrian |
#51 | 545452_profile_in_settingsphp_4.diff | 7.01 KB | adrian |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedGenerally, 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.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedWe need this for all the reasons adrian mentions ... I don't really understand Damien's comment .
Comment #4
adrian CreditAttribution: adrian commentedSo yeah, this is still causing problems in d7
#585012: Setting a default theme in install profile causes WHACK errors
Comment #5
adrian CreditAttribution: adrian commentedhere's an updated version of this patch.
Comment #6
adrian CreditAttribution: adrian commentedHere is an updated version of the patch with an upgrade path.
Comment #7
adrian CreditAttribution: adrian commentedminor 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
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedBack to rtbc ... when bot is up again, please wait for green.
Comment #9
webchickThis 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?
Comment #10
adrian CreditAttribution: adrian commentedthere's an if there.
upgraders only get set to standard IF the current profile is default.
Comment #11
webchickOh. 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?
Comment #12
adrian CreditAttribution: adrian commentedhere's a new patch.
Comment #13
adrian CreditAttribution: adrian commentedwebchick: 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.
Comment #16
adrian CreditAttribution: adrian commentedhere's a re-rolled version of the patch. too late for the alpha sadly =(
Comment #17
dwwNot 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...
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)
Inline comments should end with period.
E)
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:
("global at", "will have return the")?
H) Typo:
p.s. Please don't set your own patches to RTBC. ;)
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedReviewing this issue, I don't think I understand the motivation for this patch. Can you explain further?
For example:
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.
What is the reason Drush can't just bootstrap to DRUPAL_BOOTSTRAP_VARIABLES whenever it needs to know the install profile?
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:
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.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, this part of the patch won't work with command-line installations:
We can't take the profile directly from $_GET here, but rather would need to get it from $install_state['parameters']['profile'].
Comment #20
adrian CreditAttribution: adrian commentedDavid :
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...
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commented#16: store_settings_php_5.diff queued for re-testing.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commented#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?
Comment #24
arianek CreditAttribution: arianek commentedsubscribe
Comment #25
tnightingale CreditAttribution: tnightingale commentedsubscribe
Comment #26
arianek CreditAttribution: arianek commentedand 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?
Comment #27
arianek CreditAttribution: arianek commentedjust 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).
Comment #28
webchickWell. A good first step would be making this patch pass tests. :)
Comment #29
arianek CreditAttribution: arianek commentedhmmm. 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!
Comment #30
adrian CreditAttribution: adrian commentedhere 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.
Comment #31
anarcat CreditAttribution: anarcat commentedjust 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.
Comment #32
anarcat CreditAttribution: anarcat commentedsmall 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.
Comment #33
adrian CreditAttribution: adrian commentedwe're appending , so this is the previous patch again, which i think is cleaner.
Comment #34
anarcat CreditAttribution: anarcat commentedPlease ignore that latter reroll, it's irrelevant: we *append* and not rewrite the whole thing so there's no need to optimize here.
Comment #35
moshe weitzman CreditAttribution: moshe weitzman commentedI read this over too and it fixes a WTF for sure. Wait for green before commit.
Comment #37
aspilicious CreditAttribution: aspilicious commentedconflict cause #323477: Increase simpletest speed by running on a simplified profile is in.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #39
adrian CreditAttribution: adrian commentedI 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.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedwait for green before commit
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedadrian, not sure what you mean? I'm not talking about the upgrade path - I'm talking about fresh D7 installations.
Comment #43
adrian CreditAttribution: adrian commenteddavid: 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.
Comment #44
adrian CreditAttribution: adrian commenteddavid:
ok. thinking about it , i get what you mean.
the issue is this
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 ?
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedNope, 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.)
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedCrosspost :) I'll look at that suggestion later too.
Comment #47
anarcat CreditAttribution: anarcat commentedRight: 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?
Comment #48
adrian CreditAttribution: adrian commentedHere's a new patch using just '!empty($install_profile)' , which should work fine (instead of isset && !empty)
Comment #49
adrian CreditAttribution: adrian commentedfwiw. 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.
Comment #51
adrian CreditAttribution: adrian commentedok. here's another patch.
setting the profile to 'standard' by default.
this was causing system_requirements to cause an exception during testing.
Comment #53
David_Rothstein CreditAttribution: David_Rothstein commentedOK, so to clarify the use case where the problems are (just so everyone is on the same page) it's this:
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:
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.
Comment #54
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone willing to push this along according to David's plan?
Comment #55
adrian CreditAttribution: adrian commentedRe-roll of this patch for testing.
Comment #56
adrian CreditAttribution: adrian commentedaaand the file.
Comment #57
adrian CreditAttribution: adrian commentedHere 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.
Comment #58
David_Rothstein CreditAttribution: David_Rothstein commentedRerolled 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.
Comment #59
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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.
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.
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?
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.
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).
For DrupalUnitTestCase, does this change actually do anything? Doesn't look to me like it is needed.
Comment #60
moshe weitzman CreditAttribution: moshe weitzman commentedsigh
Comment #61
olli CreditAttribution: olli commented#1827112: Convert 'install_profile' variable to Settings
Comment #62
somar-assaad CreditAttribution: somar-assaad commentedhi
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??
Comment #63
David_Rothstein CreditAttribution: David_Rothstein commentedComment #64
somar-assaad CreditAttribution: somar-assaad commentedhi,
i need help please, i tried to install openatrium-7.x-2.0-beta4 and i have this message error during the install
<>