Problem/Motivation

Drupal\Tests\system\Functional\Module\RequiredTest is trying to test that you can't uninstall required modules from the admin/modules page but you can no longer uninstall modules there.

Proposed resolution

Move the test to be a part of UninstallTest

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#3 2949834-3.patch2.43 KBalexpott
#3 2-3-interdiff.txt802 bytesalexpott
#2 2949834-2.patch2.43 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2949834-2.patch2.43 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-3-interdiff.txt802 bytes
new 2949834-3.patch2.43 KB

Too many dots

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Nice catch.

  1. +++ b/core/modules/system/tests/src/Functional/Module/UninstallTest.php
@@ -57,6 +57,17 @@ public function testUninstallPage() {
     $this->assertTitle(t('Uninstall') . ' | Drupal');

    t() in a test. 

  2. +++ b/core/modules/system/tests/src/Functional/Module/UninstallTest.php
@@ -57,6 +57,17 @@ public function testUninstallPage() {
+    foreach (\Drupal::service('extension.list.module')->getAllInstalledInfo() as $module => $info) {

    Grab the service from $this->container

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Thanks for the review @Mile23
1. That's not part of the patch.
2. I don't think there is any difference between \Drupal::service() and $this->container. And we have no rules about what to use.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

2. I don't think there is any difference between \Drupal::service() and $this->container. And we have no rules about what to use.

Nope, just lively discussions on the subject

Feedback has been addressed. The old test never actually added a required module, so if we ever got to a state where a test install had no required modules it would pass and never test anything, the new version also fixes this gap, nice!

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

"And we have no rules about what to use."

Right, so use $this->container. :-)

No worries. RTBC is fine.

  • catch committed d47cfa0 on 8.6.x 
    Issue #2949834 by alexpott: RequiredTest is not testing what it thinks...
catch’s picture

catch
he/him
Primary language English
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!

Version: 8.5.x-dev » 8.6.x-dev
Status: Fixed » Reviewed & tested by the community
  • catch committed e6040e5 on 8.5.x 
    Issue #2949834 by alexpott: RequiredTest is not testing what it thinks...
tacituseu’s picture

tacituseu commented

This broke 8.5.x as #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList never went into 8.5.x.

https://www.drupal.org/pift-ci-job/918609

Drupal\Tests\system\Functional\Module\UninstallTest::testUninstallPage
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "extension.list.module".

  • alexpott committed 0c2507e on 8.5.x 
    Revert "Issue #2949834 by alexpott: RequiredTest is not testing what it...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@tacituseu thanks for spotting this. Just reverted the patch. We can leave this one not back ported.

Status: Fixed » Closed (fixed)

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