Problem/Motivation
If a module's hook_requirements() returns an error during the install phase, it is supposed to show an error message and return to the modules page.
When installing an experimental module, an extra confirmation form is added, warning about the fact that it's experimental.
When installing an experimental with a requirements error, you get both!
Thankfully the "Continue" button is a no-op, and you end back on the modules page.
But this is very confusing and scary.
Proposed resolution
Ensure the error takes precedence and the confirmation step doesn't happen.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#20 | 2833462-2-20.patch | 4.57 KB | alexpott |
#20 | 18-20-interdiff.txt | 828 bytes | alexpott |
#18 | 2833462-experimental-18.patch | 4.58 KB | tim.plunkett |
#18 | 2833462-experimental-18-interdiff.txt | 1.1 KB | tim.plunkett |
#13 | 12-13-interdiff.txt | 1.41 KB | alexpott |
Comments
Comment #2
alexpottHere's a test and a fix.
Comment #3
alexpottComment #4
tim.plunkettI came up with the same fix locally, +1!
80 chars. Maybe just "Module to test..."?
Missing trailing comma
Those could be fixed on commit. Ran the tests locally and failed/passed as expected.
Comment #5
alexpott@tim.plunkett thanks for the review.
Comment #9
alexpottInstallUninstallTest you are a fun one :)
Comment #12
alexpottOkay now for real. Just making the module hidden.
Comment #13
alexpottWrong .info.yml file... omg.
Comment #16
tim.plunkettExperimental modules are denoted via the package
Testing modules are also denoted via the package
Therefore, you cannot have an experimental test module.
The solution was to use
hidden: true
to fake the testing part.But then it's not in the UI to enable, making it untestable.
Comment #17
BerdirI'd love to see an explicit experimental: true flag in info.yml, so that contrib can have experimental modules too.
Testing modules are usually identified by being in a /tests/.. path, I think that test is the only thing that does the package test, maybe we should change that? Testing modules used to be hidden before we added the logic about not parsing /tests/ for modules unless a certain flag is set. Which is obviously set while running tests.
Comment #18
tim.plunkettSomething else is hiding experimental test modules, because we already have one!
experimental_module_test is a module, and it certainly doesn't show up on the modules page.
Interdiff against #9
The only thing we needed is a hook_help, borrowed from experimental_module_test.
Comment #19
alexpott\Drupal\Core\Extension\Discovery\RecursiveExtensionFilterIterator::acceptTests() is responsible for the experimental modules not being displayed. #18 looks good.
Comment #20
alexpott@xjm noticed a few things on review.
Comment #22
xjmI'm also totally in favor of this; we discussed it for somewhere. I noticed that there is already a "Chaos Tools Suite (Experimental)" package to try to accomplish the same thing we do for core, but of course it does not provide the requirements warning and confirmation that core does.I thought there was an existing issue for it but it does not seem to be listed on #2649768: [meta] No definition of "Experimental" & not nearly enough warning at least. So let's explore that in a followup. (Using the package name alone was the minimum hack we could do to get it working in the last months before 8.0.0 without any API change/addition).
I also ran into this when adding the experimental module confirm form. At the time, I think I investigated why the test used the "Testing" package name instead of "test" in the path or another strategy . This dates back to #1871328: Modules in new Multilingual package are no longer tested by Module\EnableDisableTest. Before the issue that introduced that regression, there was only the core package and the testing package, and the testing profile and test module discovery had not been introduced yet.
That said, though, when I was working on #2649768: [meta] No definition of "Experimental" & not nearly enough warning I did rely on on InstallUninstallTest to also provide test coverage for the experimental modules. So if we change InstallUninstallTest to skip them, we should also make sure that there is other coverage for the cases in that test.
TLDR, also followup material.
I asked @tim.plunkett why we are using this state check when we only explicitly test the TRUE case. This is because of the above bit where InstallUninstallTest skips the testing package, but experimental test modules can't be in the testing package because they are in the experimental package.
I thought of asking for an inline comment in case someone else had the same question, but it's not worth it for a test module IMO. After all, if someone tries to remove this, InstallUninstallTest will explain itself by failing. ;)
I committed this to 8.3.x, but we are in a commit freeze at the moment on 8.2.x, so leaving RTBC for backport tomorrow.
Thanks everyone!
Comment #24
xjmBackported to 8.2.x. Thanks!