Use a confirmation form when enabling experimental modules

fail: [Other] Line 118 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
Modules status has been updated.

fail: [Other] Line 124 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
"hook_modules_installed fired for inline_form_errors" found

fail: [Other] Line 125 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
watchdog table contains 0 rows for inline_form_errors module installed.

Well that is promising, and indicates a potential spot for test coverage. ;)

Hm actually, InstallUninstallTest is not the place for test coverage -- it would only provide coverage so long as there were an experimental module in core. And it excludes test modules by design. So we need test coverage elsewhere using the experimental test module (which already exists), and should not duplicate that coverage in InstallUninstallTest. So the attached just makes InstallUninstallTest pass. Or at least I think it should. I'm not running InstallUninstallTest locally because it is a beast. :P I will give someone the 17 cents.

As an aside, I have done a lot of typing if ($package == 'Core (Experimental)') {. I've considered several times adding Extension::isExperimental(), but as per #2657160: Allow extension objects to know their package, etc. that would currently require adding a dependency on system_get_info(), so a little repeating myself seems better than that.

I also realized we don't need nearly so many words on the confirm form -- the earlier patch verged on overwhelming, and fewer words is usually better UX.

+++ b/core/modules/system/src/Form/ModulesListExperimentalConfirmForm.php
@@ -0,0 +1,69 @@
+    // @todo Are htmlspecialchars valid in module names? If so both this and
+    //   the parent class may have a double escaping bug.
+    $items[] = $this->t('The following modules are experimental: @modules', ['@modules' => implode(', ', array_values($this->modules['experimental']))]);

Forgot to mention -- if module names are translatable strings and if special chars are valid in them, this (as well as a similar line in the parent class) will potentially cause double escaping bugs. Need to check.

Looks like no double-escaping (I guess they are not translated). So the @todo can be removed.

So given the intent of the current test that needs to be updated:

    // Now that all modules have been tested, go back and try to enable them    
    // all again at once. This tests two things:                                
    // - That each module can be successfully enabled again after being         
    //   uninstalled.                                                           
    // - That enabling more than one module at the same time does not lead to   
    //   any errors.   

It didn't seem right to blindly click "Continue" on the form if it's testing for "no errors", so I added an assertion to make sure it's the right form. I also added a similar assertion earlier in the similar hunk.

Still need an explicit test.

BTW (since @alexpott will bring this up), I did consider whether it was possible to test this using only InstallUninstallTest. The issue with that is that the current design of the test relies on excluding hidden/test modules. We could do something like this:

     // Test help on required modules, but do not test uninstalling.
     $required_modules = array_filter($all_modules, function ($module) {
       if (!empty($module->info['required']) || $module->status == TRUE) {
-        if ($module->info['package'] != 'Testing' && empty($module->info['hidden'])) {
+        if (($module->info['package'] != 'Testing' && empty($module->info['hidden']) || $module->getName() == 'experimental_module_test') {
           return TRUE;
         }
       }

But such obscure special casing seems to me to reduce maintainability. Thence a new test.

Wrong title.

Okay, at this point I'm going to have to warm up my apartment by a few degrees. Maybe InstallUninstallTest will be done running by the time I wake up.

Just had my elseif the wrong way round.

+++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
@@ -100,14 +100,24 @@ public function testInstallUninstall() {
+          // indicatng they need to be enabled.

Typo: "indicatng".

Attached adds the tests and fixes the typos. (Full interdiff is the combination of the test-only patch and the text file.)

+++ b/core/modules/system/tests/modules/experimental_module_test/experimental_module_test.info.yml
@@ -4,4 +4,3 @@ description: 'Module in the experimental package to test experimental functional
-hidden: true

I should note that the hidden: true prevented the module from being testable on the form. I did test on a normal installation (no development mode) and confirm that the test module does not show up on the modules page (because test module discovery is not enabled).

I think that's probably fine, but could use feedback. The alternative is to hack some hook_system_info_alter().

Thanks @alexpott.

I think it is possible to call the parent here and keep it in sync.

Manipulating the render array felt fragile, so I did a little internal refactoring instead to add some protected methods and make it cleaner to override.

Discussed with @xjm and we agreed that it is a good idea to refactor to a redirectToConfirmForm() method that is used by both this and the dependencies check below. This is because it keeps the way the information is stored in key value the same.

@alexpott and I discussed this more. It didn't really make sense as a protected method because it was so specific to the code flow in the submit method, but it was actually possible to just clean up the logic a little instead.

@alexpott also seemed to think #20 was okay.

I think we should put this in 8.1.x first and then do _'s for backport if we want to backport it.

We can't backport this since it is a UI and expectation change, so I'll just remove the underscore. I was thinking about base classes since I am subclassing it, but this is just a controller, so per https://www.drupal.org/core/d8-bc-policy it can be changed in a minor.

How come this isn't just part of _buildMessageList()?

I guess because I started solving the problem in a different way at first.

Both fixed in attached. Thanks @alexpott.

Fixing incorrect indentation.

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -423,16 +433,16 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-    if (!empty($modules['dependencies']) || !empty($modules['missing'])) {
+    // Redirect to a confirmation form if needed.
+    if (!empty($modules['experimental']) || !empty($modules['dependencies'])) {

Forgot to mention earlier, @alexpott and I checked and $modules['missing'] is dead code. It is not set anywhere nor referenced in the confirmation form, and @alexpott confirmed that missing modules are handled elsewhere.

Thanks @Wim Leers! I think the linked handbook page and #2656994: Experimental modules should have their own version numbers should hopefully inform site builders on the range of risk for different modules. Beyond that, it's probably up to the individual module's hook_help() and handbook page to document specifics. Note that we might do some work on that front for Migrate in particular.

@wimleers, I still think that information belongs on the handbook page for the specific module in general -- same as it would for any contrib module you were evaluating. There's no reason to special case these particular modules to add more on an already crowded form.

That said, I also do want to have a section in the core release notes that provides a high-level overview of experimental modules in the minor.

@Wim Leers,but also this is maybe worth another child issue of #2649768: [meta] No definition of "Experimental" & not nearly enough warning if you have a different proposal.