Remove fake form alter hook [#656470]

Profiles are not being loaded as modules during install. Only system and user modules are being loaded as modules. This means that hooks, like hook_form_alter, in profiles don't fire during install.

For example, this hook in default.profile doesn't do anything.

function default_form_alter(&$form, $form_state, $form_id) {
  if ($form_id == 'install_configure') {
    // Set default for site name field.
    $form['site_information']['site_name']['#default_value'] = $_SERVER['SERVER_NAME'];
  }
}

Comment	File	Size	Author
#7	drupal-profile-load-656470-7.patch	2.45 KB	David_Rothstein
#7
#5	drupal-profile-load-656470-5.patch	1.12 KB	mfer
#5
#3	drupal-profile-load-656470-3.patch	3.43 KB	mfer
#3
#1	Screenshot.png	71.15 KB	David_Rothstein

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

David_Rothstein CreditAttribution: David_Rothstein commented 10 December 2009 at 21:01

Status:

Active

» Postponed (maintainer needs more info)

File	Size
Screenshot.png	71.15 KB

Works fine for me. See the attached screenshot. I promise that I did not type the word "localhost" in there - it appeared on its own :)

Also, the hook_form_alter() that happens during install isn't a real hook; it's just named like one. I assume that if you want a profile to implement an actual hook (whether during install or otherwise), that hook would need to live in a .module file or else in a file that you explicitly load?

Comment #2

mfer CreditAttribution: mfer commented 10 December 2009 at 21:29

Status:

Postponed (maintainer needs more info)

» Active

The profile a site is created from runs on every page load with the other modules. It can contain any module hook. If you setup a site with a profile you no longer need a separate module for the profile alongside it. Checkout the system table and you'll see the profiles are listed as modules and are listed in module_implements, etc.

You are right that during install this form can have a special form alter call. Here is the code:

// Allow the profile to alter this form. $form_state isn't available
// here, but to conform to the hook_form_alter() signature, we pass
// an empty array.
$hook_form_alter = $install_state['parameters']['profile'] . '_form_alter';
if (function_exists($hook_form_alter)) {
  $hook_form_alter($form, array(), 'install_configure');
}

But, the real hook_form_alter is also being used because a full FAPI is how all the forms are functioning. So, the function above is there and hook_form_alter is being called but only system and user modules are loaded and called with it.

If the profile were loaded (not just included) like the user and system modules were the special case above could be removed and profiles could use hook_form_FORM_ID_alter, too. It seems bad to do it this way.

Plus, only special case forms can be altered this way. I didn't notice it was this way because i was trying to alter a form in a different step which didn't have the special case.

Comment #3

mfer CreditAttribution: mfer commented 11 December 2009 at 03:36

Assigned:	Unassigned	» mfer
Status:	Active	» Needs review

File	Size
drupal-profile-load-656470-3.patch	3.43 KB

The attached patch loads (not just includes) the profile after the system and user modules. That means the standard hook_form_alter and hook_form_FORM_ID_alter calls will work and that the profile and form_alter other forms (such as the database form). The special case/hard coded profile_form_alter is removed.

Note, I didn't update the expert.profile because it was already using hook_form_FORM_ID_alter.

Comment #4

David_Rothstein CreditAttribution: David_Rothstein commented 11 December 2009 at 05:49

I'm a bit uneasy about this, because the way things currently work, a profile is only "installed" (i.e., treated as though it were a full-fledged module) after all the modules it depends on (i.e., the list of dependencies[] in its .info file) are themselves installed. This makes sense to me.

With the above patch, won't it be the case that when implementing certain hooks, my profile can no longer assume that its dependencies are even met? So basically, I have to tiptoe around things just to avoid crashing the installer, even though in theory those dependencies are met at any other time.

For altering things that display very, very early in the installation process, we have http://api.drupal.org/api/function/hook_install_tasks_alter/7 which was created for just this purpose. It's a bit more of a brute force tool than you need if you're just trying to change a single form field or something, but it should work fine as an alternative to this patch.

***

Also:

The special case/hard coded profile_form_alter is removed. ... Note, I didn't update the expert.profile because it was already using hook_form_FORM_ID_alter.

Indeed! I believe it's correct that even regardless of the rest of the patch, the pseudo-hook for this particular form can simply be removed. Because at that point, the profile should have been fully installed anyway and ready to treat as a module. We should definitely do that.

The profile a site is created from runs on every page load with the other modules. It can contain any module hook. If you setup a site with a profile you no longer need a separate module for the profile alongside it.

Right, I understood that - what I was getting at was the (seemingly reasonable) possibility that if so, profile hooks would need to be implemented in a myprofile.module file rather than a myprofile.profile file. Looking more closely, I now see why this is not the case - this part of the code is basically put together by chewing gum and relies on myprofile.profile getting inserted into the database at the correct times :)

What this probably means, by the way, is that this part of your patch, although it certainly looks right, doesn't actually do anything:

+  if (!empty($install_state['parameters']['profile'])) {
+    drupal_load('module', $install_state['parameters']['profile']);
+  }

Because if you look at what drupal_load() does in situations where the database is not yet active, it does not do anything that would result in myprofile.profile ever getting found. Instead I think the patch is simply relying on the fact that the .profile file is already included in a separate part of install.php anyway...

Comment #5

mfer CreditAttribution: mfer commented 11 December 2009 at 13:20

File	Size
drupal-profile-load-656470-5.patch	1.12 KB

@David_Rothstein It seems that just before this settings form it does do a complete bootstrap (via install_bootstrap_full). Before that it only does the lowest level bootstrap (not even hook_boot fires).

Can you describe a case where you'd have to tip-toe around? I'm starting to lean towards what you are saying with the dependancy issue and instead use http://api.drupal.org/api/function/hook_install_tasks_alter/7.

No matter what, we need to have this special case removed. A full bootstrap has happened at this point so to the real hook_form_alter hooks will work. The attached patch removed this special case.

Comment #6

moshe weitzman CreditAttribution: moshe weitzman commented 13 December 2009 at 19:03

Status:

Needs review

» Reviewed & tested by the community

looks reasonable to me.

Comment #7

David_Rothstein CreditAttribution: David_Rothstein commented 13 December 2009 at 20:26

File	Size
drupal-profile-load-656470-7.patch	2.45 KB

Agreed, this looks good. Technically it's a (minor) API change, but only because the old code was inexplicably passing along the wrong form ID.

I'm attaching a version that fixes the code comments since it is no longer true that $form_state is ever passed as an empty array.... still RTBC.

Re #5, well, I was thinking about any hook, e.g. hook_init(), that might be triggered during those full bootstraps while the profile modules are still being installed. However, actually trying it out I see that's not a problem, I guess because the full bootstrap overrides the fixed module list. So the only actual concern would be for something like hook_form_alter() itself, that can get called before a full bootstrap ever happens. It means that if I try to depend on my profile's dependencies existing in that hook, and use one of their API functions, it will break the installer even though it should be able to work fine and will work perfectly well everywhere else. It's less of a concern if it's really only that one hook (although still somewhat fragile and unexpected behavior), so maybe we should reconsider #3 after all. But let's get the current patch in first.

Comment #8

mfer CreditAttribution: mfer commented 2 January 2010 at 19:01

Title:

Profiles not loaded as modules during install

» Remove fake form alter hook

This patches above remove a fake form alter hook. At this point core has a full boot strap and the FAPI system is in place. There is no need to the fake form alter here.