I have written a install profile and defined a hook_install_tasks() with a form as one of the tasks. The form gets displayed and can be filled in and submitted, but the associated submit function doesn't get called.

The form is defined as

function wskp_user_config_form($form, &$form_state, &$install_state)

and the submit function is defined as

function wskp_user_config_form_submit($form, &$form_state)

Is that a bug or am I doing something wrong?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Status: Active » Postponed (maintainer needs more info)

Can you post more of your code, including the code you have in hook_install_tasks() as well as some code from the form functions themselves?

I just tried it myself with a very basic example, and it seemed to work correctly for me.

jurgenhaas’s picture

Here is a strip-down version of the install file from my profile:

<?php
// $Id$

/**
 * Implement hook_install().
 *
 * Perform actions to set up the site for this profile.
 */
function wskp_install() {
  //some functions here
}

/**
 * Implement hook_install_tasks().
 */
function wskp_install_tasks() {
  return array(
    'wskp_user_config_form' => array(
      'display_name' => st('Configure WSKP'),
      'type' => 'form',
    ),
  );
}

function wskp_some_functions() {
  //...
}

function wskp_user_config_form($form, &$form_state, &$install_state) {
  $form = array();
  $form['email_admin'] = array(
    //field def
  );
  //more fields
  $form['#submit'][] = 'wskp_user_config_form_submit';
  return $form;
}

function wskp_user_config_form_submit($form, &$form_state) {
  watchdog('wsk', 'USER SUBMIT FORM REACHED');
  //some actions
}
David_Rothstein’s picture

Status: Postponed (maintainer needs more info) » Active

Try moving all the hook_install_tasks() and related form definition functions to your .profile file rather than your .install file. I think that will fix it.

I guess we should probably update the API docs for http://api.drupal.org/api/function/hook_install_tasks/7 and http://api.drupal.org/api/function/hook_install_tasks_alter/7 to explain where these functions need to be defined - it is indeed not at all obvious given that install profiles now have many more associated files than they used to.

jurgenhaas’s picture

Thanks a lot David, this is indeed solving the problem. In a first attempt I had only moved the form and the submit function to the .profile file and that didn't work. Next, I also moved the hook_install_tasks() and suddenly all worked fine. Technically I would have argued that doesn't matter, but obviously it does.

Again, thanks for your help and yes, I guess the great new features of install profiles could do well with a little bit more documentation.

David_Rothstein’s picture

Title: A form submit function not called in install profile » Allow hook_install_tasks() to be called from a profile's .install file (or document that it can't be)

OK, let's give this a more accurate title :)

It would actually make a fair amount of sense if you could just put it in an .install file, especially given the way the hook is named and the fact that it only gets called at install time. (A lot of these changes were being made a few months ago all at the same time, and I think the .install files got introduced only around the same time the new hook behavior was being finalized, so they never synced up. But I'm not sure there's any obvious reason not to just always load the .install files during Drupal installation, and therefore allow that to occur.)

jurgenhaas’s picture

Yes, putting it in the .install files is a good solution and confimed that it works. Still, it is confusing why there are two files (.install and .profile) and what needs to go into which file. Or I may have missed some description that's available and I just haven't found it yet?

Both files are called during install only, so I don't get the point yet, why there is a distinct difference.

David_Rothstein’s picture

Both files are called during install only, so I don't get the point yet, why there is a distinct difference.

Actually, the files are used after the site is installed also; the idea is that install profiles now work a lot like regular modules. They can implement hooks, have update functions, etc. So the .install file works similar to a module's install file, and the .profile file works similar to a module's .module file. Looking at http://drupal.org/node/224333, it does seem that none of this was really documented - it should be.

Yes, putting it in the .install files is a good solution and confimed that it works.

By "confirmed that it works" you mean you actually got it to work? How (did you write code for this)? I thought the original problem you had was that it didn't work when you put it there...

jurgenhaas’s picture

Sorry, if my feedback wasn't quite clear. It does work now, since I've copied all three components (hook_install_tasks, form and submit-handler) into the .install file.

