Problem/Motivation

#2942769-2: Consolidate umami .htaccess files and testing. passed on 8.6.x but failed on 8.5.x because drupal_get_path('module', 'demo_umami_content') is successful on 8.6.x but not in 8.5.x when the testing profile is used. The 8.5.x behaviour is correct.

Proposed resolution

Ensure that a profile's modules are not available to other profiles.

Remaining tasks

User interface changes

None

API changes

Data model changes

None

CommentFileSizeAuthor
#11 2945306-11.patch2.37 KBalexpott
#11 8-11-interdiff.txt638 bytesalexpott
#8 2945306-8.patch2.38 KBalexpott
#8 7-8-interdiff.txt612 bytesalexpott
#7 2945306-7.patch2.25 KBalexpott
#7 4-7-interdiff.txt3.26 KBalexpott
#4 2945306-4.patch4.42 KBalexpott
#4 2945306-4.test-only.patch1.7 KBalexpott
#3 2945306-3.patch1.2 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

Okay so the good news is that this appears to be a testing only issue. After installing standard on 8.6.x I can't get the path to demo_umami_content module.

Psy Shell v0.8.0 (PHP 7.1.10 — cli) by Justin Hileman
>>> drupal_get_path('module', 'demo_umami_content');
PHP warning:  The following module is missing from the file system: demo_umami_content in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/bootstrap.inc on line 276
>>>
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Active » Needs review
FileSize
2945306-3.patch1.2 KB

So the patch attach passes on 8.6.x but fails on 8.5.x therefore showing the behaviour difference.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
2945306-4.test-only.patch1.7 KB
2945306-4.patch4.42 KB

I think we should take a defensive position with the installer service providers and use drupal_installation_attempted() since this means that only the global install_state and this function determines if we are installing or not.

Also the new behaviour in KernelTestBase tests is probably desired since it makes it easier for modules provided by profiles to have kernel tests.

Status: Needs review » Needs work

The last submitted patch, 4: 2945306-4.patch, failed testing. View results

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

While I agree that we should be defensive here, I also believe we should actually reset the globals as soon as we possible can. Having a more normal environment is a good thing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Needs work » Needs review
FileSize
4-7-interdiff.txt3.26 KB
2945306-7.patch2.25 KB

Actually looking at install_drupal() there's a really good place to put an unset.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
7-8-interdiff.txt612 bytes
2945306-8.patch2.38 KB

A comment would be nice.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/includes/install.core.inc
@@ -135,6 +135,7 @@ function install_drupal($class_loader, $settings = []) {
   if (!empty($install_state['installation_finished'])) {
     unset($GLOBALS['install_state']);
+    unset($GLOBALS['conf']['container_service_providers']['InstallerServiceProvider']);
   }

Nice! 

  1. +++ b/core/includes/install.core.inc
@@ -135,6 +135,9 @@ function install_drupal($class_loader, $settings = []) {
+    // If installation is finished ensure any further container rebuilds do not
+    // use the installer's service provider.
+    unset($GLOBALS['conf']['container_service_providers']['InstallerServiceProvider']);

    I wish we would have a different mechanism for that than setting globals, ¯\_(ツ)_/¯

  2. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
@@ -306,4 +306,15 @@ protected function tearDown() {
+  /**
+   * Ensures KernelTestBase tests can access module's in profiles.
+   */
+  public function testProfileModules() {
+    $this->assertFileExists('core/profiles/demo_umami/modules/demo_umami_content/demo_umami_content.info.yml');
+    $this->assertSame(
+      'core/profiles/demo_umami/modules/demo_umami_content/demo_umami_content.info.yml',
+      \Drupal::service('extension.list.module')->getPathname('demo_umami_content')
+    );
+  }

    I'm a bit confused, what is special about kernel tests?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

KernelTests are special because never call install_drupal(). So they're not really part of the problem. Kernel tests have the profile set to an empty string. In 8.5.x they couldn't install real modules that were part of profiles. With the new extension listing they can - which is a cool, unintended I guess, side-effect.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
8-11-interdiff.txt638 bytes
2945306-11.patch2.37 KB

Small typo.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Title: The new ModuleList in 8.6.x can see modules the previous code could not. » The new ModuleList in 8.6.x can discover modules in tests the previous code could not.

The last submitted patch, 4: 2945306-4.test-only.patch, failed testing. View results

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you @alexpott for the explanation!

  • catch committed 80ee826 on 8.6.x 
    Issue #2945306 by alexpott, dawehner: The new ModuleList in 8.6.x can...

  • catch committed a0484ea on 8.5.x 
    Issue #2945306 by alexpott, dawehner: The new ModuleList in 8.6.x can...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed ed37dfe on 8.5.x 
    Revert "Issue #2945306 by alexpott, dawehner: The new ModuleList in 8.6....
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.5.x-dev » 8.6.x-dev

*Cough* clue's in the title.

Status: Fixed » Closed (fixed)

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