Problem/Motivation

Modules specified in $modules currently get installed separately from modules in the install profile. This means two calls to ModuleInstaller::install() where it could be one instead.

Steps to reproduce

Proposed resolution

Add 'extra_modules' support to the installer, then set modules from the class property to that.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3614825

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

catch created an issue. See original summary.

catch opened merge request !16539

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs work
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Fixed the last dozen or so remaining test failures.

Ended up adding default theme support to the installer too, since otherwise I think we'd have to rebuild the container whenever a non-stark theme is installed via the class property or similar. Doing this fixed all the tests using the starterkit theme via a class property.

A handful of test modules don't specify dependencies (because they were only ever installed after the module they depend on), so added those - straight bug in the test modules.

And a couple of other fixes which I've self-reviewed in the MR.

There is non-zero test fallout from this change but overall it's not very much in the scheme of things.

nicxvan’s picture

nicxvan commented

I think we need a CR here for the deprecation and we need to update the deprecation links.

I think a follow up for the core/modules/views/tests/src/Functional/Plugin/NumericFormatPluralTest.php rebuild is probably enough, I suspect your suspicion is correct and it's probably in a test module.

catch’s picture

catch
he/him
Primary language English
commented

Added a CR here: https://www.drupal.org/node/3616201

catch’s picture

catch
he/him
Primary language English
commented

Updated the deprecations to point to the CR. Also opened #3616202: Remove ::rebuildContainer() call in NumericFormatPluralTest.

catch’s picture

catch
he/him
Primary language English
commented

Cherry-picked a commit from @mstrelan based on #3616202-5: Remove ::rebuildContainer() call in NumericFormatPluralTest, that's good to narrow down the workaround to the minimum.

catch’s picture

catch
he/him
Primary language English
commented

Opened #3616642: Path alias module can't be installed in the same batch as another module that requests a route, although having opened it, I'm not sure it's possible to fix that without adding installer-specific code, which would potentially be worse than the try/catch, but we might find a different approach in the issue.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

I think this is ready now, this is for the testing framework and should save significant CPU time.

There is the one workaround that is a bit messy, but it's pretty minimal and I think the gains on the testing framework is worth it.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs review

Have some minor questions on the MR.

Also, do we need a follow up for the path alias/route thing, or is that likely only something the test module runs into?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

The test module explicitly tests the case where the module generates a link (e.g. in hook_install()) before the database is ready. It's possible for contrib modules to do this so theoretically another module can cause the same issue, I'm personally not convinced we should support it at all. #3616642: Path alias module can't be installed in the same batch as another module that requests a route is already open for the follow-up.

I think I was able to resolve the rest of the feedback. All small changes so moving back to RTBC.

godotislate’s picture

godotislate
he/him
commented

I think the commit to change to DatabaseException might have not been pushed, so I just added and applied suggestions.

I also rebased just in case, since a few other things have just been committed.

catch’s picture

catch
he/him
Primary language English
commented

Ooops sorry, a bit of multi-tasking going on...

https://git.drupalcode.org/project/drupal/-/jobs/11763921 got through all functional tests in 1m3s which is better than I was getting testing the stacked MRs. Lots of variation with the other jobs but getting towards where it should be.

  • godotislate committed 6a9373d7 on main 
    task: #3614825 Allow additional modules to be installed alongside the...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed 6a9373d and pushed to main. Thanks!

Published the CR.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

This made tests shipping with CKEditor editor config crash when setting up the test, as the modules get installed with the profile before the theme, but the installed module alerady tries to load the CSS from the theme that does not exist. :) Core does not have one such test, but Canvas has 40 or so. See #3403394: CKEditor 5 throws an exception when the default theme is not installed yet which was a 2023 issue but is now higher significance IMHO.