But your other feedback sounds very interesting too: the fact that profiles work like modules in particular with regard to hooks and updates, is great. It means that if a profile gets improved for new sites it can also be used to keep existing sites up-to-date as well. Nice concept.

dealancer’s picture

Hey guys,

I was trying to use hook_install_tasks() on the OpenAcaDept profile in install file and I had a problem with a running batches. Looks like normal tasks are working great for me, however if there are batches I have following error:

"Requested Range Not Satisfiable"

I was trying to debug, looks like batches are scheduled, however after reloading url is install.php?profile=openacadept&locale=en&op=start&id=2 and there aren't any batch tasks in the queue.

My code is pretty simple:

/**
 * Implements hook_install_tasks().
 *
 * Perform some taks after features have been extracted.
 */
function openacadept_install_tasks() {
  $tasks = array(
    '_openacadept_create_menus' => array(),
    '_openacadept_batch_processing' => array(
      'display_name' => st('Create nodes, menu items and terms.'),
      'display' => TRUE,
      'type' => 'batch',
      'run' => INSTALL_TASK_RUN_IF_NOT_COMPLETED,
    ),
  );
  return $tasks;
}

function _openacadept_batch_processing(&$install_state) {
  return array(
    'title' => t('Create new items'),
    'operations' => array(
        array('_my_function_2', array()),
    ),
  );
}

// More advanced example: multi-step operation - load all nodes, five by five
function _my_function_2(&$context) {
  if (empty($context['sandbox'])) {
    $context['sandbox']['progress'] = 0;
    $context['sandbox']['current_node'] = 0;
    $context['sandbox']['max'] = 3;
  }

  $context['sandbox']['progress']++;
  
  if ($context['sandbox']['progress'] != $context['sandbox']['max']) {
    $context['finished'] = $context['sandbox']['progress'] / $context['sandbox']['max'];
  }
}

I am going to debug it a little bit more.

dealancer’s picture

In install.core.inc there are following lines:

  // Now add any tasks defined by the installation profile.
  if (!empty($install_state['parameters']['profile'])) {
    $function = $install_state['parameters']['profile'] . '_install_tasks';
    if (function_exists($function)) {
      $result = $function($install_state);
      if (is_array($result)) {
        $tasks += $result;
      }
    }
  }

also I see following lines of code:

  // Allow the installation profile to modify the full list of tasks.
  if (!empty($install_state['parameters']['profile'])) {
    $profile_file = DRUPAL_ROOT . '/profiles/' . $install_state['parameters']['profile'] . '/' . $install_state['parameters']['profile'] . '.profile';
    if (is_file($profile_file)) {
      include_once $profile_file;
      $function = $install_state['parameters']['profile'] . '_install_tasks_alter';
      if (function_exists($function)) {
        $function($tasks, $install_state);
      }
    }
  }

Weird thing that in the first snippet we don't call include_once $profile_file;

dealancer’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
1.15 KB

Advice #6 helped me. I have just moved implementation of hook_install_tasks to .profile dir and it worked well.

However, the real problem is that hook sometimes called from .install file cause .install can be already loaded, but when performing a batch it isn't loaded, so we need to load it manually. Here is a patch

pingers’s picture

Okay, the patch looks good and follows the pattern for including the .profile file.

// Need to load profile install file, because it isn't loaded when batch operations are being executed

I think should be the following:

// Load the profile install file, because it is not loaded when batch operations are being executed.

Patch contains this wording change and period at end of comment.

It would be good to have a test for this, but I can't see where/if we test installation profiles (or their hooks.. hook_install_tasks, hook_install_tasks_alter).

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
catch’s picture

Status: Needs review » Needs work
+++ b/includes/install.core.incundefined
@@ -570,11 +570,16 @@ function install_tasks($install_state) {
-        $tasks += $result;
+    // Load the profile install file, because it is not loaded when batch operations are being executed.
+    $profile_install_file = DRUPAL_ROOT . '/profiles/' . $install_state['parameters']['profile'] . '/' . $install_state['parameters']['profile'] . '.install';
+    if (is_file($profile_install_file)) {
+      include_once $profile_install_file;
+      $function = $install_state['parameters']['profile'] . '_install_tasks';
+      if (function_exists($function)) {
+        $result = $function($install_state);
+        if (is_array($result)) {
+          $tasks += $result;
+        }
       }
     }
   }

