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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

daniel.bosen’s picture

Thanks! 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 the dependencies key depends on the existence of the install key. But I have no better idea how to keep BC :-/

Status: Needs review » Needs work

The last submitted patch, 2: 2952888-2.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.14 KB
17.31 KB

Fixing 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.

alexpott’s picture

Improved docs and tests.

alexpott’s picture

+++ b/core/includes/install.inc
@@ -1039,6 +1039,8 @@ function drupal_check_module($module) {
  * - dependencies: An array of shortnames of other modules that this install
  *   profile requires.

Notice how this documentation does not even need changing :)

phenaproxima’s picture

This looks pretty thorough.

  1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -31,6 +31,13 @@ public function parse($filename) {
    +      // Special BC handling.
    +      if ($parsed_info['type'] === 'profile' && isset($parsed_info['dependencies']) && !array_key_exists('install', $parsed_info)) {
    +        // Move dependencies to install so that if a profile has both
    +        // dependencies and install then dependencies are real.
    +        $parsed_info['install'] = $parsed_info['dependencies'];
    +        $parsed_info['dependencies'] = [];
    +      }
    

    Maybe this should have a @todo marking it for removal in Drupal 9?

  2. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
    @@ -200,6 +201,45 @@ public function testUninstallProfileDependency() {
    +   * Tests uninstalling a modules installed by a profile.
    

    Small typo: "modules" should be "module". Either that, or the "a" should be removed.

phenaproxima’s picture

Issue tags: -Needs change record
alexpott’s picture

Thanks for the review and change record @phenaproxima.
Re #8.1 - introduced an @trigger_error
#8.2 fixed.

bircher’s picture

Status: Needs review » Reviewed & tested by the community

This 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.

phenaproxima’s picture

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 sounds like follow-ups are needed.

neclimdul’s picture

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -31,6 +31,24 @@ public function parse($filename) {
+          if (preg_match($pattern, $filename)) {
+            @trigger_error("The install profile $filename only implements a 'dependencies' key. As of Drupal 8.6.0 profile's support a new 'install' key for modules that should be installed but not depended on. See https://www.drupal.org/node/2952947.", E_USER_DEPRECATED);
+          }

So 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.

alexpott’s picture

@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.

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -31,6 +31,24 @@ public function parse($filename) {
+      // Special backwards compatible handling profiles and their 'dependencies'
+      // key.
+      if ($parsed_info['type'] === 'profile' && isset($parsed_info['dependencies']) && !array_key_exists('install', $parsed_info)) {
+        // Only trigger the deprecation message if we are actually using the
+        // profile with the missing 'install' key. This avoids triggering the
+        // deprecation when scanning all the available install profiles.
+        global $install_state;
+        if (isset($install_state['parameters']['profile'])) {
+          $pattern = '@' . preg_quote(DIRECTORY_SEPARATOR . $install_state['parameters']['profile'] . '.info.yml') . '$@';
+          if (preg_match($pattern, $filename)) {
+            @trigger_error("The install profile $filename only implements a 'dependencies' key. As of Drupal 8.6.0 profile's support a new 'install' key for modules that should be installed but not depended on. See https://www.drupal.org/node/2952947.", E_USER_DEPRECATED);
+          }
+        }
+        // Move dependencies to install so that if a profile has both
+        // dependencies and install then dependencies are real.
+        $parsed_info['install'] = $parsed_info['dependencies'];
+        $parsed_info['dependencies'] = [];
+      }

Here is the full context. The deprecation is only triggered if there is no install key.

alexpott’s picture

And 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.

alexpott’s picture

Issue summary: View changes
neclimdul’s picture

Ok, yeah I read it in context I was miss-understanding the code I guess. +1 awesome work!

jibran’s picture

Is there a reason why we are not adding an upgrade path for this?

alexpott’s picture

@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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2952888-10.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Reviewed & tested by the community

Temporary testbot hiccup.

larowlan’s picture

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -31,6 +31,24 @@ public function parse($filename) {
+        global $install_state;
+        if (isset($install_state['parameters']['profile'])) {

ouch

This looks good

alexpott’s picture

@larowlan fortunately thats only for the deprecation so will not be there in Drupal 9 :)

larowlan’s picture

I'd like to get a review on this from a subsystem maintainer

dawehner’s picture

+++ b/core/includes/install.core.inc
@@ -1085,7 +1085,7 @@ function install_base_system(&$install_state) {
-  $modules = $install_state['profile_info']['dependencies'];
+  $modules = $install_state['profile_info']['install'];

I really like the new name! This describes it quite well.

Here are just two small adaptions I made.

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -31,6 +31,24 @@ public function parse($filename) {
+        if (isset($install_state['parameters']['profile'])) {

For future work ... It would be super nice if install state would be a service one could access.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2952888-25.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test fail.

alexpott’s picture

@larowlan there is no official subsystem maintainer for the installer

Installer
- ?

from MAINTAINERS.txt
I guess you could say that @dawehner and I are because we've done the most work on it recently.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2952888-25.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test fail.

larowlan’s picture

Adding a review credit for @phenaproxima for review/authoring change notice.

  • larowlan committed b607eb2 on 8.6.x
    Issue #2952888 by alexpott, dawehner, phenaproxima: Allow install...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as b607eb2 and pushed to 8.6.x.

Published change record.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.