Timeout on enabling modules: make it a batch operation

More necessary than ever, now that we're downloading interface translations and translating config data, even though the latter two are batched.
The more we can see the actual module_enable() and module_disable() as the missing piece in batching module installation now.

Not ready, doesn't work at all. Just a starter so I keep track.

Here's a working patch. It has a small issue with the progress count, but batch enable and disable are functioning as expected.

So, the counting was correct, but the previous patch was performing enables and disables as 2 operations (so the count was based on 2 possible actions). I corrected that, which simplifies the code.

I tested in simplytest.me. I set it up with the minimal profile, then enabled every possible module, and it worked great. I then disabled every possible module and it also worked great. Both enabling and disabling used the batch system.

Good job.

-James

I have reviewed this and ran a test against the current git install of drupal 8.
Worked as expected.

-- Karyn

#5: module_enabling_batch-1387438-5.patch queued for re-testing.

Looks like this also needs to update some of the core tests.

So, there's a few other issues with this patch:

1. batch runs asynchronously, so the function doesn't set the message or flush caches
2. should rename the batch function so it's properly name spaced for system

New patch to address the issues for namespacing and moves cache flush into the finish callback. It also removes the unnecessary module list comparison. It still uses module_enable and module_disable which may be throwing the errors in testing.

#13: module_enabling_batch-1387438-13.patch queued for re-testing.

#13: module_enabling_batch-1387438-13.patch queued for re-testing.

Nevermind, something seems to be wrong with the bot.
I had already prepared a working patch, but don't want to take over your issue with posting it, so here's a few remarks to #13:

• There is already a _install_module_batch() in install.core.inc. We should definitely use one and the same batch callback on both instances, so we should probably move it to install.inc, rename it to _install_module_enable_batch(), and add a separate _install_module_disable_batch(), instead of reimplementing in system.admin.inc.
• We want the nice module name not the raw filename in the batch status, so in system_modules() we need to pass $info['name'] or why not the whole $info array as value: $form['info'] = array('#type' => 'value', '#value' => $info);, so in the submit handler we can pull it from $form_state['values']: $modules[$module]['name'] = $form_state['values']['info']['name'];.
• Nice if the module list comparison really can go away. Had anyway problems with it anyway in #1990544: Convert system_modules() to a Controller.
• Don't know if we can come up with something better than "Processing configuration options." and "There was a problem saving the configuration options." This is slightly diffuse. Maybe "Processing module configuration." is better. Maybe we should also get more specific if we're "just enabling" or "just disabling".

Thanks for the insight and advice! I assigned this to myself during the DrupalCon Portland sprints, just to prevent duplicate work, but I'm happy to reset to "unassigned" if you'd like to work on it.

Don't worry, I'm fine with either way we're getting this ready to land! :)

How about my remarks, especially the wording of the batch title?
I've not yet found a really optimal wording for 'module enable & disable operations'. We could take the best one we come up with, though, and leave the final wording for UX finetuning.

Here's an update using the module name (as from form_state).
It still needs work for the install process. I played with it a bit, but didn't get as far as i'd like, but I agree that this should move. system_mdoules_submit already includes the install.inc, so it makes sense to move it. I'll likely work on the move next week.

Nice. However, #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair removes module_enable/_disable replacing it by module_install/_uninstall, so this needs to be refactored.

Really makes no sense with a moving target. Postponing on #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair and possible followup refactoring.
Timeouts leading to failing or borked module installations however make this a major bug.

At the same time we need to take into consideration that batches cover a number of errors & warnings, so making this a batch operation makes debugging modules harder. This doesn't constitute a no-go, but we need to figure out how to avoid a DX regression. Possibly devel might introduce an additional button "Install un-batched" or something.

This looks like a simple enough patch so far that it doesn't need to be postponed on that one... does it?

I don't think the postpone was on the core of that issue, but mostly on changes from module_enable/disable to module_install/uninstall. I haven't been keeping up with any of that work, but assuming the dust has settled, I think this can go back to active development.

Active! I'm gonna work on this for the BADCamp sprint.

