Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
Part of #2649768: [meta] No definition of "Experimental" & not nearly enough warning. When you enable an experimental module, you have no warning or indication that it is a risky thing to do.
Proposed resolution
- Redirect to a confirmation form when enabling an experimental module.
- Use the same form when any dependency is a required module.
@webchick was in favor of this approach when we discussed it previously.
Remaining tasks
- Drush still provides no specific warning when the experimental module is enabled. Followup PR to provide some warning in Drush?
$ drush en migrate The following extensions will be enabled: migrate Do you really want to continue? (y/n): y migrate was enabled successfully.
- Add a method for checking whether a module is experimental. Probably would need to be a followup for #2657160: Allow extension objects to know their package, etc..
User interface changes
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#29 | Screen Shot 2016-02-09 at 14.07.03.png | 111.87 KB | alexpott |
#26 | interdiff-26.txt | 1.15 KB | xjm |
#26 | 2663796-26.patch | 17.66 KB | xjm |
#25 | interdiff-25.txt | 3.43 KB | xjm |
#25 | 2663796-25.patch | 17.65 KB | xjm |
Comments
Comment #2
xjmComment #4
xjmWell that is promising, and indicates a potential spot for test coverage. ;)
Comment #5
xjmHm 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 inInstallUninstallTest
. So the attached just makesInstallUninstallTest
pass. Or at least I think it should. I'm not runningInstallUninstallTest
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 addingExtension::isExperimental()
, but as per #2657160: Allow extension objects to know their package, etc. that would currently require adding a dependency onsystem_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.
Comment #6
xjmForgot 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.
Comment #7
xjmLooks like no double-escaping (I guess they are not translated). So the @todo can be removed.
Comment #9
xjmSo given the intent of the current test that needs to be updated:
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.
Comment #10
xjmBTW (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:But such obscure special casing seems to me to reduce maintainability. Thence a new test.
Comment #11
xjmWrong title.
Comment #13
xjmOkay, 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.Comment #14
xjmJust had my
elseif
the wrong way round.Comment #15
xjmTypo: "indicatng".
Comment #16
xjmComment #17
xjmComment #18
xjmAttached adds the tests and fixes the typos. (Full interdiff is the combination of the test-only patch and the text file.)
Comment #20
xjmI 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().
Comment #21
alexpottI think it is possible to call the parent here and keep it in sync.
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.Unused.
inheritdoc
Missing visibility
Comment #22
xjmThanks @alexpott.
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.
@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.
Comment #24
alexpottI think we should put this in 8.1.x first and then do
_
's for backport if we want to backport it.How come this isn't just part of _buildMessageList()?
Comment #25
xjmWe 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.
I guess because I started solving the problem in a different way at first.
Both fixed in attached. Thanks @alexpott.
Comment #26
xjmFixing incorrect indentation.
Comment #27
xjmForgot 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.Comment #28
xjmComment #29
alexpottI considered whether or not this question should change when there are required modules too. Discussed with @xjm, who having tried exactly that, found it to add too much text to the screen. Given that the item list contains exactly what is happening I think this is okay.
I've manually tested this and it looks good.
Comment #31
catchCouldn't find anything to complain about with the patch, and this will potentially save people from some horrible experiences.
Committed/pushed to 8.1.x, thanks!
Comment #32
Wim Leers+1, this makes a ton of sense.
I do think that the risk varies by experimental module to experimental module though. I think it'd be valuable if it were possible for a specific experimental module to be able to offer a
risks: 'The risks of this experimental module are: X, Y, Z.'
declaration in their*.info.yml
file. Though perhaps that's a bit too much. This generic warning is probably sufficient.Comment #33
xjmThanks @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.Comment #34
Wim LeersBut you only see the help after enabling the module.
I'm saying we need more nuanced information when considering to enable an experimental module.
Comment #35
xjm@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.
Comment #36
xjm@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.