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?
Comment | File | Size | Author |
---|---|---|---|
#40 | 655190-d7-40.patch | 1.95 KB | xjm |
#34 | hook_install_tasks_in_install-655190-34.patch | 1.97 KB | pingers |
#30 | hook_install_tasks_in_install-655190-30.patch | 1.97 KB | kathyh |
#29 | hook_install_tasks_in_install-655190-29.patch | 1.97 KB | kathyh |
#26 | hook_install_tasks_in_install-655190-26.patch | 1.95 KB | dealancer |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedCan 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.
Comment #2
jurgenhaasHere is a strip-down version of the install file from my profile:
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedTry 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.
Comment #4
jurgenhaasThanks 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.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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.)
Comment #6
jurgenhaasYes, 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.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedActually, 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.
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...
Comment #8
jurgenhaasSorry, 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.
Comment #9
dealancer CreditAttribution: dealancer commentedHey 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:
I am going to debug it a little bit more.
Comment #10
dealancer CreditAttribution: dealancer commentedIn install.core.inc there are following lines:
also I see following lines of code:
Weird thing that in the first snippet we don't call include_once $profile_file;
Comment #11
dealancer CreditAttribution: dealancer commentedAdvice #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
Comment #12
pingers CreditAttribution: pingers commentedOkay, the patch looks good and follows the pattern for including the .profile file.
I think should be the following:
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).
Comment #13
catchComment #14
catchThis line is way over 80 characters.
Why is_file() instead of file_exists()?
Otherwise looks fine.
-1 days to next Drupal core point release.
Comment #15
dealancer CreditAttribution: dealancer commentedRight, 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...
Comment #16
dealancer CreditAttribution: dealancer commentedif_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.
Comment #17
catchThere'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...
Comment #18
dealancer CreditAttribution: dealancer commentedOk, going to fix that.
Comment #19
dealancer CreditAttribution: dealancer commentedHere is new patch which solves the issue and which updates lines width and uses file_exists() instead of is_file().
Comment #20
dealancer CreditAttribution: dealancer commentedOops, last patch updates mode of default.settings.php. Here is new one.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedLooks pretty good except for a couple things:
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.
No reason to break this up onto several lines (same goes for other places in the patch).
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 :)
Comment #22
dealancer CreditAttribution: dealancer commentedOk 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.
Comment #23
dealancer CreditAttribution: dealancer commentedOk, so here is a new patch.
Condition effect, changed comment, line width.
Comment #24
dealancer CreditAttribution: dealancer commentedComment #25
catchhook_install_tasks() should have the parentheses.
This hunk is adding trailing whitespace.
-24 days to next Drupal core point release.
Comment #26
dealancer CreditAttribution: dealancer commentedThx for notice.
Here is a new working patch with fixed issue above.
Comment #27
pingers CreditAttribution: pingers commentedmight be better as:
Comment #28
xjmThank 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.
Comment #29
kathyh CreditAttribution: kathyh commentedUpdated for #28 d8 /core change. #27 got fixed as well. Plz switch to "needs work" after testbot for someone to add the automated test.
Comment #30
kathyh CreditAttribution: kathyh commentedReturned 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.
Comment #31
dealancer CreditAttribution: dealancer commented@kathyh, thx for re roll.
Since we have testing profile it should be easy to add some tests there. My plan is following.
Comment #32
dealancer CreditAttribution: dealancer commentedComment #33
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
Needs a quick reroll to fix the missing space after "invoked"; everything looks good to me besides that.
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 :)
Comment #34
pingers CreditAttribution: pingers commentedTend 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...
Comment #35
pingers CreditAttribution: pingers commentedBah.
Comment #36
catchI'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.
Comment #37
David_Rothstein CreditAttribution: David_Rothstein commentedDouble-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.
Comment #38
catchI've opened #1333940: Consistently use either is_file() or file_exists() to discuss is_file() vs. file_exists().
Comment #39
catchCommitted/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.
Comment #40
xjm7.x patch. I kept double-checking because it refs
DRUPAL_ROOT
, but it's just the profiles dir.Comment #41
dealancer CreditAttribution: dealancer commentedIt 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.
Comment #42
catchSorry folks, committed but not pushed, now done that last bit..
Comment #43
BTMash CreditAttribution: BTMash commentedThe 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.
Comment #44
catchLooks 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.
Comment #45
webchickHm. Kitteny is right. ;) But I follow the logic, so ok.
Committed and pushed to 7.x. Thanks!
Comment #46
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedWould this benefit from a change notification explaining that hook_install_tasks can now be placed in a profile's .install file?