Here is a small patch that allows for the "drush site-install PROFILE" method of installing a Drupal site. Currently, this patch will only work in cases where sites/all/modules is writeable but it will automatically download and install the apps that are defaulted in the form.
In terms of improvement, there are two areas that I can see the need for additional development:
1.) Support for multisite or installation of apps in areas that are not sites/all/modules. This will require additional work with the apps.module.
2.) Support for other methods of installing apps beyond the webserver writing to disk.
Comment | File | Size | Author |
---|---|---|---|
#19 | apps-drush-install-1561652-19.patch | 7.93 KB | febbraro |
#18 | apps-drush-install-1561652-18.patch | 6.82 KB | febbraro |
#15 | apps-drush-install-1561652-15.patch | 6.77 KB | nedjo |
#13 | 1561652-apps-allow-drush-site-install-patch-reroll-1479164.patch | 1.72 KB | populist |
#9 | apps-drush-install-1561652-9.patch | 6.98 KB | nedjo |
Comments
Comment #1
populist CreditAttribution: populist commentedUpdating the title to be more precise and differentiate from #1349668: Drush commands for Apps
Comment #2
sylus CreditAttribution: sylus commentedThis still does not work for me and also fails in panopoly distribution.
Comment #3
sylus CreditAttribution: sylus commentedNevermind drush site install does work ended up being a proxy problem ^_^ Thanks for the patch!
Comment #4
nedjoA good start.
Is this an unrelated fix?
This comment needs updating.
Comment #5
populist CreditAttribution: populist commentedThe issue mentioned in #4 is in fact unrelated. It seemed odd, to me, that $archive_errors was set and then set to array()! I think this patch still needs work, since its use case for me was narrow, but just clarifying things here.
Comment #6
sylus CreditAttribution: sylus commentedHey Populist amazed by your work on panopoly. Have you came across this error during a drush si?
WD php: Notice: Undefined index: op in apps_profile_apps_select_form_submit() (line 147 of
/var/www/html/site/profiles/panopoly/modules/contrib/apps/apps.profile.inc).
Comment #7
sylus CreditAttribution: sylus commentedI managed to get xdebug to finally work with drush and set a breakpoint for the specified line. The corresponding problem is:
// Checking to make sure that skip was not clicked. We compare the op
// against the form_state for click to handle both the interactive case
// where op is set and the non-interactive case where there is no op.
if ($form_state['values']['op'] != $form_state['values']['skip']) {
$_SESSION['apps'] = array_filter($form_state['values']['apps']);
$_SESSION['apps_default_content'] = $form_state['values']['default_content'];
}
Where when using drush si the form_state['values']['op'] parameter does not exist.
I changed the code to check whether the form element exists and the problem went away.
if (isset($form_state['values']['op']) != $form_state['values']['skip']) {
$_SESSION['apps'] = array_filter($form_state['values']['apps']);
$_SESSION['apps_default_content'] = $form_state['values']['default_content'];
}
Comment #8
nedjoWhile testing for directory writability will catch some cases where apps can be installed without user input, it will miss another valid use case: apps are already locally installed and therefore don't need to be downloaded and installed. See #1714552: Install tasks for downloading and installing apps run even if all selected apps already installed.
Looks like we should do something like this:
if ($install_state['interactive'])
test, rather than extending it.Comment #9
nedjoHere's an untested draft.
Incorporates the patches at #1714552: Install tasks for downloading and installing apps run even if all selected apps already installed and #1479164: Method for checking writability of server because their changes are required here.
Approach is roughly similar to what I outlined in #8, with some minor variance.
Comment #10
Robin Millette CreditAttribution: Robin Millette commentedPatch in OP allowed me to install the dev version of Open Outreach with Aegir. I'm going to give the patch in #9 a try now.
I noticed a couple of undefined indexes (in $_SERVER) but those are related to the openoutreach profile.
Comment #11
Robin Millette CreditAttribution: Robin Millette commentedUsing latest openoutreach again, this time adding apps dev version. Patch applied correctly, added platform to Aegir and successfully created an open outreach site.
Excellent, muchos gracias!
[sorry, I edited my comment a few times with false starts and wrongs leads]
Comment #12
nedjo@Robin Millette: thanks for testing! The patch doesn't apply because it includes two other patches that are already applied to the version of apps in openoutreach.
To test, the best thing would be to start with a fresh git checkout of the 7.x-1.x branch (or a dev download) and apply the patch in #9.
Comment #13
populist CreditAttribution: populist commentedI spent some time with the patch in #9 and, while it was able to install the site, it wasn't able to automatically enable my modules that has already been downloaded. It is possible that my profile implementation is different (there are emerging, but not yet consistent standards here) or that some of other patches are confusing things, but here is a re-roll of my patch from #1 which assumes the much needed permissions patch in #1479164: Method for checking writability of server.
Comment #14
nedjo@populist. Thanks for testing.
The reason I'm very reluctant to see the is_writable() test added here is that it would strengthen the current unwritten assumption that install-time apps must be downloaded.
After switching to apps, I watched Open Outreach install stats take their first ever significant plunge. I don't know why, but I wouldn't be surprised if the apps download ftp screen was a factor. In the case of Open Outreach, folks have already downloaded a fully packaged distro and figured out how to run an installer. If they're new to Drupal, that's already a big step. Now they have to get past the hurdle of providing authentication information for protocols that their host may not even support--to download what's already there?
Tempting as it is to quickly fix errors, we shouldn't be introducing new code at this point that reinforces this mistaken assumption.
But I understand and sympathize with the desire to just get some of these issues fixed in Apps. I'm right now trying to get a bugfix release posted for Open Outreach and having to refresh and rework various apps patches is the detail holding me back. With various distros now depending on apps, we need a bit of concerted joint work to tighten it up.
Comment #15
nedjoDuelling patches ;)
We probably just both need something on d.org so we can get our releases to package again....
This is a refresh of the patch in #9 with a fix (I'd used array_diff_key() instead of array_intersect_key()). I just ran a quick
drush si openoutreach
with this patch and got the in-place apps enabling. Needs more review and testing.Comment #16
febbraro CreditAttribution: febbraro commentedI like the direction that nedjo is taking it with #15. If all the apps are already downloaded, lets not hold up things if you dont have write access. Nedjo, have you tested your patch where apps DO have to be downloaded to install? We obviously need to support both cases, and i was just about to test it out, but figured I'd ask first.
Comment #17
nedjo@febbraro: no, I haven't done that testing.
Comment #18
febbraro CreditAttribution: febbraro commentedHere is a re-roll of #15 against the current code base.
Here are the tests I have run.
1) Interactive Install, app not downloaded. PASSED (downloaded and enabled)
2) Interactive Install, app downloaded. PASSED (enabled)
3) Drush Install, app not downloaded. FAILED (downloaded but not enabled)
4) Drush Install, app downloaded. PASSED (enabled)
Need to figure out why the app was not enabled in test 3 (which is consistent with @populist's test in comment #13). I have a vanilla drupal install with the standard profile modified for apps goodness. Wanted to make sure I was not influenced by a Panopoly, OpenPublic, or OpenOutreach specific profile setup)
Comment #19
febbraro CreditAttribution: febbraro commentedNew version of the patch. Since a drush site-install is one big request I had to clear some static caches to get it to recognize that a module was just downloaded and available for install. Pretty sure this one is good to go, but if nedjo or populist could give it a whirl to guarantee that'd be swell. If not I will commit it in the next day or so.
Comment #20
nedjo@febbraro: thanks for the additional testing and fixing and for your other recent work in the issue queues.
This patch is introducing some fairly complex patterns--the four outlined in #18 plus, probably, four more:
Do we need tests? If so, what would they look like?
We may in future have cases where this code block should be run for interactive installs as well. E.g., if a user is prompted for FTP creds and doesn't have them, we may want to offer a screen saying something like "3 of the apps you've selected are not installed locally and will need to be downloaded. You may click the Cancel link to skip this download step and only enable the 5 apps that are already installed." But we don't yet support skipping the download, so we could make that change if and when we do.
Comment #21
nedjoKeep forgetting that dreditor sets the status for me....
Comment #22
febbraro CreditAttribution: febbraro commented@nedjo, yeah I think those 8 tests encompass the variation we want to test for, however I don't yet see how to make an automated (simpletest) test to actually test the installation on those cases.
We would need a profile with hook_install_tasks implemented in order to define the app install steps. Profiles are only searched for in DRUPAL_ROOT/profiles, so we could not bundle that test profile with the apps module and have it still be installable/testable. We could create an apps_test profile, and have that bundle the apps module which contains the tests. Those test cases can use the apps_test profile as the install for those specific tests, but even then, the simpletest runner can not test the interactive install (I don't think), only the automated install, essentially only the non-interactive portion.
Seems like it might not be possible to test everything we want in an automated fashion unless we use something like casperjs/behat to run the interactive portion and some simpletest shennanigans like I mentioned above to test the drush portion.
Anyone else have any thoughts on how we might accomplish that?
Comment #23
febbraro CreditAttribution: febbraro commentedRan all 8 tests and things work as expected.
Comment #24
populist CreditAttribution: populist commentedThe patch in #19 works for me in the following cases:
-- drush install with local apps
-- interactive install with local apps
-- drush install with downloaded apps
Comment #26
febbraro CreditAttribution: febbraro commentedSweet, it's in. Thanks for your help testing.
http://drupalcode.org/project/apps.git/commit/d74715e
Comment #27.0
(not verified) CreditAttribution: commentedAdding additional information