This line is way over 80 characters.

Why is_file() instead of file_exists()?

Otherwise looks fine.

-1 days to next Drupal core point release.

dealancer’s picture

Right, we need to use file_exists(). Going to rewrite patch today.

But also we have same issue here:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/includes/i...
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/includes/i...

dealancer’s picture

if_file checks if it is correct filename and file_exists checks if file or directory exists. Looks like from this point it is better to call both functions.

catch’s picture

There's no valid install profile that's going to have a directory called $profile.install, so file_exists() should be plenty. The same check is used for module files in http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal...

dealancer’s picture

Ok, going to fix that.

dealancer’s picture

Here is new patch which solves the issue and which updates lines width and uses file_exists() instead of is_file().

dealancer’s picture

Oops, last patch updates mode of default.settings.php. Here is new one.

David_Rothstein’s picture

Looks pretty good except for a couple things:

+    // Load the profile install file, because it is not loaded when batch
+    // operations are being executed.

I don't understand this code comment - what does this have to do with batch operations? I think the issue here is just that the .install file is normally not loaded by Drupal (except while the module/profile is actually being installed, or under other specific scenarios). So not just batch operations, but lots of other places during the installer will not load this file unless we specifically tell it to.

+    $profile_install_file = DRUPAL_ROOT . '/profiles/' .
+      $install_state['parameters']['profile'] . '/' .
+      $install_state['parameters']['profile'] . '.install';

No reason to break this up onto several lines (same goes for other places in the patch).

