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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

populist’s picture

Title: Allow Apps to Be Installed with Drush » Allow Apps to Be Installed with Drush During Site Install

Updating the title to be more precise and differentiate from #1349668: Drush commands for Apps

sylus’s picture

This still does not work for me and also fails in panopoly distribution.

sylus’s picture

Nevermind drush site install does work ended up being a proxy problem ^_^ Thanks for the patch!

nedjo’s picture

Status: Needs review » Needs work

A good start.

+++ b/apps.installer.inc
@@ -118,7 +118,6 @@ function apps_download_batch($project, $url, $type, &$context) {
-  $archive_errors = array();

Is this an unrelated fix?

+++ b/apps.profile.inc
@@ -13,7 +13,7 @@
   // Only use apps forms during interactive installs.

This comment needs updating.

populist’s picture

The 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.

sylus’s picture

Hey 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).

sylus’s picture

I 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'];
}

nedjo’s picture

While 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:

  • Simply drop the if ($install_state['interactive']) test, rather than extending it.
  • Add logic to cover the non-interactive case. Check for directory writability. If available, okay. If not:
    • Test required and selected apps against already installed apps.
    • If not all required apps are available locally, set a flag and skip all further apps install tasks.
    • If some selected apps are available locally, reduce selected apps to those available locally and proceed.
nedjo’s picture

Here'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.

Robin Millette’s picture

Patch 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.

Robin Millette’s picture

Using 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]

nedjo’s picture

@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.

populist’s picture

I 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.

nedjo’s picture

@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.

nedjo’s picture

Status: Needs work » Needs review
FileSize
6.77 KB

Duelling 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.

febbraro’s picture

Status: Needs review » Needs work

I 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.

nedjo’s picture

@febbraro: no, I haven't done that testing.

febbraro’s picture

Here 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)

febbraro’s picture

Status: Needs work » Needs review
FileSize
7.93 KB

New 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.

nedjo’s picture

Status: Needs review » Needs work

@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:

  • Interactive Install, both local apps and remote apps
  • Interactive Install, neither local nor remote apps
  • Drush Install, both local apps and remote apps
  • Drush Install, neither local nor remote apps

Do we need tests? If so, what would they look like?

+++ b/apps.profile.inc
@@ -288,7 +293,24 @@ function apps_profile_install_app_modules(&$install_state) {
+  if (!$install_state['interactive']) {
+    apps_include('manifest');
+    // If not interactive it is one big request, clear some static caches
+    // so we recognized modules that have been downloaded just prior.
+    system_list_reset();
+    drupal_static_reset('apps_manifest');
+    $installed_apps = apps_apps($_SESSION['apps_server']['machine name'], array('installed' => TRUE), TRUE);
+    $_SESSION['apps'] = array_intersect_key($_SESSION['apps'], $installed_apps);
+    // If no selected apps are installed, we have nothing to enable.
+    if (empty($_SESSION['apps'])) {
+      return;
+    }
+  }

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.

nedjo’s picture

Status: Needs work » Needs review

Keep forgetting that dreditor sets the status for me....

febbraro’s picture

@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?

febbraro’s picture

Ran all 8 tests and things work as expected.

populist’s picture

The 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

febbraro’s picture

Status: Needs review » Fixed

Sweet, it's in. Thanks for your help testing.

http://drupalcode.org/project/apps.git/commit/d74715e

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Adding additional information