ModuleManifest::getModuleByPath fix to ensure right module is returned

src/Core/Manifest/ModuleManifest.php

@@ -66,6 +66,20 @@ class ModuleManifest
      */
     private static $project = null;
 
+    /**
+     * Constructs and initialises a new configuration object, either loading
+     * from the cache or re-scanning for classes.
+     *
+     * @param string $base The project base path.
+     * @param CacheFactory $cacheFactory Cache factory to use
+     */
+    public function __construct($base, CacheFactory $cacheFactory = null)
+    {
+        $this->base = $base;
+        $this->cacheKey = sha1($base) . '_modules';
+        $this->cacheFactory = $cacheFactory;
+    }
+
     /**
      * Adds a path as a module
      *
@@ -104,20 +118,6 @@ public function moduleExists($name)
         return !empty($module);
     }
 
-    /**
-     * Constructs and initialises a new configuration object, either loading
-     * from the cache or re-scanning for classes.
-     *
-     * @param string $base The project base path.
-     * @param CacheFactory $cacheFactory Cache factory to use
-     */
-    public function __construct($base, CacheFactory $cacheFactory = null)
-    {
-        $this->base = $base;
-        $this->cacheKey = sha1($base) . '_modules';
-        $this->cacheFactory = $cacheFactory;
-    }
-
     /**
      * @param bool $includeTests
      * @param bool $forceRegen Force the manifest to be regenerated.
@@ -175,7 +175,7 @@ public function regenerate($includeTests = false)
                 if ($finder->isDirectoryModule($basename, $pathname, $depth)) {
                     $this->addModule($pathname);
                 }
-            }
+            },
         ]);
         $finder->find($this->base);
 
@@ -226,20 +226,18 @@ public function getModules()
 
     /**
      * Sort modules sorted by priority
-     *
-     * @return void
      */
     public function sort()
     {
         $order = static::config()->uninherited('module_priority');
         $project = static::config()->get('project');
 
-        /* @var PrioritySorter $sorter */
+        /** @var PrioritySorter $sorter */
         $sorter = Injector::inst()->createWithArgs(
             PrioritySorter::class . '.modulesorter',
             [
                 $this->modules,
-                $order ?: []
+                $order ?: [],
             ]
         );
 
@@ -269,13 +267,28 @@ public function getModuleByPath($path)
 
         // Find based on loaded modules
         $modules = ModuleLoader::inst()->getManifest()->getModules();
+
         foreach ($modules as $module) {
             // Check if path is in module
-            $realPath = realpath($module->getPath());
-            if ($realPath && stripos($path, $realPath) !== 0) {
-                continue;
+            $modulePath = realpath($module->getPath());
+            // if there is a real path
+            if ($modulePath) {
+                // we remove separator to ensure that we are comparing fairly
+                $modulePath = rtrim($modulePath, DIRECTORY_SEPARATOR);
+                $path = rtrim($path, DIRECTORY_SEPARATOR);
+                // if the paths are not the same
+                if ($modulePath !== $path) {
+                    //add separator to avoid mixing up, for example:
+                    //silverstripe/framework and silverstripe/framework-extension
+                    $modulePath .= DIRECTORY_SEPARATOR;
+                    $path .= DIRECTORY_SEPARATOR;
+                    // if the module path is not the same as the start of the module path being tested
+                    if (stripos($path, $modulePath) !== 0) {
+                        // then we need to test the next module
+                        continue;
+                    }
+                }
             }
-
             // If this is the root module, keep looking in case there is a more specific module later
             if (empty($module->getRelativePath())) {
                 $rootModule = $module;

tests/php/Core/Manifest/ClassManifestTest.php

@@ -76,6 +76,7 @@ public function testGetClasses()
             'classd' => "{$this->base}/module/classes/ClassD.php",
             'classe' => "{$this->base}/module/classes/ClassE.php",
             'vendorclassa' => "{$this->base}/vendor/silverstripe/modulec/code/VendorClassA.php",
+            'vendorclassx' => "{$this->base}/vendor/silverstripe/modulecbetter/code/VendorClassX.php",
         ];
         $this->assertEquals($expect, $this->manifest->getClasses());
     }
@@ -90,6 +91,7 @@ public function testGetClassNames()
                 'classd' => 'ClassD',
                 'classe' => 'ClassE',
                 'vendorclassa' => 'VendorClassA',
+                'vendorclassx' => 'VendorClassX',
             ],
             $this->manifest->getClassNames()
         );

tests/php/Core/Manifest/ModuleManifestTest.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Core\Tests\Manifest;
 
+use SilverStripe\Core\Manifest\ModuleLoader;
 use SilverStripe\Core\Manifest\ModuleManifest;
 use SilverStripe\Dev\SapphireTest;
 
@@ -34,6 +35,7 @@ public function testGetModules()
                 'module',
                 'silverstripe/awesome-module',
                 'silverstripe/modulec',
+                'silverstripe/modulecbetter',
                 'silverstripe/root-module',
             ],
             array_keys($modules)
@@ -105,4 +107,37 @@ public function testGetResourcePathOnRoot()
             $module->getResource('composer.json')->getRelativePath()
         );
     }
+
+    /**
+     * @return array
+     */
+    public function providerTestGetModuleByPath()
+    {
+        return [
+            ['vendor/silverstripe/modulec/code/VendorClassA.php', 'silverstripe/modulec'],
+            ['vendor/silverstripe/modulecbetter/code/VendorClassX.php', 'silverstripe/modulecbetter'],
+        ];
+    }
+
+    /**
+     * @dataProvider providerTestGetModuleByPath
+     * @param string $path
+     * @param string $expectedModuleName
+     */
+    public function testGetModuleByPath($path, $expectedModuleName)
+    {
+        // important - load the manifest that we are working with to the ModuleLoader
+        ModuleLoader::inst()->pushManifest($this->manifest);
+
+        // work out variables
+        $path = $this->base . '/' . $path;
+        $module = $this->manifest->getModuleByPath($path);
+        $actualModuleName = $module->getName();
+
+        // it is testing time!
+        $this->assertEquals($expectedModuleName, $actualModuleName);
+
+        // bye bye bogus manifest
+        ModuleLoader::inst()->popManifest();
+    }
 }

tests/php/Core/Manifest/fixtures/classmanifest/vendor/silverstripe/modulecbetter/_config/config.yml

@@ -0,0 +1,4 @@
+---
+Name: blankconfigmodulecbetter
+---
+{}

tests/php/Core/Manifest/fixtures/classmanifest/vendor/silverstripe/modulecbetter/client/script.js

@@ -0,0 +1 @@
+/* script.js */

tests/php/Core/Manifest/fixtures/classmanifest/vendor/silverstripe/modulecbetter/code/VendorClassX.php

@@ -0,0 +1,6 @@
+<?php
+
+class VendorClassX
+{
+
+}

tests/php/Core/Manifest/fixtures/classmanifest/vendor/silverstripe/modulecbetter/composer.json

@@ -0,0 +1,5 @@
+{
+  "name": "silverstripe/modulecbetter",
+  "description": "dummy test module",
+  "require": {}
+}