ModuleManifest::getModuleByPath fix to ensure right module is returned

[sunnysideup]

see: #9561 for details.

I have added forward slash for more reliable matching AND
I have added a unit test that passes AFAIK.

[emteknetnz]

Hey @sunnysideup thanks for the PR. It looks good. I've asked a couple of questions on it.

[emteknetnz]

@sunnysideup would you be able to update the title on this PR to something a little more descriptive, cheers

[sunnysideup]

Do we need to do anything else?

[sunnysideup]

@sunnysideup would you be able to update the title on this PR to something a little more descriptive, cheers

Done ... not sure if it is any better as I am not sure what is customary.

[robbieaverill]

Hi @sunnysideup, it looks like this PR has broken a bunch of our unit tests. Would you mind fixing those up?

[sunnysideup]

Yeah - that does not look good! @robbieaverill - will look at this!

[sunnysideup]

I fixed my specific tests, but I am not sure why other tests are broken. Is that normal?

[robbieaverill]

I would suggest that the changes you've made are causing those test failures, particularly the ones that are saying it can't resolve the silverstripe/admin module

[sunnysideup]

Hmmm, yes. But some of the errors are really confusing to me.

[sunnysideup]

I am getting different errors locally.

[sunnysideup]

adding this test:

    /**
     * @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);
    }

breaks a whole bunch of other tests ....

Fixing this now.

[sunnysideup]

I think I cracked it!

[sunnysideup]

@robbieaverill - is this better?

[sunnysideup]

@robbieaverill I have linted to code and that now seems to pass.

I would love to run my linter on all of framework. Could I do that as a pull request?

[ScopeyNZ]

Probably not. Your linting includes rules like putting a space after ! in conditionals, which we don't have a specific rule for (apparently), but we do have a de-facto standard to not include that space.

A linting PR like that would just enforces a bunch of arbitrary new rules that we would need to discuss.

[robbieaverill]

@sunnysideup I only work part time on the Silverstripe project nowadays, apologies if I don't get to this promptly.

[sunnysideup]

@ScopeyNZ - no worries. Fixed. Your lint test should pick that up.

I have removed the exclamation mark space. What linter do you use? I use this one:

• easy coding standards (https://packagist.org/packages/symplify/easy-coding-standard) with this config:

• https://github.com/sunnysideup/silverstripe-easy-coding-standards/blob/master/ecs.yml

[sunnysideup]

@sunnysideup I only work part time on the Silverstripe project nowadays, apologies if I don't get to this promptly.

All good. Sorry for all the noise!

[sunnysideup]

Probably not. Your linting includes rules like putting a space after ! in conditionals, which we don't have a specific rule for (apparently), but we do have a de-facto standard to not include that space.

A linting PR like that would just enforces a bunch of arbitrary new rules that we would need to discuss.

Obviously there would be a super clear discussion about the rule-set applied. Probably a lot of hassle to even get done, but then it would be good to always apply that ruleset automatically at every commit or something like that - so that even sloppy coders just end up with the same clean code. That could save a lot of time.

[robbieaverill]

Hey @sunnysideup, our linting rules are PSR-12. We are aiming for full compatibility in SilverStripe 5.x. You can see the rules per module in phpcs.xml for each module. These are configuration for PHP CodeSniffer (the linting tool). We have some opinionated rules on top of PSR-12, but not many. If you'd like to discuss our linting rules further I'd suggest opening an RFC on framework with your recommended changes 😄

[sunnysideup]

Hey @sunnysideup, our linting rules are PSR-12. We are aiming for full compatibility in SilverStripe 5.x. You can see the rules per module in phpcs.xml for each module. These are configuration for PHP CodeSniffer (the linting tool). We have some opinionated rules on top of PSR-12, but not many. If you'd like to discuss our linting rules further I'd suggest opening an RFC on framework with your recommended changes

Thank you for explaining that.

In terms of changing / discussing: yeah nah.... the less I have to think about that the better.

[sunnysideup]

@robbieaverill @emteknetnz - what do we need to do to get this released?

[kinglozzer]

Thanks for your work on this so far @sunnysideup. I think the logic in the getModuleByPath() method is a little more verbose than it needs to be - I appreciate you’re probably trying to retain the existing strpos() logic, but I think if we trim the directory separators we can just remove that and compare the two strings directly:

public function getModuleByPath($path)
{
    // Ensure path exists
    $path = trim(realpath($path), DIRECTORY_SEPARATOR);
    if (!$path) {
        return null;
    }

    /** @var Module $rootModule */
    $rootModule = null;

    // Find based on loaded modules
    $modules = ModuleLoader::inst()->getManifest()->getModules();

    foreach ($modules as $module) {
        // If the module has a path, check whether it matches - otherwise move on to next module
        $modulePath = trim(realpath($module->getPath()), DIRECTORY_SEPARATOR);
        if ($modulePath && $modulePath !== $path) {
            continue;

        }
        
        // If this is the root module, keep looking in case there is a more specific module later
        if (empty($module->getRelativePath())) {
            $rootModule = $module;
        } else {
            return $module;
        }
    }

    // Fall back to top level module
    return $rootModule;
}

[sunnysideup]

Sure, but I think the real issue is the reverse logic... We should say, instead:

if modulePath is the same as the start of the Path then return the ModulePath

But does it really matters if it works? I'd rather just have a it fixed now.

[sunnysideup]

@kinglozzer
what are we going to do here?

[kinglozzer]

Sorry for the delay! I was completely wrong with my suggestion above anyway, I misunderstood how the method works 🤦‍♂️

I do still think the logic in getModuleByPath() should be simplified though (sorry...), I had to re-read it a few times to work out why we're removing and then re-adding the directory separators. How about this:

foreach ($modules as $module) {
    $modulePath = realpath($module->getPath());
    // If this module has a path
    if ($modulePath) {
        // Force a trailing slash so we don't mix up, for example:
        // silverstripe/framework and silverstripe/framework-extension
        $modulePath = $modulePath . DIRECTORY_SEPARATOR;

        // If the path doesn't begin with this module's path, skip this module
        if (stripos($path, $modulePath) !== 0) {
            continue;
        }
    }

    // ...
}

[sunnysideup]

@kinglozzer - that is better, BUT ... ;-) sometimes the path is the $modulePath without the final DIRECTORY_SEPARATOR - so we need to check for both.

The real way to simplify it is go away from if not present then check next and say: if the same then return. That makes it much easier to read and understand. Negatives are always hareder then straight: if ($a === $b) {return $x;}

[kinglozzer]

that is better, BUT ... ;-) sometimes the path is the $modulePath without the final DIRECTORY_SEPARATOR - so we need to check for both.

Gah, I was going by the docblock which says $path is a path to a specific file!

Let’s just get this bug fixed. The extra unit test coverage will make refactoring it in future easier anyway if anyone decides to take that on. Sorry for the noise @sunnysideup 😅

Could you squash the changes into a single commit?