Some module (like those with default content) can take quite a long time to install/enable. Batch modules one at a time to ensure we get the best chance to not hit max execution time.

CommentFileSizeAuthor
#3 apps-rework-batch-1750240-3.patch10.54 KBnedjo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

populist’s picture

Assigned: Unassigned » populist
nedjo’s picture

nedjo’s picture

Version: 7.x-1.0-beta7 » 7.x-1.x-dev
Assigned: populist » nedjo
Status: Active » Needs review
FileSize
10.54 KB

Attached patch reworks app enabling to use per-module batches, both at install time and in post-install app enabling.

As well as addressing potential out of memory errors, this approach has the advantage of using the same code for install-time and post-install app enabling.

#1572578: Rethink the way features are enabled / disabled is related, in that it addresses a distinct source of out of memory errors when enabling apps.

A few outstanding questions are noted in TODO code comments. Specifically:

+++ b/apps.module_batch.inc
@@ -0,0 +1,180 @@
+      // If we're enabling a single app and have set a redirect to a
+      // configuration page, we need to reset the redirect.
+      // See apps_app_enable().
+      // TODO: is this the best way to override a batch redirect?
+      $batch = &batch_get();
+      // Conditionally set the redirect based on access to a configure page.
+      if (apps_app_access('administer apps', $app, 'configure') && $batch['redirect'] == apps_app_page_path($app, 'configure')) {
+        $batch['redirect'] = apps_app_page_path($app);
+      }

Prior to the patch, on app enabling, those with access are redirected to a config page if one exists, unless app enabling fails. To maintain this functionality, we need to conditionally change the redirect passed to batch_process(). Is this the best way to do so?

+++ b/apps.module_batch.inc
@@ -0,0 +1,180 @@
+      // Don't generate success messages in install mode because the message
+      // will show up at an inappropriate time (on the next page).
+      // TODO: is there a better way to test for install time usage?
+      if (!(defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install')) {
+        // Store result for post-processing in the finished callback.
+        $context['results'][] = $module_name;
+      }

Is there a better way to test for install time usage?

+++ b/apps.module_batch.inc
@@ -0,0 +1,180 @@
+      // Invoke a post install callback if one is defined.
+      // TODO: should we skip this call at install time?
+      if (!$app['disabled'] && ($callback = apps_app_callback($app, 'post install callback'))) {
+        $callback($app);
+      }

Prior to this patch, an app's 'post install callback' was called for apps enabled individually post-install but not at site install time. Now the callback will be called in both cases. Is this desired?

nedjo’s picture

Assigned: nedjo » Unassigned
Issue summary: View changes