hmm... Many things have changed since I last looked at this, so I think some discussion of how best to implement this would be helpful.

Should batch install be it's own method on the ModuleHandler or should it just be an additional param on the current install method. i.e. \Drupal::ModuleHandler->install($module_list, $enable_dependencies, $batch) or \Drupal::ModuleHandler->installBatch($module_list, $enable_dependencies)

This issue is also seen in D7 when enabling features that have a high dependency of modules. If a user enables the internationalization modules before enabling Lingotek, thats fine. But if they do it as part of one module enable, it fails on sites with lower memory limits. Admins can temporarily turn up memory when enabling the modules, but that is sorta a janky solution.

I had the same issue with D7. I wanted to install only the commerce module. On a clean install of CentOS in VirtualBox and Drupal, I downloaded the follwoing modules (to fulfill commerce module dependencies):

• addressfield
• commerce
• ctools
• devel
• entity
• rules
• views

And I tried to install all at once by checking:

• Cart
• Checkout
• Commerce
• Commerce UI
• Customer
• Customer UI
• ...
• Rules
• Rules UI
• Views
• Views UI

practically all modules I need for my enviroment. Drupal redirected me for dependencies like CTools, etc confirmation page and after confirmation: php timed out letting me with modules partially installed and not sure if those installed were installed correctly.

I think this is not right and if Drupal is using Batch for less critical task like content managing it should definitely use Batch for critical operations that can blow up the db or partially install modules that need manually be uninstalled, so i start digging on how drupal is installing it's modules on first install page. The attached patch almost replicate that behavior on modules form also.

Still, the system.test ModuleTestCases should be carefuly reviewed on module enable from UI part.

I also think that the uninstall form should also use batch (I'm not talking about disable form here that already use batch with this patch but uninstall form).

Reviewed and tested simpletests.

Let's keep this as a D8 issue, since marking it as D7 means that it will disappear from D8 radar. While it's fine to post D7 patches, those have very small chance to get into D7 core. Keeping this a D8 issue will give it a chance to actually get in core.

Should we open a diffrent issue for D7?

This would significantly help with #2494987: [meta-6] Reduce cold cache memory requirements.

Marking for D7 backport, based on #29/#30.

A plain reroll was impossible because of the major changes in HEAD since the last patch. Attached patch is a full rewrite of the previous patch.

This should fix some failing tests.

The batch is now created the same way when installing an install profile or enabling modules (with dependencies) from the UI. This should also fix most of the failing tests.

Fixed another failing test, slowly getting there.

I'm going to have to admit defeat where Drupal\locale\Tests\LocaleUpdateTest is concerned. I'll explain my findings so far.

locale_system_update gets triggered after enabling a module and then sets two batches. One to update the project translations and one to update the configuration translations. Both batches require a different include file to be loaded. The warnings are caused by the way batch api handles the addition of multiple new batches while already running a batch. Both batches are added, but the 'file' arguments get switched around causing the wrong include file to be loaded when the batch is run. This only happens when setting the batches from a running batch and should receive it's own bug report in my opinion. The installer gets around this problem by implementing it's own translation import code, which (unfortunately) is unusable for our purpose because it's very tightly coupled to other installer code.

I got around the warnings by cross-requiring the include files when they are loaded, which feels like a really dirty hack, but at least the import code now runs. Unfortunately the test keeps failing, because the message being asserted in this assert doesn't get set.

// Check if translations have been imported.
    $this->assertRaw(t('One translation file imported. %number translations were added, %update translations were updated and %delete translations were removed.',
      array('%number' => 7, '%update' => 0, '%delete' => 0)), 'One translation file imported.');

I'm stuck for now and I hope someone else could look at this with a fresh pair of eyes. I attached the patch to see what testbot thinks about it.

High-level question, if a site had tons of languages enabled, isn't the queue system more appropriate for downloading translations on operations that can cause dangerous problems if modules do not get enabled/disabled correctly?

Moving this to 8.2.x

Is this still a problem in D10 or even D7? If not, then we should close it or reassign to D7.