Problem/Motivation

After the introduction of ModuleInstaller::invoke() the ModuleHandler::add* method family likely belongs to ModuleInstaller, if at all. In the current state it does not accurately represent what users may expect.
It picks up procedural implementations only.

This is because OOP implementations would need a container rebuild.

ModuleHandler::add() is protected, it is called by ModuleHandler::addProfile() and ModuelHandler::addModule().

These two in turn are only called here:
install.core.inc

// Override the module list with a minimal set of modules.
  $module_handler = \Drupal::moduleHandler();
  if (!$module_handler->moduleExists('system')) {
    $module_handler->addModule('system', 'core/modules/system');
  }
  if ($profile && !$module_handler->moduleExists($profile)) {
    $module_handler->addProfile($profile, $install_state['profiles'][$profile]->getPath());
  }

Also in authorize.php

// We have to enable the user and system modules, even to check access and
// display errors via the maintenance theme.
\Drupal::moduleHandler()->addModule('system', 'core/modules/system');
\Drupal::moduleHandler()->addModule('user', 'core/modules/user');
\Drupal::moduleHandler()->load('system');
\Drupal::moduleHandler()->load('user');

// Initialize the maintenance theme for this administrative script.
drupal_maintenance_theme();

It is called in several tests:

  • core/tests/Drupal/KernelTests/KernelTestBase.php
  • core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
  • core/tests/Drupal/Tests/UpdatePathTestTrait.php

Steps to reproduce

N/A

Proposed resolution

Replace calls to addModule() and addProfile() in core.
Deprecate addModule() and addProfile().
Mark add() for deletion when addModule() and addProfile() are removed in Drupal 12.

Remaining tasks

N/A

User interface changes

N/A

Introduced terminology

N/A

API changes

Do not call addModule() or addProfile()

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3481778

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

nicxvan created an issue. See original summary.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented
Issue summary: View changes
Parent issue: » #3442009: OOP hooks using attributes and event dispatcher
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Related issues: +#3487971: Convert system_page_attachments to OOP

nicxvan opened merge request !10262

nicxvan opened merge request !10365

nicxvan’s picture

nicxvan commented
Title: Investigate ModuleHandler::add » Deprecate functions using ModuleHandler::add
nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan changed the visibility of the branch 3481778-investigate-modulehandleradd to hidden.

nicxvan’s picture

nicxvan commented
Status: Active » Needs review
daffie’s picture

daffie commented
Status: Needs review » Needs work

Back to NW for the 2 remarks on the MR.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I think I answered your concerns, let me know if you have any further questions.

daffie’s picture

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

My questions have been answered.
All code changes look good to me.
The IS and the CR are in order.
For me it is RTBC.

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Needs work

Thanks for working on this!
GitLab thinks this needs a rebase due to conflicts.
Meanwhile, I opened some MR threads, mostly with pedantic nits.

nicxvan’s picture

nicxvan commented

Yeah I saw the conflict but when I went to rebase it was gone, maybe an issue was reverted I'll fix it and address your comments.

nicxvan’s picture

nicxvan commented

Rebase is werd I'll look tomorrow

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Rebased and comments addressed.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work
Related issues: +#3491731: [META] Remove the ability to update modules and themes via authorize.php

The functional test fails in https://git.drupalcode.org/issue/drupal-3481778/-/pipelines/367140 are random, but the fail in Unit is legit. Opened MR threads with suggestions.

Almost there, thanks!
-Derek

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
dww’s picture

dww
we/he/they
commented
Title: Deprecate functions using ModuleHandler::add » Deprecate functions using ModuleHandler::add()
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Whereas:

  1. All feedback addressed.
  2. Pipeline 367282 is green.
  3. This is my first time experimenting with requesting changes. Even though I've got somewhat elevated perms in GitLab MRs for core as a subsystem maintainer, I can't actually resolve that request (even though I made it myself). Good to know, I won't be using that feature anymore. The committer can ignore the "Merge blocked: 1 check failed" error on the MR.
  4. Tweaked some formatting in the summary.
  5. Initial pass at saving credit.
  6. I don't mind RTBC'ing this, even though I pushed a trivial commit to update the formatting of the deprecation message.

Therefore, RTBC!

Thanks again,
-Derek

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
longwave’s picture

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

This looks like a nice cleanup, added a couple of minor questions to the MR.

nicxvan’s picture

nicxvan commented

I replied to the comments that I could not address.

nicxvan’s picture

nicxvan commented
Related issues: +#3496970: Explore changing DrupalKernel::getExtensions(): array|false to always return an array

Follow up created and all comments addressed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record updates

Resolved all threads

When looking at the CR though it wasn't super clear, maybe something like

ModuleHandler::addModule and ModuleHandler::addProfile have been deprecated. There is no direct replacement. (can the reason why be mentioned?)

Adding modules or profiles run time is not supported. Use ModuleInstallerInterface::install instead.

before/after implementations or least an after of how one would use ModuleInstallerInterface::install

nicxvan’s picture

nicxvan commented
Issue tags: -Needs change record updates

Updated the CR.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Thanks!

CR reads much better. I believe all feedback for this one is complete. Fingers crossed.

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

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record updates

I left some questions in the MR. The change record before section should be for 'addModule' and 'addProfile' not 'add'. Tagging for updates.

oily’s picture

oily commented

@quiteone RE: #30, updated the CR.

oily’s picture

oily commented

Dealt with 2 of 3 review comments in the MR. Rebased. The 3rd review comment involves a code comment. I am not sure what the coder's reason for the comment was. Hopefully they can deal with that review comment.

nicxvan’s picture

nicxvan commented

I will revert one of your changes you removed the deprecation on module handler add.

I'll address the remaining comment too it just needs to be more imperative.

oily’s picture

oily commented

@nicxvan Ah, I see. Thank you for fixing.

nicxvan’s picture

nicxvan commented

@quietone, I don't think we can change the return type since it's inherit doc and the parent has return type of array|false?

Is that ok?

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

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

nicxvan’s picture

nicxvan commented
nicxvan’s picture

nicxvan commented

I rebased on head, the last comment was addressed upstream so this should be ready.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates +Needs Review Queue Initiative, +11.2.0 release priority

CR does appear to be updated so removing that tag.

Checked the MR and all threads do appear to be addressed, even went back and read some older ones.

Feel this could be considered a 11.2 release priority (remove if I'm wrong)

Believe this one is good to go.

catch’s picture

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

This looks good but one small question.

nicxvan’s picture

nicxvan commented

This should be ready now.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs review » Reviewed & tested by the community

Feedback addressed, and I gave it a once over myself, this looks good to go.

  • catch committed 8b5a3ff1 on 11.x 
    Issue #3481778 by nicxvan, oily, dww, quietone, smustgrave, daffie,...
catch’s picture

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

I'm not sure it's necessary. The only way I could see it being an issue is in the later stages of installation if a profile changed the weight of the system module in a hook_system_info_alter maybe? Which seems like a really bad idea. Could go with the following I guess, just to be sure.

Yeah I agree it'd be a horrible idea, but it just made me think about it whereas conditionally setting it doesn't.

Looks good. Committed/pushed to 11.x, thanks!

catch closed merge request !10365

Status: Fixed » Closed (fixed)

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

donquixote’s picture

donquixote commented
Related issues: +#3528899: ModuleHandler::add() removes implementations from other modules

The ModuleHandler::add() is more broken than we thought.

It picks up procedural implementations only.

This is the least part of the problem.
Much worse is that it removes existing implementations, and prevents them from being added later.
See #3528899: ModuleHandler::add() removes implementations from other modules.

nicxvan closed merge request !10262