Problem/Motivation

Installing a module or a theme using the UI, it tries to determine it's a theme by checking for a file that ends in *.module.

  /**
   * Implements Drupal\Core\Updater\UpdaterInterface::canUpdateDirectory().
   */
  static function canUpdateDirectory($directory) {
    // This is a lousy test, but don't know how else to confirm it is a theme.
    if (file_scan_directory($directory, '/.*\.module$/')) {
      return FALSE;
    }
    return TRUE;
  }

This is inadequate, the module page_manager doesn't have that file so it ends up in the theme directory.

Proposed resolution

Find the info.yml file and read the type.

Remaining tasks

  1. Write patch to fix this issue.
  2. Write tests to prove this is broken/won't happen again.

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#14 interdiff.txt3.58 KBjoelpittet
#14 modules_getting-2413695-14.patch4.06 KBjoelpittet
#10 modules_getting-2413695-10.patch3.46 KBjoelpittet
#10 modules_getting-2413695-10-tests-only.patch1.19 KBjoelpittet
#3 modules_getting-2413695-3.patch2.27 KBjoelpittet
#1 modules_getting-2413695-1.patch1.73 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new modules_getting-2413695-1.patch1.73 KB

Here's one fix that works.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new modules_getting-2413695-3.patch2.27 KB

This one is a bit better and removes the @todo that was there.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Parent issue: » #2042447: Install a module user interface does not install modules (or themes)

Needs this patch to test #2042447-58: Install a module user interface does not install modules (or themes)

The last submitted patch, 1: modules_getting-2413695-1.patch, failed testing.

vj’s picture

vj commented

@joelpittet: This patch works but requires this #2042447-58: Install a module user interface does not install modules (or themes)

Tried omega theme to upload it moved to themes folder
then tried a custom module without .module file and it moved to modules folder.

Note* : Still error exists which i have mentioned in #2042447-60.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@Vj yes it does, but I need someone to help with the batch API related changes in that one. I'd like this to go in regardless as it's a somewhat separate issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work
Issue tags: +Needs tests
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new modules_getting-2413695-10-tests-only.patch1.19 KB
new modules_getting-2413695-10.patch3.46 KB

The last submitted patch, 10: modules_getting-2413695-10-tests-only.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
  1. +++ b/core/lib/Drupal/Core/Updater/Module.php
@@ -51,10 +51,13 @@ public function isInstalled() {
+    $info_file = static::findInfoFile($directory);
+    $info = \Drupal::service('info_parser')->parse($info_file);
+    if (empty($info)) {
+      throw new UpdaterException(t('Unable to parse info file: %info_file.', ['%info_file' => $info_file]));
     }

+++ b/core/lib/Drupal/Core/Updater/Theme.php
@@ -51,11 +51,13 @@ public function isInstalled() {
+    $info_file = static::findInfoFile($directory);
+    $info = \Drupal::service('info_parser')->parse($info_file);
+    if (empty($info)) {
+      throw new UpdaterException(t('Unable to parse info file: %info_file.', ['%info_file' => $info_file]));
     }

    This is shared code and there is a base class. how about a protected static function getExtensionInfo($directory) ? 

  2. +++ b/core/modules/update/src/Tests/UpdateUploadTest.php
@@ -111,4 +112,18 @@ function testUpdateManagerCoreSecurityUpdateMessages() {
+   * Tests that modules with only module info.yml files are detected without
+   * a module file.

    Can this be rewritten to one line? 

  3. +++ b/core/modules/update/src/Tests/UpdateUploadTest.php
@@ -111,4 +112,18 @@ function testUpdateManagerCoreSecurityUpdateMessages() {
+   * @covers Module::canUpdateDirectory().

    @covers is only for phpunit, and doesn't use () or . 

  4. +++ b/core/modules/update/src/Tests/UpdateUploadTest.php
@@ -111,4 +112,18 @@ function testUpdateManagerCoreSecurityUpdateMessages() {
+  function testUpdateDirectory() {

    public function

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Needs work
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new modules_getting-2413695-14.patch4.06 KB
new interdiff.txt3.58 KB

Thanks @tim.plunkett.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Has test coverage, fix looks good!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/lib/Drupal/Core/Updater/Theme.php
@@ -51,11 +51,9 @@ public function isInstalled() {
-    // This is a lousy test, but don't know how else to confirm it is a theme.

ha!

Committed/pushed to 8.0.x, thanks!

  • catch committed dc52e85 on 8.0.x 
    Issue #2413695 by joelpittet: Modules getting installed in the theme...

Status: Fixed » Closed (fixed)

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