+    if (file_exists($profile_install_file)) {
+      include_once $profile_install_file;
+      $function = $install_state['parameters']['profile'] . '_install_tasks';
+      if (function_exists($function)) {
+        $result = $function($install_state);
+        if (is_array($result)) {
+          $tasks += $result;
+        }

I think the 'include_once' statement is the only part that should go inside the file_exists() check. Unless we are trying to change things so that putting hook_install_tasks() in an .install file is now a requirement....

Hm, maybe that does make sense, but if so we have to document it (and that part is not safe for backport to D7).

***

In general I'm noticing that the inclusion of .profile and .install files during installation is kind of a mess (lots of repetitive file inclusions all over the place whereas it would be better to centralize it in one place), but we can save cleaning that up for a separate issue :)

dealancer’s picture

Ok we need to changes text 'Load the profile install file, because it is not loaded when batch operations are being executed' to 'Load the profile install file, because it is not loaded when hook_install_tasks is invoked sometimes (e.g. batch processing)'. In some cases it is invoked in other parts of code.

> I think the 'include_once' statement is the only part that should go inside the file_exists() check. Unless we are trying to change things so that putting hook_install_tasks() in an .install file is now a requirement....

agreed

> In general I'm noticing that the inclusion of .profile and .install files during installation is kind of a mess (lots of repetitive file inclusions all over the place whereas it would be better to centralize it in one place), but we can save cleaning that up for a separate issue :)

Actually .install is included in almost every place when running install profile. The real problem is: if we implement hook_install_tasks in .install it is invoked, but then some of operations, like batch, stops working cause it isn't included in this peace of code.

dealancer’s picture

Ok, so here is a new patch.

Condition effect, changed comment, line width.

dealancer’s picture

Status: Needs work » Needs review
catch’s picture

Status: Needs review » Needs work
+++ b/includes/install.core.incundefined
@@ -571,13 +571,19 @@ function install_tasks($install_state) {
+    // Load the profile install file, because it is not loaded when

hook_install_tasks() should have the parentheses.

+++ b/includes/install.core.incundefined
@@ -571,13 +571,19 @@ function install_tasks($install_state) {
-    }
+    }    ¶

This hunk is adding trailing whitespace.

-24 days to next Drupal core point release.

dealancer’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

Thx for notice.

Here is a new working patch with fixed issue above.

pingers’s picture

Load the profile install file, because it is not loaded when hook_install_tasks() is invoked sometimes (e.g. batch processing).

might be better as:

Load the profile install file, because it is not always loaded when hook_install_tasks() is invoked (e.g. batch processing).

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Novice

Thank you for your work on this issue. I think we should try to add an automated test for this, if possible, The comment change suggested in #27 also looks like a good idea to me.

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

kathyh’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

Updated for #28 d8 /core change. #27 got fixed as well. Plz switch to "needs work" after testbot for someone to add the automated test.

kathyh’s picture

Returned a lost period and removed an extra space (both my errors in #29). Again, plz switch to "needs work" after testbot for someone to add the automated test.

dealancer’s picture

@kathyh, thx for re roll.

Since we have testing profile it should be easy to add some tests there. My plan is following.

  1. Add to testing.install file implementation of hook_install_tasks.
  2. When install tasks are run, save variables that indicates that tasks are run.
  3. Add test that will check for this variables.
dealancer’s picture

Status: Needs review » Needs work
David_Rothstein’s picture

I don't think it's possible to test this, due to #1215104: Use the non-interactive installer in WebTestBase::setUp()? Simpletest does not use the normal installation procedure when setting up a profile for the tests.

Otherwise:

+    // Load the profile install file, because it is not always loaded when
+    // hook_install_tasks() is invoked(e.g. batch processing).

Needs a quick reroll to fix the missing space after "invoked"; everything looks good to me besides that.

-    if (is_file($profile_file)) {
+    if (file_exists($profile_file)) {

While I don't care much either way, I don't really understand this change? The rest of Drupal core is entirely inconsistent in these situations (some places use file_exists and others use is_file), so it's not like we have a convention we need to enforce. Since is_file() is more accurate and (ever-so-slightly) faster, I'm not sure what the reason is to change it to file_exists() here, but whatever :)

pingers’s picture

Tend to agree with David_Rothstein re: file_exists(). is_file() performs all the checking we need. Maybe this is something that should be it's own issue though (probably many more cases throughout core that could be improved - albeit a very small improvement).

Here's the re-roll for the comment space issue...

pingers’s picture

Status: Needs work » Needs review

Bah.

catch’s picture

I'm fine with switching to is_file(), just wanted it to be consistent with what we do elsewhere - but if we're inconsistent elsewhere too let's try to solve that separately. I don't notice when writing that comment that we're using is_file() elsewhere in the file.

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Double-checked this patch, and it's good to go.

Yeah, we can certainly solve is_file() vs file_exists() elsewhere. By changing all the locations, the patch does keep the install system self-consistent (now using file_exists everywhere), which is good enough.

catch’s picture

I've opened #1333940: Consistently use either is_file() or file_exists() to discuss is_file() vs. file_exists().

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x. Looks like updates were made to the 8.x patch after the last 7.x patch was rolled, so needs re-rolling I think.

xjm’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.95 KB

7.x patch. I kept double-checking because it refs DRUPAL_ROOT, but it's just the profiles dir.

dealancer’s picture

It is great idea discuss is_file/file_exists in separate place.

Re pushing patch for D8. Was it commited? Nothis is here http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/inclu...

Thx for re-roll for D7.

catch’s picture

Sorry folks, committed but not pushed, now done that last bit..

BTMash’s picture

The patch looks good to me and given that the backport patch is fairly identical to what went in for D8 (minus the /core but nonetheless using DRUPAL_ROOT), a def +1 that its ready for D7.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good here too. The is_file() checks are a bit kitteny for D7 but will also make backports easier if we have to touch the same code again, so I think ok in this instance.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. Kitteny is right. ;) But I follow the logic, so ok.

Committed and pushed to 7.x. Thanks!

Tor Arne Thune’s picture

Would this benefit from a change notification explaining that hook_install_tasks can now be placed in a profile's .install file?

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -Novice, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.