storm icon indicating copy to clipboard operation
storm copied to clipboard

[WIP] Revise class loader

Open LukeTowers opened this issue 3 years ago • 1 comments

Fixes https://github.com/wintercms/winter/discussions/214.

This fixes a long standing issue with the ClassLoader that handles loading module and plugin classes where only lowercase paths to the (properly cased) class file were technically supported. This prevents PSR-4 folder structures in plugins and modules from working; and even worse it would usually only show up in production with an opaque error message since production machines typically have case sensitive filesystems and development machines tend to not.

The current logic uses a directory scanning approach of generating many different possible variations of a valid path for a given class and then looping over the "loaded directories" to scan each of them for a valid instance of a possible path.

The new logic instead switches to requiring class prefix to class path to be registered with the ClassLoader explicitly which not only makes the resolution logic much faster (since the prefix is directly matched with the request class instead of always having to scan for a potential match) but it also means that the autoloading can be completely disabled for plugins and modules that aren't currently enabled which isn't currently the case and will help with fully cutting off disabled plugins and modules from the application.

LukeTowers avatar Mar 03 '22 03:03 LukeTowers

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. If this is still being worked on, please respond and we will re-open this pull request. If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

github-actions[bot] avatar May 09 '22 01:05 github-actions[bot]

@LukeTowers Is this PR responsible for the class loading issues in Windows?

damsfx avatar Dec 13 '22 14:12 damsfx

@damsfx most likely yes. We still haven't encountered the issues you were reporting nor have we heard from anyone else about them so if you're able to track it down on your machine and submit a PR I would appreciate that :)

LukeTowers avatar Dec 13 '22 16:12 LukeTowers

@LukeTowers After investigating ... the resolvePath of the ClassLoader seems to be the faulty.

When running the Winter.Test plugin migration, I've got this issue.

In seed_tables.php line 31:
  Class "Winter\Test\Models\Person" not found

The resolvePath() method is called with the following argument in this case

$path = "P:\_Sites\_Labs\winterdev\plugins\winter\test\Models\Person.php"

but the return value is

$path = "P:\_Sites\_Labs\winterdev\P:\_Sites\_Labs\winterdev\plugins\winter\test\Models\Person.php"

The method adds $this->basePath . DIRECTORY_SEPARATOR unnecessarily.

It may be necesary to include the base path in the test :

    /**
     * Resolve the provided path, relative or absolute
     */
    protected function resolvePath(string $path): string
    {
        if (!Str::startsWith($path, ['/', '\\', $this->basePath . DIRECTORY_SEPARATOR])) {
            $path = $this->basePath . DIRECTORY_SEPARATOR . $path;
        }
        return $path;
    }

damsfx avatar Dec 20 '22 17:12 damsfx

Oh, I see, on windows the basePath does not necessarily start with a slash or backslash...

Care to submit a PR @damsfx ?

mjauvin avatar Dec 20 '22 17:12 mjauvin