Problem/Motivation

In the ModuleInstaller we load the .module file prior to changing the class loader to allow classes from the module to autoloaded. This causes problems when we trying to deprecate constants defined in .module files in favour of constants in class file. See #3075703-45: Move search text processing to a service

Proposed resolution

Load .module and .install files after fixing the autoloader.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
1.58 KB

I'm sure I've made this change on another issue somewhere... but let's see what breaks.

I tried this without the $this->moduleHandler->load($module); but we still need that because the .module files are not loaded by \Drupal\Core\DrupalKernelInterface::updateModules().

alexpott’s picture

Turns out the state of the module handler before updating the kernel is important. It probably should not be but let's not change that here.

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

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

+1 to RTBC. Looks good.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -210,6 +205,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
         $this->updateKernel($module_filenames);

updateModules() inside updateKernel initialize the container which means the new code loads the module with a handler with the current module list.

I believe, this is what we want instead of loading from the old/obsolete container.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Gonna some test coverage to one of the test modules for this.

alexpott’s picture

Here's a test. Discovered that all modules are autoloadable from the test-runner. This is an artifact of how we have to make test classes autoloadable. Other project don't do this. At some point we should stop doing that too. So in order to uncover the bug we need to do the install via the UI.

The last submitted patch, 7: 3144354-7.test-only.patch, failed testing. View results

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I looks ready++ just not sure about freeze!

  • catch committed 1a48485 on 9.1.x
    Issue #3144354 by alexpott, vijaycs85, andypost: ModuleInstaller loads ....

  • catch committed e0abc7a on 9.0.x
    Issue #3144354 by alexpott, vijaycs85, andypost: ModuleInstaller loads ....
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!

  • catch committed 200cef5 on 8.9.x
    Issue #3144354 by alexpott, vijaycs85, andypost: ModuleInstaller loads ....
dww’s picture

I was super excited to find this issue (thanks to @larowlan via Slack) since I thought it might solve a very old Update Manager bug: #2350711: Update module fails on installation -- sadly, this patch isn't sufficient. :( On a totally clean 8.9.1 Pantheon test site, even if with this patch applied, after installing the site via install.php and checking the 'Enable update notifications' checkbox, once you're logged into the live site and try to use various Update manager pages (e.g. the settings form) you get a WSOD due to Class "\Drupal\update\UpdateSettingsForm" does not exist. ModuleInstaller::install() jumps through all kinds of hoops to rebulid the service container, but the autoloader itself doesn't seem to kick in and find the classes. If you clear all caches (rebuilding the full container), everything starts working fine.

Anyone from here available to take a look over there and offer any insights? ;) 🤞

Thanks!
-Derek

Status: Fixed » Closed (fixed)

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

quietone’s picture

Issue tags: +Bug Smash Initiative

Closed #2843457: Provide PSR4 namespaces before loading the module code in ModuleInstaller as a duplicate, adding credit for the work in that issue which was an identical fix, though without a test.