Problem/Motivation

It became possible to have themes depend on modules in #474684: Allow themes to declare dependencies on modules. In this implementation, the dependee-modules must be enabled in admin/modules before a module-depending theme can be enabled.
Ideally, it would be possible to enable the dependent modules as part of enabling the theme. This was not part of the initial implementation due to the complexities of permissions and confirmation forms when both themes and modules are being enabled.
A partial implementation was added in an earlier iteration of the patch #474684: Allow themes to declare dependencies on modules, once it was apparent that additional validation (including experimental modules) was needed, it was decided that this was better addressed in a followup.

Proposed resolution

Determine how to best implement module-enabling validation in both admin/modules and when enabling module-dependent themes.

Then implement that.

Remaining tasks

Ensure modules that enabled themes depend on cannot be uninstalled.
Handle forms that extend the newly deprecated: ModulesListConfirmForm

User interface changes

...

API changes

...

Data model changes

...

Release notes snippet

...

CommentFileSizeAuthor
#26 interdiff_25-26.txt771 bytesbnjmnm
#26 3100374--26.patch63.25 KBbnjmnm
#25 3100374-25.patch63.25 KBbnjmnm
#24 drupal-n3100374-24.patch59.94 KBdamienmckenna
#23 drupal-n3100374-23.patch59.92 KBdamienmckenna
#21 3100374-nr-bot.txt150 bytesneeds-review-queue-bot
#13 3100374-13.patch62.77 KBmirom
#11 interdiff_9-11.txt5.71 KBbnjmnm
#11 3100374-11.patch62.77 KBbnjmnm
#9 interdiff_6-9.txt6.9 KBraman.b
#9 3100374-9.patch62.83 KBraman.b
#6 3100374-6.patch62.4 KBbnjmnm
#6 interdiff_5-6.txt21.48 KBbnjmnm
#5 3100374-5.patch50.45 KBbnjmnm

Issue fork drupal-3100374

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bnjmnm created an issue. See original summary.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Issue summary: View changes
Status: Postponed » Active

#474684: Allow themes to declare dependencies on modules has been committed so this can be set to Active.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

saschaeggi’s picture

saschaeggi
he/him
commented

This would also be very useful for other admin themes like Gin or Adminimal to automatically enable the toolbar helper module for the frontend.

So +1 on this.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3100374-5.patch50.45 KB

This adds the ability to enable modules via the theme installer, and test coverage has been modified to confirm this works. Additional tests are still needed for the new criteria that can trigger a confirmation form on module install - experimental modules or indirect dependent modules.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue tags: -Needs tests
StatusFileSize
new interdiff_5-6.txt21.48 KB
new 3100374-6.patch62.4 KB

Tests added

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented

This would be awesome and the missing bit of enabling a theme with modules, also a much more convenient UX. So +1

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

raman.b’s picture

raman.b commented
StatusFileSize
new 3100374-9.patch62.83 KB
new interdiff_6-9.txt6.9 KB

Created change records, updated deprecation messages for 9.2.x and resolved few CS issues.

  1. https://www.drupal.org/node/3188194
  2. https://www.drupal.org/node/3188195
saschaeggi’s picture

saschaeggi
he/him
commented

Glad so see some activity in here :)

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new 3100374-11.patch62.77 KB
new interdiff_9-11.txt5.71 KB

This addresses a few things surfaced by the committer test tools being made available to all contributors.

I also tried to address the Nightwatch test failure in #9 and I'm a bit confused as to what is happening. The claroAutocompleteTest is failing in a very early step: after the "FormAPI Test" module is enabled via the UI, the test expects a confirmation form for enabling the additional dependent modules. This is challenging as

  • When the test is run locally, there's no failure and I confirmed a confirmation form appears
  • I also manually tested on a fresh Drupal install with the nightwatch_testing profile, and the confirmation form also appears.

Although the local/manual tests work, the changes we've made here specifically target extension confirmation forms so it's entirely possible claroAutocompleteTest has surfaced an edge case that actually needs fixing, as opposed to this being a false alarm related to test config. I wasn't able to figure that out, but perhaps someone else can?

manuel garcia’s picture

manuel garcia commented
Related issues: +#3176625: Install from existing config with a theme that declares module dependencies fails
mirom’s picture

mirom commented
StatusFileSize
new 3100374-13.patch62.77 KB

This is just fixing the coding standard failure from the previous run.

bnjmnm opened merge request !523

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott made their first commit to this issue’s fork.

alexpott opened merge request !1634

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3100374-nr-bot.txt150 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Assigned: Unassigned » damienmckenna
Issue tags: +DrupalWTF

This is a definite DrupalWTF as theme builders should expect that listing a module as a dependency of their theme would make it work the same as on another module.

Working on a reroll.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-n3100374-23.patch59.92 KB

Rerolled, hopefully successfully; the only part I'm not 100% about are the changes to core/lib/Drupal/Core/Extension/ThemeInstaller.php, some refactoring had already been done there on another issue.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Assigned: damienmckenna » Unassigned
StatusFileSize
new drupal-n3100374-24.patch59.94 KB

Patch #23 was against 9.5.x. This patch is for 10.1.x

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new 3100374-25.patch63.25 KB
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
StatusFileSize
new 3100374--26.patch63.25 KB
new interdiff_25-26.txt771 bytes

Things have been deprecated in the time that passed since the last working patch ⏰⏰⏰⏰💀

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Seems there were some failures.

Love the idea though as I would make my themes require the components module.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Related issues: +#3215043: Indicate the non-stable statuses in admin/modules page

I believe some (maybe all) of the remaining test failures are due to #3215043: Indicate the non-stable statuses in admin/modules page. This made some nice changes to the module form experience, but will require the logic in this issue to be updated to account for the differences.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Our install profile has a base theme that requires modules, and we realized because of this issue, our install profile cannot be installed anymore because the installer installs themes before modules. I would say this is a bug more than a feature request at this point.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Category: Feature request » Bug report

Switching to bug per #30.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Related issues: +#3485462: Unable to install theme: '…' due to unmet module dependencies

ivnish made their first commit to this issue’s fork.

ivnish changed the visibility of the branch 11.x to hidden.

ivnish opened merge request !10901

ivnish’s picture

ivnish
he/him
commented

MR for Drupal 11 is 90% ready. Needs to fix some phpstan issues. I don't have time to deal with this task anymore.

martijn de wit’s picture

martijn de wit
Primary language Dutch
Location 🇳🇱 The Netherlands
commented

thank you for all the work @ivnish ! 🙏

nicxvan changed the visibility of the branch 3100374 to hidden.

nicxvan changed the visibility of the branch 3100374-make-it-possible to hidden.

nicxvan’s picture

nicxvan commented
nicxvan’s picture

nicxvan commented

I'll take a look.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

Addressed the phpstan issues in the new extensionconfirm form. Added some items to the remaining tasks.

Still needs work.

I think there might be something missing for uninstalling modules when a theme is enabled, I didn't look close enough. We do need to handle the forms extending the newly deprecated form, and most likely create new ones.

There are also things like this:
if (isset($theme) || $this->themeInstaller->install([$theme])) {
$theme is always set there, it used to be $themes[$theme] I've not read through the code enough to know why that changed.

nicxvan’s picture

nicxvan commented

Ok I fixed the CS errors.

I also undid the module confirm deprecation because extension confirmation doesn't cover what moduleslistunstableconfirmform needs.

This probably needs some additional work, but I think tests will run.

Edit: I'm not sure how to fix that last phpstan issue.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.