BUG ModuleManifest::getModuleByPath returns the wrong module

[sunnysideup]

Affected Version

SS4

Description

Have a look here: https://github.com/silverstripe/silverstripe-framework/blob/4/src/Core/Manifest/ModuleManifest.php#L275-L277 This code does not make any sense to me (my comments are the unindented ones):

            // Check if path is in module
//get the real path of the module - so far so good
            $realPath = realpath($module->getPath());
// if the realpath exists (fine) 
// and the module path ($realPath) and ($path) do not start with the same string  then bail out - fine
            if ($realPath && stripos($path, $realPath) !== 0) {
                continue;
            }

            // If this is the root module, keep looking in case there is a more specific module later
// it is in the root - ok - fine, maybe check for '/'?
            if (empty($module->getRelativePath())) {
                $rootModule = $module;
            } else {
//now we just return any old module that did not yet bail out - leading to false positives.
                return $module;
            }

Here is some examples of false positive:

$path = /var/www/mysite/vendor/sunnysideup/ecommerce_tax/scr/Model/MyCoolThing.php $module->getPath = /var/www/mysite/vendor/sunnysideup/ecommerce

$path = /var/www/mysite/vendor/silverstripe/frameworkwhatevers/scr/Model/MyCoolThing.php $module->getPath = /var/www/mysite/silverstripe/framework

ref: https://www.php.net/manual/en/function.stripos.php

[sunnysideup]

A suggested solution is to add a forward slash at the end of the module path.

[emteknetnz]

Hi thanks for reporting.

Just to reiterate the core issue in-case anyone else reading this is confused, the use of stripos means that that module names that have the same characters as a longer module name are being matched e.g.

ecommerce is incorrectly being selected instead of ecommerce_tax

Would you be keen to create a PR to fix this?

I'm not sure how we'd go about unit testing this to validate that it's working though. Potentially we'd need to create a separate service class that we'd feed a $path a list of $realPath's to ensure that the correct one is matched.

[sunnysideup]

Hi Steve! @emteknetnz

Thank you for looking at this.

I can do a PR. Here are my questions:

• what branch shall I fork?
• are you happy with the solution of adding the forward slash at the end of the modulePath?
• when will this patch be released (approximately?), I need it desperately!

[emteknetnz]

what branch shall I fork?

Fork the branch 4.6

are you happy with the solution of adding the forward slash at the end of the modulePath?

If it's demonstrated with unit tests that it fixes the issue, then yes absolutely

when will this patch be released (approximately?), I need it desperately!

Likely there will be 4.6.1 released around September. Until then I'd recommend using a forked version of 4.6 for you own project(s) that rely on this fix

[sunnysideup]

Ok - great... Can you give me an example of a unit test that is close to what we want to test?

[emteknetnz]

pseduo code (without thinking too much about the actual problem or solution, I've probably mixed $path and $realPath up):

class SomeUnitTest extends SapphireTest
{

    public function testSomething()
    {
        $realPaths = [
            'framework/src/model/abc.php',
            'ecommerce_tax/src/model/def.php',
            'frameworksomething/src/model/xyz.php',
            'ecommerce/src/model/xyz.php'
        ];
        $arr = [
            '/framework' => 'framework/src/model/abc.php',
            '/ecommerce' => 'ecommerce/src/model/def.php'
        ];
        $newServiceClass = NewServiceClass::singleton();
        foreach ($arr as $path => $realPath) {
            $matchedRealPath = $newServiceClass->matchRealPath($path, $realPaths);
            $this->assertEquals($realPath, $matchedRealPath);
        }

[ScopeyNZ]

We can do a 4.6.1 pretty much immediately @sunnysideup .

@emteknetnz see https://github.com/silverstripe/silverstripe-framework/issues/8970

[michalkleiner]

Nice find @sunnysideup. I've fixed something very mildly related https://github.com/silverstripe/silverstripe-framework/commit/ab87bdc04466cf9da95da3670a5db9e30cfce64d, it was also matching substrings.

[sunnysideup]

see: https://github.com/silverstripe/silverstripe-framework/pull/9569