From 443e4033b4b040bfe2580954026c7c760826dac2 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Wed, 2 Mar 2022 21:11:06 -0600 Subject: [PATCH 1/6] Initial work on revising the ClassLoader logic to use registered packages instead of directory scanning --- .../Bootstrap/RegisterClassLoader.php | 5 - src/Support/ClassLoader.php | 149 +++++++----------- src/Support/ModuleServiceProvider.php | 46 ++++-- tests/Extension/ExtendableTest.php | 4 - tests/Support/ClassLoaderTest.php | 4 - 5 files changed, 83 insertions(+), 125 deletions(-) diff --git a/src/Foundation/Bootstrap/RegisterClassLoader.php b/src/Foundation/Bootstrap/RegisterClassLoader.php index 4df803f6b..47069ff3a 100644 --- a/src/Foundation/Bootstrap/RegisterClassLoader.php +++ b/src/Foundation/Bootstrap/RegisterClassLoader.php @@ -24,11 +24,6 @@ public function bootstrap(Application $app) $loader->register(); - $loader->addDirectories([ - 'modules', - 'plugins' - ]); - $app->after(function () use ($loader) { $loader->build(); }); diff --git a/src/Support/ClassLoader.php b/src/Support/ClassLoader.php index da80d04f0..a57088f0e 100644 --- a/src/Support/ClassLoader.php +++ b/src/Support/ClassLoader.php @@ -13,79 +13,57 @@ class ClassLoader { /** - * The filesystem instance. - * - * @var \Winter\Storm\Filesystem\Filesystem + * @var \Winter\Storm\Filesystem\Filesystem The filesystem instance. */ public $files; /** - * The base path. - * - * @var string + * @var string The base path. */ public $basePath; /** - * The manifest path. - * - * @var string|null + * @var string|null The manifest path. */ public $manifestPath; /** - * The loaded manifest array. - * - * @var array + * @var array The loaded manifest array. */ public $manifest; /** - * Determine if the manifest needs to be written. - * - * @var bool + * @var bool Determine if the manifest needs to be written. */ protected $manifestDirty = false; /** - * The registered directories. - * - * @var array + * @var array The registered packages to autoload for */ - protected $directories = []; + protected $autoloadedPackages = []; /** - * Indicates if a ClassLoader has been registered. - * - * @var bool + * @var bool Indicates if a ClassLoader has been registered. */ protected $registered = false; /** - * Class alias array. - * - * @var array + * @var array Class alias array. */ protected $aliases = []; /** - * Namespace alias array. - * - * @var array + * @var array Namespace alias array. */ protected $namespaceAliases = []; /** - * Aliases that have been explicitly loaded. - * - * @var array + * @var array Aliases that have been explicitly loaded. */ protected $loadedAliases = []; /** - * Reversed classes to ignore for alias checks. - * - * @var array + * @var array Reversed classes to ignore for alias checks. */ protected $reversedClasses = []; @@ -119,6 +97,7 @@ public function load($class) return true; } + // Check the class manifest for the class' location if ( isset($this->manifest[$class]) && $this->isRealFilePath($path = $this->manifest[$class]) @@ -135,28 +114,35 @@ class_alias($class, $reverse); return true; } - list($lowerClass, $upperClass, $lowerClassStudlyFile, $upperClassStudlyFile) = static::getPathsForClass($class); - - foreach ($this->directories as $directory) { - $paths = [ - $directory . DIRECTORY_SEPARATOR . $lowerClass, - $directory . DIRECTORY_SEPARATOR . $upperClass, - $directory . DIRECTORY_SEPARATOR . $lowerClassStudlyFile, - $directory . DIRECTORY_SEPARATOR . $upperClassStudlyFile, - ]; - - foreach ($paths as $path) { - if ($this->isRealFilePath($path)) { - $this->includeClass($class, $path); - - if (!is_null($reverse = $this->getReverseAlias($class))) { - if (!class_exists($reverse, false) && !in_array($reverse, $this->loadedAliases)) { - class_alias($class, $reverse); - $this->reversedClasses[] = $reverse; + // Check our registered autoload packages for a match + foreach ($this->autoloadedPackages as $prefix => $path) { + if (Str::startsWith($class, $prefix)) { + $parts = explode('\\', Str::after($class, $prefix)); + $file = array_pop($parts) . '.php'; + $namespace = implode('\\', $parts); + $directory = str_replace(['\\', '_'], DIRECTORY_SEPARATOR, $namespace); + + $pathsToTry = [ + // Lowercase directory structure - default structure of plugins and modules + $path . strtolower($directory) . DIRECTORY_SEPARATOR . $file, + + // Fallback to the unmodified path + $path . $directory . DIRECTORY_SEPARATOR . $file, + ]; + + foreach ($pathsToTry as $classPath) { + if ($this->isRealFilePath($classPath)) { + $this->includeClass($class, $classPath); + + if (!is_null($reverse = $this->getReverseAlias($class))) { + if (!class_exists($reverse, false) && !in_array($reverse, $this->loadedAliases)) { + class_alias($class, $reverse); + $this->reversedClasses[] = $reverse; + } } - } - return true; + return true; + } } } } @@ -169,14 +155,18 @@ class_alias($alias, $class); } /** - * Determine if a relative path to a file exists and is real + * Determine if the provided path to a file exists and is real * * @param string $path * @return bool */ protected function isRealFilePath($path) { - return is_file(realpath($this->basePath.DIRECTORY_SEPARATOR.$path)); + if (!Str::startsWith($path, ['/', '\\'])) { + $path = $this->basePath . DIRECTORY_SEPARATOR . $path; + } + + return is_file(realpath($path)); } /** @@ -188,7 +178,11 @@ protected function isRealFilePath($path) */ protected function includeClass($class, $path) { - require_once $this->basePath.DIRECTORY_SEPARATOR.$path; + if (!Str::startsWith($path, ['/', '\\'])) { + $path = $this->basePath . DIRECTORY_SEPARATOR . $path; + } + + require_once $path; $this->manifest[$class] = $path; @@ -241,46 +235,11 @@ public function build() } /** - * Add directories to the class loader. - * - * @param string|array $directories - * @return void - */ - public function addDirectories($directories) - { - $this->directories = array_merge($this->directories, (array) $directories); - - $this->directories = array_unique($this->directories); - } - - /** - * Remove directories from the class loader. - * - * @param string|array $directories - * @return void - */ - public function removeDirectories($directories = null) - { - if (is_null($directories)) { - $this->directories = []; - } - else { - $directories = (array) $directories; - - $this->directories = array_filter($this->directories, function ($directory) use ($directories) { - return !in_array($directory, $directories); - }); - } - } - - /** - * Gets all the directories registered with the loader. - * - * @return array + * Add a namespace prefix to the autoloader */ - public function getDirectories() + public function autoloadPackage(string $namespacePrefix, string $relativePath): void { - return $this->directories; + $this->autoloadedPackages[$namespacePrefix] = $relativePath; } /** diff --git a/src/Support/ModuleServiceProvider.php b/src/Support/ModuleServiceProvider.php index 318ec3966..4edbf49cc 100644 --- a/src/Support/ModuleServiceProvider.php +++ b/src/Support/ModuleServiceProvider.php @@ -1,5 +1,7 @@ getModule(func_get_args())) { - /* - * Register paths for: config, translator, view - */ - $modulePath = base_path() . '/modules/' . $module; - $this->loadViewsFrom($modulePath . '/views', $module); - $this->loadTranslationsFrom($modulePath . '/lang', $module); - $this->loadConfigFrom($modulePath . '/config', $module); + $module = strtolower($this->getModule()); + $modulePath = base_path("modules/$module"); - /* - * Add routes, if available - */ - $routesFile = base_path() . '/modules/' . $module . '/routes.php'; - if (File::isFile($routesFile)) { - $this->loadRoutesFrom($routesFile); - } + // Register paths for: config, translator, view + $this->loadViewsFrom($modulePath . '/views', $module); + $this->loadTranslationsFrom($modulePath . '/lang', $module); + $this->loadConfigFrom($modulePath . '/config', $module); + + // Register routes if present + $routesFile = "$modulePath/routes.php"; + if (File::isFile($routesFile)) { + $this->loadRoutesFrom($routesFile); } } + /** + * Registers the Module service provider. + * @return void + */ + public function register() + { + // Register this module with the application's ClassLoader for autoloading + $module = $this->getModule(); + $this->app->make(ClassLoader::class)->autoloadPackage($module . '\\', "modules/" . strtolower($module) . '/'); + } + /** * Get the services provided by the provider. * @return array @@ -39,9 +48,12 @@ public function provides() return []; } - public function getModule($args) + /** + * Gets the name of this module + */ + public function getModule(): string { - return (isset($args[0]) and is_string($args[0])) ? $args[0] : null; + return Str::before(get_class($this), '\\'); } /** diff --git a/tests/Extension/ExtendableTest.php b/tests/Extension/ExtendableTest.php index 4e7a6efb7..7390baa23 100644 --- a/tests/Extension/ExtendableTest.php +++ b/tests/Extension/ExtendableTest.php @@ -16,10 +16,6 @@ public function setUp(): void $this->registerMockClassLoader(); - $this->classLoader->addDirectories([ - 'plugins' - ]); - $this->classLoader->addNamespaceAliases([ 'Real\\ExtendableTest' => 'Alias\\ExtendableTest', 'Real' => 'Alias', diff --git a/tests/Support/ClassLoaderTest.php b/tests/Support/ClassLoaderTest.php index 827c5f709..acb493f5b 100644 --- a/tests/Support/ClassLoaderTest.php +++ b/tests/Support/ClassLoaderTest.php @@ -19,10 +19,6 @@ public function setUp(): void ); $this->classLoader->register(); - - $this->classLoader->addDirectories([ - 'plugins' - ]); } public function tearDown(): void From 98f9dc9dda2a3f12ce09d064534887177646577c Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Wed, 2 Mar 2022 21:16:14 -0600 Subject: [PATCH 2/6] Fix class path loading when manifest file is generated. --- src/Support/ClassLoader.php | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/Support/ClassLoader.php b/src/Support/ClassLoader.php index a57088f0e..05aa4404f 100644 --- a/src/Support/ClassLoader.php +++ b/src/Support/ClassLoader.php @@ -102,7 +102,7 @@ public function load($class) isset($this->manifest[$class]) && $this->isRealFilePath($path = $this->manifest[$class]) ) { - require_once $this->basePath.DIRECTORY_SEPARATOR.$path; + require_once $this->resolvePath($path); if (!is_null($reverse = $this->getReverseAlias($class))) { if (!class_exists($reverse, false) && !in_array($reverse, $this->loadedAliases)) { @@ -154,6 +154,17 @@ class_alias($alias, $class); } } + /** + * Resolve the provided path, relative or absolute + */ + protected function resolvePath(string $path): string + { + if (!Str::startsWith($path, ['/', '\\'])) { + $path = $this->basePath . DIRECTORY_SEPARATOR . $path; + } + return $path; + } + /** * Determine if the provided path to a file exists and is real * @@ -162,11 +173,7 @@ class_alias($alias, $class); */ protected function isRealFilePath($path) { - if (!Str::startsWith($path, ['/', '\\'])) { - $path = $this->basePath . DIRECTORY_SEPARATOR . $path; - } - - return is_file(realpath($path)); + return is_file(realpath($this->resolvePath($path))); } /** @@ -178,11 +185,7 @@ protected function isRealFilePath($path) */ protected function includeClass($class, $path) { - if (!Str::startsWith($path, ['/', '\\'])) { - $path = $this->basePath . DIRECTORY_SEPARATOR . $path; - } - - require_once $path; + require_once $this->resolvePath($path); $this->manifest[$class] = $path; From 3f7ad2778b084490787ceafa3e4dd3c7b62620ee Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Tue, 29 Nov 2022 15:20:45 -0600 Subject: [PATCH 3/6] Update src/Support/ClassLoader.php Co-authored-by: Ben Thomson --- src/Support/ClassLoader.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Support/ClassLoader.php b/src/Support/ClassLoader.php index 7a2b97a68..890856174 100644 --- a/src/Support/ClassLoader.php +++ b/src/Support/ClassLoader.php @@ -135,8 +135,8 @@ class_alias($class, $reverse); foreach ($pathsToTry as $classPath) { if ($this->isRealFilePath($classPath)) { $this->includeClass($class, $classPath); - - if (!is_null($reverse = $this->getReverseAlias($class))) { + $reverse = $this->getReverseAlias($class); + if (!is_null($reverse)) { if (!class_exists($reverse, false) && !in_array($reverse, $this->loadedAliases)) { class_alias($class, $reverse); $this->reversedClasses[] = $reverse; From e009da0bce343a003188fb1d4b0ca3d52adc0f6f Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Tue, 29 Nov 2022 18:39:38 -0600 Subject: [PATCH 4/6] Sort autoloaded packages by prefix length --- src/Support/ClassLoader.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Support/ClassLoader.php b/src/Support/ClassLoader.php index 890856174..e227195e6 100644 --- a/src/Support/ClassLoader.php +++ b/src/Support/ClassLoader.php @@ -248,6 +248,12 @@ public function build() public function autoloadPackage(string $namespacePrefix, string $relativePath): void { $this->autoloadedPackages[$namespacePrefix] = $relativePath; + + // Ensure packages are sorted by length of the prefix to prevent a greedier prefix + // from being matched first when attempting to autoload a class + uksort($this->autoloadedPackages, function ($a, $b) { + return Str::substrCount($b, '\\') <=> Str::substrCount($a, '\\'); + }); } /** From 5c6701a8219e7b2dd5d8c4d0c31d87cb4c82fe40 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Tue, 29 Nov 2022 18:40:15 -0600 Subject: [PATCH 5/6] Add PHP typehints to the ClassLoader, fix tests --- src/Support/ClassLoader.php | 168 +++++++++--------------------- tests/Support/ClassLoaderTest.php | 2 + 2 files changed, 51 insertions(+), 119 deletions(-) diff --git a/src/Support/ClassLoader.php b/src/Support/ClassLoader.php index e227195e6..92968f709 100644 --- a/src/Support/ClassLoader.php +++ b/src/Support/ClassLoader.php @@ -1,5 +1,6 @@ autoloadPackage("Namespace\Prefix", + * "path/to/namespace"). It supports both the original October approach of all + * lowercase folder names with proper cased filenames and the PSR-4 approach of + * proper cased folder and filenames. */ class ClassLoader { /** - * @var \Winter\Storm\Filesystem\Filesystem The filesystem instance. + * The filesystem instance. */ - public $files; + public Filesystem $files; /** - * @var string The base path. + * The base path. */ - public $basePath; + public string $basePath; /** - * @var string|null The manifest path. + * The manifest path. */ - public $manifestPath; + public ?string $manifestPath; /** - * @var array|null The loaded manifest array. + * The loaded manifest array. */ - public $manifest = null; + public ?array $manifest = null; /** - * @var bool Determine if the manifest needs to be written. + * Determine if the manifest needs to be written. */ - protected $manifestDirty = false; + protected bool $manifestDirty = false; /** - * @var array The registered packages to autoload for + * The registered packages to autoload for */ - protected $autoloadedPackages = []; + protected array $autoloadedPackages = []; /** * The registered callback for loading plugins. - * - * @var callable|null */ - protected $registered = null; + protected ?Closure $registered = null; /** - * @var array Class alias array. + * Class alias array. */ - protected $aliases = []; + protected array $aliases = []; /** - * @var array Namespace alias array. + * Namespace alias array. */ - protected $namespaceAliases = []; + protected array $namespaceAliases = []; /** - * @var array Aliases that have been explicitly loaded. + * Aliases that have been explicitly loaded. */ - protected $loadedAliases = []; + protected array $loadedAliases = []; /** - * @var array Reversed classes to ignore for alias checks. + * Reversed classes to ignore for alias checks. */ - protected $reversedClasses = []; + protected array $reversedClasses = []; /** * Create a new package manifest instance. - * - * @param \Winter\Storm\Filesystem\Filesystem $files - * @param string $basePath - * @param string $manifestPath - * @return void */ - public function __construct(Filesystem $files, $basePath, $manifestPath) + public function __construct(Filesystem $files, string $basePath, string $manifestPath) { $this->files = $files; $this->basePath = $basePath; @@ -86,11 +83,8 @@ public function __construct(Filesystem $files, $basePath, $manifestPath) /** * Load the given class file. - * - * @param string $class - * @return bool|null */ - public function load($class) + public function load(string $class): ?bool { $class = static::normalizeClass($class); @@ -135,7 +129,7 @@ class_alias($class, $reverse); foreach ($pathsToTry as $classPath) { if ($this->isRealFilePath($classPath)) { $this->includeClass($class, $classPath); - $reverse = $this->getReverseAlias($class); + $reverse = $this->getReverseAlias($class); if (!is_null($reverse)) { if (!class_exists($reverse, false) && !in_array($reverse, $this->loadedAliases)) { class_alias($class, $reverse); @@ -154,6 +148,8 @@ class_alias($class, $reverse); class_alias($alias, $class); return true; } + + return null; } /** @@ -169,23 +165,16 @@ protected function resolvePath(string $path): string /** * Determine if the provided path to a file exists and is real - * - * @param string $path - * @return bool */ - protected function isRealFilePath($path) + protected function isRealFilePath(string $path): bool { return is_file(realpath($this->resolvePath($path))); } /** * Includes a class and adds to the manifest - * - * @param string $class - * @param string $path - * @return void */ - protected function includeClass($class, $path) + protected function includeClass(string $class, string $path): void { require_once $this->resolvePath($path); @@ -196,10 +185,8 @@ protected function includeClass($class, $path) /** * Register the given class loader on the auto-loader stack. - * - * @return void */ - public function register() + public function register(): void { if (!is_null($this->registered)) { return; @@ -215,10 +202,8 @@ public function register() /** * De-register the given class loader on the auto-loader stack. - * - * @return void */ - public function unregister() + public function unregister(): void { if (is_null($this->registered)) { return; @@ -230,10 +215,8 @@ public function unregister() /** * Build the manifest and write it to disk. - * - * @return void */ - public function build() + public function build(): void { if (!$this->manifestDirty) { return; @@ -261,11 +244,8 @@ public function autoloadPackage(string $namespacePrefix, string $relativePath): * * Aliases are first-come, first-served. If a real class already exists with the same name as an alias, the real * class is used over the alias. - * - * @param array $aliases - * @return void */ - public function addAliases(array $aliases) + public function addAliases(array $aliases): void { foreach ($aliases as $original => $alias) { if (!array_key_exists($alias, $this->aliases)) { @@ -281,11 +261,8 @@ public function addAliases(array $aliases) * * Aliases are first-come, first-served. If a real class already exists with the same name as an alias, the real * class is used over the alias. - * - * @param array $namespaceAliases - * @return void */ - public function addNamespaceAliases(array $namespaceAliases) + public function addNamespaceAliases(array $namespaceAliases): void { foreach ($namespaceAliases as $original => $alias) { if (!array_key_exists($alias, $this->namespaceAliases)) { @@ -298,11 +275,8 @@ public function addNamespaceAliases(array $namespaceAliases) /** * Gets an alias for a class, if available. - * - * @param string $class - * @return string|null */ - public function getAlias($class) + public function getAlias(string $class): ?string { if (count($this->namespaceAliases)) { foreach ($this->namespaceAliases as $alias => $original) { @@ -319,11 +293,8 @@ public function getAlias($class) /** * Gets aliases registered for a namespace, if available. - * - * @param string $namespace - * @return array */ - public function getNamespaceAliases($namespace) + public function getNamespaceAliases(string $namespace): array { $aliases = []; foreach ($this->namespaceAliases as $alias => $original) { @@ -337,11 +308,8 @@ public function getNamespaceAliases($namespace) /** * Gets a reverse alias for a class, if available. - * - * @param string $class - * @return string|null */ - public function getReverseAlias($class) + public function getReverseAlias(string $class): ?string { if (count($this->namespaceAliases)) { foreach ($this->namespaceAliases as $alias => $original) { @@ -360,11 +328,8 @@ public function getReverseAlias($class) /** * Normalise the class name. - * - * @param string $class - * @return string */ - protected static function normalizeClass($class) + protected static function normalizeClass(string $class): string { /* * Strip first slash @@ -378,40 +343,10 @@ protected static function normalizeClass($class) }, explode('\\', $class))); } - /** - * Get the possible paths for a class. - * - * @param string $class - * @return array - */ - protected static function getPathsForClass($class) - { - /* - * Lowercase folders - */ - $parts = explode('\\', $class); - $file = array_pop($parts); - $namespace = implode('\\', $parts); - $directory = str_replace(['\\', '_'], DIRECTORY_SEPARATOR, $namespace); - - /* - * Provide both alternatives - */ - $lowerClass = strtolower($directory) . DIRECTORY_SEPARATOR . $file . '.php'; - $upperClass = $directory . DIRECTORY_SEPARATOR . $file . '.php'; - - $lowerClassStudlyFile = strtolower($directory) . DIRECTORY_SEPARATOR . Str::studly($file) . '.php'; - $upperClassStudlyFile = $directory . DIRECTORY_SEPARATOR . Str::studly($file) . '.php'; - - return [$lowerClass, $upperClass, $lowerClassStudlyFile, $upperClassStudlyFile]; - } - /** * Ensure the manifest has been loaded into memory. - * - * @return void */ - protected function ensureManifestIsLoaded() + protected function ensureManifestIsLoaded(): void { if (!is_null($this->manifest)) { return; @@ -424,15 +359,12 @@ protected function ensureManifestIsLoaded() if (!is_array($this->manifest)) { $this->manifest = []; } - } - catch (Exception $ex) { + } catch (Exception $ex) { $this->manifest = []; - } - catch (Throwable $ex) { + } catch (Throwable $ex) { $this->manifest = []; } - } - else { + } else { $this->manifest = []; } } @@ -440,11 +372,9 @@ protected function ensureManifestIsLoaded() /** * Write the given manifest array to disk. * - * @param array $manifest - * @return void - * @throws \Exception + * @throws \Exception if the manifest path is not writable */ - protected function write(array $manifest) + protected function write(array $manifest): void { if (!is_writable(dirname($this->manifestPath))) { throw new Exception('The storage/framework/cache directory must be present and writable.'); diff --git a/tests/Support/ClassLoaderTest.php b/tests/Support/ClassLoaderTest.php index baf16db44..626b0682c 100644 --- a/tests/Support/ClassLoaderTest.php +++ b/tests/Support/ClassLoaderTest.php @@ -19,6 +19,8 @@ public function setUp(): void ); $this->classLoader->register(); + + $this->classLoader->autoloadPackage('Winter\\Plugin', 'plugins/winter/plugin'); } public function tearDown(): void From 16b898b334aba8bec9581f515a75831d5c9bc3fc Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Tue, 29 Nov 2022 19:27:55 -0600 Subject: [PATCH 6/6] Namespace prefixes are case insensitive --- src/Support/ClassLoader.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Support/ClassLoader.php b/src/Support/ClassLoader.php index 92968f709..1645432cb 100644 --- a/src/Support/ClassLoader.php +++ b/src/Support/ClassLoader.php @@ -112,8 +112,9 @@ class_alias($class, $reverse); // Check our registered autoload packages for a match foreach ($this->autoloadedPackages as $prefix => $path) { - if (Str::startsWith($class, $prefix)) { - $parts = explode('\\', Str::after($class, $prefix)); + $lowerClass = strtolower($class); + if (Str::startsWith($lowerClass, $prefix)) { + $parts = explode('\\', substr($class, strlen($prefix))); $file = array_pop($parts) . '.php'; $namespace = implode('\\', $parts); $directory = str_replace(['\\', '_'], DIRECTORY_SEPARATOR, $namespace); @@ -230,7 +231,7 @@ public function build(): void */ public function autoloadPackage(string $namespacePrefix, string $relativePath): void { - $this->autoloadedPackages[$namespacePrefix] = $relativePath; + $this->autoloadedPackages[ltrim(Str::lower($namespacePrefix), '\\')] = $relativePath; // Ensure packages are sorted by length of the prefix to prevent a greedier prefix // from being matched first when attempting to autoload a class