Problem/Motivation
In an install profile's .info.yml file the dependencies
behaves differently than a module's key of the same name. For an install profile it is a list of modules to install when the install profile is installed and not a dependency of the install profile. This means that the modules listed here can be uninstalled. For example, if you use the Standard install profile you can still uninstall the Contact module.
This works great for starter kit install profiles but creates problems for distributions that want to be more than that as every upgrade hook they write has to contend with the fact that the the ground has completely shifted from underneath them.
Proposed resolution
Add a new key to install profile's info.yml files: install
. This key is a list of modules to install but the install profile does not depend on them. If the install profile also has a dependencies
key then these are treated as real dependencies that can not be uninstalled because the install profile is required.
Since we don't want to affect existing install profiles unless they opt in we need a BC layer. The BC layer moves all an install profile dependencies to the install key if the install key is not set.
An advantage of this new approach is that the install profile special casing in ModuleInstaller and ModulesUninstallForm has been removed.
This is a different issue from #820054: Add support for recommends[] and fix install profile dependency special casing because I think the recommends key is something different than the list of modules that should be installed when installing an install profile.
Remaining tasks
- Write a change record
- Find documentation on drupal.org that will need updating
- Find documentation in codebase that will need updating.
User interface changes
None
API changes
New key for install profile's to use in their info.yml file
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff-2952888.txt | 1.83 KB | dawehner |
#25 | 2952888-25.patch | 26.12 KB | dawehner |
#10 | 2952888-10.patch | 26.12 KB | alexpott |
#10 | 6-10-interdiff.txt | 7.96 KB | alexpott |
#6 | 2952888-6.patch | 20.97 KB | alexpott |
Comments
Comment #2
alexpottComment #3
daniel.bosenThanks! This will make the life of distribution maintainers much easier. I do not see the problem of the "fact that the the ground has completely shifted from underneath them" completely go away, but at least in some cases it will get reliable.
As the maintainer of a distribution, that is trying to provide updates between releases, I have to add the concern, that people do not like to be able to disable modules. It was actually the biggest complaint we had in the first versions of Thunder, when we enforced several modules by other means.
In the end we will probably only make very few modules as hard dependencies and will still always be cautious about checking the ground we think we stand on.
I would actually love to hear experiences of other distribution maintainers about this topic.
About the specific implementation: I like the idea of having the new
install
key. I am not thrilled about the fact, that the meaning of thedependencies
key depends on the existence of theinstall
key. But I have no better idea how to keep BC :-/Comment #5
alexpottFixing the tests and also handling the case where you want only dependencies in your profile. In D8 to accommodate the BC layer you'd need to add an empty install key.
Comment #6
alexpottImproved docs and tests.
Comment #7
alexpottNotice how this documentation does not even need changing :)
Comment #8
phenaproximaThis looks pretty thorough.
Maybe this should have a @todo marking it for removal in Drupal 9?
Small typo: "modules" should be "module". Either that, or the "a" should be removed.
Comment #9
phenaproximaChange record drafted: https://www.drupal.org/node/2952947
Comment #10
alexpottThanks for the review and change record @phenaproxima.
Re #8.1 - introduced an
@trigger_error
#8.2 fixed.
Comment #11
bircherThis looks good to me.
The scope of this issue is very clear and narrow.
For the case of re-installing from configuration further things need to be done to standard and demo_umami in their install hooks. But that is outside of the scope of this issue.
This is a great addition for distributions and makes it easier to to use the profile as opposed to using a module that depend on all the distributions required modules. It also makes it clearer what is the distributions responsibility as you would now have to fork the distribution if you want to uninstall modules that the distribution sees as essential. For optional modules of the distro we still have to do the dance and check the situation in update hooks.
Comment #12
phenaproximaThis sounds like follow-ups are needed.
Comment #13
neclimdulSo is this saying any use of dependencies is deprecated? I came here thinking dependencies with the presence of an install entry would behave like module dependencies. Moving toward profiles being consistent with our other "things"
If that where the case we wouldn't "deprecate" the behavior but just have a TODO to remove the BC behavior and document that you need the install key to use dependencies like a... dependency.
Comment #14
alexpott@neclimdul no it is not saying that at all. If your install profile has both an install and dependencies key then your dependencies will be real dependencies.
Here is the full context. The deprecation is only triggered if there is no install key.
Comment #15
alexpottAnd yes if an install profile has both a dependencies key and an install key then the dependencies are real and depend on the install profile.
Comment #16
alexpottComment #17
neclimdulOk, yeah I read it in context I was miss-understanding the code I guess. +1 awesome work!
Comment #18
jibranIs there a reason why we are not adding an upgrade path for this?
Comment #19
alexpott@jibran because there is nothing to upgrade. You can't upgrade .info.yml files. There is a BC layer and there is a deprecation notice if you don't have an install key in your profile's info.yml.
Comment #21
MixologicTemporary testbot hiccup.
Comment #22
larowlanouch
This looks good
Comment #23
alexpott@larowlan fortunately thats only for the deprecation so will not be there in Drupal 9 :)
Comment #24
larowlanI'd like to get a review on this from a subsystem maintainer
Comment #25
dawehnerI really like the new name! This describes it quite well.
Here are just two small adaptions I made.
For future work ... It would be super nice if install state would be a service one could access.
Comment #27
alexpottUnrelated test fail.
Comment #28
alexpott@larowlan there is no official subsystem maintainer for the installer
from MAINTAINERS.txt
I guess you could say that @dawehner and I are because we've done the most work on it recently.
Comment #30
alexpottUnrelated test fail.
Comment #31
larowlanAdding a review credit for @phenaproxima for review/authoring change notice.
Comment #33
larowlanCommitted as b607eb2 and pushed to 8.6.x.
Published change record.