From a8b528e789bc07f0fa9626210a6a7a8f9374f017 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 9 Jul 2023 17:12:50 +0900 Subject: [PATCH 01/27] style: break long lines --- system/Config/Factories.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 222df063196a..fa533d9a791b 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -125,7 +125,11 @@ public static function __callStatic(string $component, array $arguments) protected static function locateClass(array $options, string $name): ?string { // Check for low-hanging fruit - if (class_exists($name, false) && self::verifyPreferApp($options, $name) && self::verifyInstanceOf($options, $name)) { + if ( + class_exists($name, false) + && self::verifyPreferApp($options, $name) + && self::verifyInstanceOf($options, $name) + ) { return $name; } @@ -136,7 +140,10 @@ protected static function locateClass(array $options, string $name): ?string : rtrim(APP_NAMESPACE, '\\') . '\\' . $options['path'] . '\\' . $basename; // If an App version was requested then see if it verifies - if ($options['preferApp'] && class_exists($appname) && self::verifyInstanceOf($options, $name)) { + if ( + $options['preferApp'] && class_exists($appname) + && self::verifyInstanceOf($options, $name) + ) { return $appname; } From 85dd31b54817c22c7cceb5c532939a3845691aa1 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 10:48:07 +0900 Subject: [PATCH 02/27] refactor: extract isConfig() --- system/Config/Factories.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index fa533d9a791b..16be3abe0e2f 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -116,6 +116,14 @@ public static function __callStatic(string $component, array $arguments) return self::$instances[$options['component']][$class]; } + /** + * Is the component Config? + */ + private static function isConfig(string $component): bool + { + return $component === 'config'; + } + /** * Finds a component class * @@ -135,7 +143,7 @@ class_exists($name, false) // Determine the relative class names we need $basename = self::getBasename($name); - $appname = $options['component'] === 'config' + $appname = self::isConfig($options['component']) ? 'Config\\' . $basename : rtrim(APP_NAMESPACE, '\\') . '\\' . $options['path'] . '\\' . $basename; @@ -194,7 +202,7 @@ protected static function verifyPreferApp(array $options, string $name): bool } // Special case for Config since its App namespace is actually \Config - if ($options['component'] === 'config') { + if (self::isConfig($options['component'])) { return strpos($name, 'Config') === 0; } @@ -233,7 +241,7 @@ public static function getOptions(string $component): array return self::$options[$component]; } - $values = $component === 'config' + $values = self::isConfig($component) // Handle Config as a special case to prevent logic loops ? self::$configOptions // Load values from the best Factory configuration (will include Registrars) From 79d3f351c9459ac464b6c23b49ebbb4f5ecfcb20 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 13:51:52 +0900 Subject: [PATCH 03/27] docs: add doc comments --- system/Config/Factories.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 16be3abe0e2f..a0802bdf9bd2 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -65,6 +65,8 @@ class Factories * A multi-dimensional array with components as * keys to the array of name-indexed instances. * + * [component => [FQCN => instance]] + * * @var array> * @phpstan-var array> */ @@ -118,6 +120,8 @@ public static function __callStatic(string $component, array $arguments) /** * Is the component Config? + * + * @param string $component Lowercase, plural component name */ private static function isConfig(string $component): bool { @@ -231,6 +235,8 @@ protected static function verifyInstanceOf(array $options, string $name): bool * @param string $component Lowercase, plural component name * * @return array + * + * @internal For testing only */ public static function getOptions(string $component): array { @@ -247,6 +253,8 @@ public static function getOptions(string $component): array // Load values from the best Factory configuration (will include Registrars) : config(Factory::class)->{$component} ?? []; + // The setOptions() reset the component. So getOptions() may reset + // the component. return self::setOptions($component, $values); } @@ -254,6 +262,7 @@ public static function getOptions(string $component): array * Normalizes, stores, and returns the configuration for a specific component * * @param string $component Lowercase, plural component name + * @param array $values option values * * @return array The result after applying defaults and normalization */ @@ -305,6 +314,8 @@ public static function reset(?string $component = null) * * @param string $component Lowercase, plural component name * @param string $name The name of the instance + * + * @internal For testing only */ public static function injectMock(string $component, string $name, object $instance) { @@ -321,6 +332,8 @@ public static function injectMock(string $component, string $name, object $insta /** * Gets a basename from a class name, namespaced or not. + * + * @internal For testing only */ public static function getBasename(string $name): string { From d7ed0536b786e473ccb6570592cbfc8bc0b44bad Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 13:52:23 +0900 Subject: [PATCH 04/27] feat: [BC] add Factories::define() to override module classes Except for Config, if FQCN is specified, preferApp is ignored and that class is loaded. --- system/Config/Factories.php | 95 +++++++++++++++++++++--- tests/system/Config/FactoriesTest.php | 103 ++++++++++++++++++++++++-- 2 files changed, 183 insertions(+), 15 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index a0802bdf9bd2..c91d62d75b37 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -14,6 +14,7 @@ use CodeIgniter\Database\ConnectionInterface; use CodeIgniter\Model; use Config\Services; +use InvalidArgumentException; /** * Factories for creating instances. @@ -51,9 +52,11 @@ class Factories ]; /** - * Mapping of class basenames (no namespace) to + * Mapping of classnames (with or without namespace) to * their instances. * + * [component => [name => FQCN]] + * * @var array> * @phpstan-var array> */ @@ -72,6 +75,37 @@ class Factories */ protected static $instances = []; + /** + * Define the class to load. You can *override* the concrete class. + * + * @param string $component Lowercase, plural component name + * @param string $name Classname. The first parameter of Factories magic method + * @param string $classname FQCN to load + * @phpstan-param class-string $classname FQCN to load + */ + public static function define(string $component, string $name, string $classname): void + { + if (isset(self::$basenames[$component][$name])) { + if (self::$basenames[$component][$name] === $classname) { + return; + } + + throw new InvalidArgumentException( + 'Already defined in Factories: ' . $component . ' ' . $name . ' -> ' . self::$basenames[$component][$name] + ); + } + + if (! class_exists($classname)) { + throw new InvalidArgumentException('No such class: ' . $classname); + } + + // Force a configuration to exist for this component. + // Otherwise, getOptions() will reset the component. + self::getOptions($component); + + self::$basenames[$component][$name] = $classname; + } + /** * Loads instances based on the method component name. Either * creates a new instance or returns an existing shared instance. @@ -88,6 +122,12 @@ public static function __callStatic(string $component, array $arguments) $options = array_merge(self::getOptions(strtolower($component)), $options); if (! $options['getShared']) { + if (isset(self::$basenames[$component][$name])) { + $class = self::$basenames[$component][$name]; + + return new $class(...$arguments); + } + if ($class = self::locateClass($options, $name)) { return new $class(...$arguments); } @@ -95,15 +135,39 @@ public static function __callStatic(string $component, array $arguments) return null; } - $basename = self::getBasename($name); - // Check for an existing instance - if (isset(self::$basenames[$options['component']][$basename])) { - $class = self::$basenames[$options['component']][$basename]; + if (isset(self::$basenames[$options['component']][$name])) { + $class = self::$basenames[$options['component']][$name]; // Need to verify if the shared instance matches the request if (self::verifyInstanceOf($options, $class)) { + if (isset(self::$instances[$options['component']][$class])) { + return self::$instances[$options['component']][$class]; + } + self::$instances[$options['component']][$class] = new $class(...$arguments); + return self::$instances[$options['component']][$class]; + + } + } + + // Check for an existing Config instance with basename. + if (self::isConfig($options['component'])) { + $basename = self::getBasename($name); + + if (isset(self::$basenames[$options['component']][$basename])) { + $class = self::$basenames[$options['component']][$basename]; + + // Need to verify if the shared instance matches the request + if (self::verifyInstanceOf($options, $class)) { + if (isset(self::$instances[$options['component']][$class])) { + return self::$instances[$options['component']][$class]; + } + self::$instances[$options['component']][$class] = new $class(...$arguments); + + return self::$instances[$options['component']][$class]; + + } } } @@ -112,8 +176,13 @@ public static function __callStatic(string $component, array $arguments) return null; } - self::$instances[$options['component']][$class] = new $class(...$arguments); - self::$basenames[$options['component']][$basename] = $class; + self::$instances[$options['component']][$class] = new $class(...$arguments); + self::$basenames[$options['component']][$name] = $class; + + // If a short classname is specified, also register FQCN to share the instance. + if (! isset(self::$basenames[$options['component']][$class])) { + self::$basenames[$options['component']][$class] = $class; + } return self::$instances[$options['component']][$class]; } @@ -153,7 +222,9 @@ class_exists($name, false) // If an App version was requested then see if it verifies if ( - $options['preferApp'] && class_exists($appname) + // preferApp is used only for no namespace class or Config class. + (strpos($name, '\\') === false || self::isConfig($options['component'])) + && $options['preferApp'] && class_exists($appname) && self::verifyInstanceOf($options, $name) ) { return $appname; @@ -326,8 +397,12 @@ public static function injectMock(string $component, string $name, object $insta $class = get_class($instance); $basename = self::getBasename($name); - self::$instances[$component][$class] = $instance; - self::$basenames[$component][$basename] = $class; + self::$instances[$component][$class] = $instance; + self::$basenames[$component][$name] = $class; + + if (self::isConfig($component)) { + self::$basenames[$component][$basename] = $class; + } } /** diff --git a/tests/system/Config/FactoriesTest.php b/tests/system/Config/FactoriesTest.php index 655329ad95de..dc56c6535ab6 100644 --- a/tests/system/Config/FactoriesTest.php +++ b/tests/system/Config/FactoriesTest.php @@ -12,9 +12,13 @@ namespace CodeIgniter\Config; use CodeIgniter\Test\CIUnitTestCase; +use InvalidArgumentException; use ReflectionClass; use stdClass; +use Tests\Support\Config\TestRegistrar; +use Tests\Support\Models\EntityModel; use Tests\Support\Models\UserModel; +use Tests\Support\View\SampleClass; use Tests\Support\Widgets\OtherWidget; use Tests\Support\Widgets\SomeWidget; @@ -251,7 +255,33 @@ class_alias(SomeWidget::class, $class); $this->assertInstanceOf(SomeWidget::class, $result); } - public function testpreferAppOverridesClassname() + public function testPreferAppOverridesConfigClassname() + { + // Create a config class in App + $file = APPPATH . 'Config/TestRegistrar.php'; + $source = <<<'EOL' + assertInstanceOf('Config\TestRegistrar', $result); + + Factories::setOptions('config', ['preferApp' => false]); + + $result = Factories::config(TestRegistrar::class); + + $this->assertInstanceOf(TestRegistrar::class, $result); + + // Delete the config class in App + unlink($file); + } + + public function testPreferAppIsIgnored() { // Create a fake class in App $class = 'App\Widgets\OtherWidget'; @@ -260,11 +290,74 @@ class_alias(SomeWidget::class, $class); } $result = Factories::widgets(OtherWidget::class); - $this->assertInstanceOf(SomeWidget::class, $result); + $this->assertInstanceOf(OtherWidget::class, $result); + } - Factories::setOptions('widgets', ['preferApp' => false]); + public function testCanLoadTwoCellsWithSameShortName() + { + $cell1 = Factories::cells('\\' . SampleClass::class); + $cell2 = Factories::cells('\\' . \Tests\Support\View\OtherCells\SampleClass::class); - $result = Factories::widgets(OtherWidget::class); - $this->assertInstanceOf(OtherWidget::class, $result); + $this->assertNotSame($cell1, $cell2); + } + + public function testDefineTwice() + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage( + 'Already defined in Factories: models CodeIgniter\Shield\Models\UserModel -> Tests\Support\Models\UserModel' + ); + + Factories::define( + 'models', + 'CodeIgniter\Shield\Models\UserModel', + UserModel::class + ); + Factories::define( + 'models', + 'CodeIgniter\Shield\Models\UserModel', + EntityModel::class + ); + } + + public function testDefineNonExistentClass() + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('No such class: App\Models\UserModel'); + + Factories::define( + 'models', + 'CodeIgniter\Shield\Models\UserModel', + 'App\Models\UserModel' + ); + } + + public function testDefineAfterLoading() + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage( + 'Already defined in Factories: models Tests\Support\Models\UserModel -> Tests\Support\Models\UserModel' + ); + + model(UserModel::class); + + Factories::define( + 'models', + UserModel::class, + 'App\Models\UserModel' + ); + } + + public function testDefineAndLoad() + { + Factories::define( + 'models', + UserModel::class, + EntityModel::class + ); + + $model = model(UserModel::class); + + $this->assertInstanceOf(EntityModel::class, $model); } } From 510c73fea47b74d2947b98af9e8187d2a23ca9b1 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 16:41:03 +0900 Subject: [PATCH 05/27] chore: update psalm-baseline.xml for UndefinedClass --- psalm-baseline.xml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 41c5c1830d3f..dcef743f607f 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,5 +1,5 @@ - + Memcache @@ -104,7 +104,9 @@ - + + + From 8629fdd4154ea4e115c10146c66ffa01f326e1d4 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 17:08:12 +0900 Subject: [PATCH 06/27] test: update failed test --- tests/system/Test/FabricatorTest.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/system/Test/FabricatorTest.php b/tests/system/Test/FabricatorTest.php index 6f92be2aaaad..fdc2d8465a8b 100644 --- a/tests/system/Test/FabricatorTest.php +++ b/tests/system/Test/FabricatorTest.php @@ -11,7 +11,7 @@ namespace CodeIgniter\Test; -use CodeIgniter\Database\ModelFactory; +use CodeIgniter\Config\Factories; use Tests\Support\Models\EntityModel; use Tests\Support\Models\EventModel; use Tests\Support\Models\FabricatorModel; @@ -98,12 +98,16 @@ public function testConstructorUsesProvidedLocale() public function testModelUsesNewInstance() { - // Inject the wrong model for UserModel to show it is ignored by Fabricator + // Inject the wrong model for UserModel $mock = new FabricatorModel(); - ModelFactory::injectMock(UserModel::class, $mock); + Factories::injectMock('models', UserModel::class, $mock); $fabricator = new Fabricator(UserModel::class); - $this->assertInstanceOf(UserModel::class, $fabricator->getModel()); + + // Fabricator gets the instance from Factories, so it is FabricatorModel. + $this->assertInstanceOf(FabricatorModel::class, $fabricator->getModel()); + // But Fabricator creates a new instance. + $this->assertNotSame($mock, $fabricator->getModel()); } public function testGetModelReturnsModel() From 3f23901eee76442a97faf360673ca5fb1fd80eed Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 17:45:26 +0900 Subject: [PATCH 07/27] fix: when injecting Config mock with `Validation`, register the same instance as `Config\Validation` When you call `Factories::injectMock('config', 'Validation', $config)`, but if the production code calls `config(\Config\Validation::class)`, the mock instance will not be used and fail the test: CodeIgniter\Database\ModelFactoryTest::testBasenameReturnsExistingNamespaceInstance --- system/Config/Factories.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index c91d62d75b37..54620e3cf7a9 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -401,7 +401,11 @@ public static function injectMock(string $component, string $name, object $insta self::$basenames[$component][$name] = $class; if (self::isConfig($component)) { - self::$basenames[$component][$basename] = $class; + if ($name !== $basename) { + self::$basenames[$component][$basename] = $class; + } else { + self::$basenames[$component]['Config\\' . $basename] = $class; + } } } From 86ca97bec97d5bd2d6c785868d2d2484ed411d00 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 18:10:58 +0900 Subject: [PATCH 08/27] style: remove empty line --- system/Config/Factories.php | 1 - 1 file changed, 1 deletion(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 54620e3cf7a9..8d8fd148a89b 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -166,7 +166,6 @@ public static function __callStatic(string $component, array $arguments) self::$instances[$options['component']][$class] = new $class(...$arguments); return self::$instances[$options['component']][$class]; - } } } From 74177efa190da86270a4749b2278d693f2d49061 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 18:11:29 +0900 Subject: [PATCH 09/27] test: update failed test --- tests/system/Database/ModelFactoryTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/system/Database/ModelFactoryTest.php b/tests/system/Database/ModelFactoryTest.php index 4ec47a9b5967..1f69ee865bb5 100644 --- a/tests/system/Database/ModelFactoryTest.php +++ b/tests/system/Database/ModelFactoryTest.php @@ -66,12 +66,12 @@ public function testReset() $this->assertNull(ModelFactory::get('Banana')); } - public function testBasenameReturnsExistingNamespaceInstance() + public function testBasenameDoesNotReturnExistingNamespaceInstance() { ModelFactory::injectMock(UserModel::class, new JobModel()); $basenameModel = ModelFactory::get('UserModel'); - $this->assertInstanceOf(JobModel::class, $basenameModel); + $this->assertInstanceOf(UserModel::class, $basenameModel); } } From 88bbbcf906b5988ea448466896b8db48bd5e6d7d Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 24 Jul 2023 18:56:44 +0900 Subject: [PATCH 10/27] fix: preferApp is used with short classname only --- system/Config/Factories.php | 4 ++-- tests/system/Config/FactoriesTest.php | 24 ++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 8d8fd148a89b..a5e085c1d8aa 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -221,8 +221,8 @@ class_exists($name, false) // If an App version was requested then see if it verifies if ( - // preferApp is used only for no namespace class or Config class. - (strpos($name, '\\') === false || self::isConfig($options['component'])) + // preferApp is used only for no namespace class. + strpos($name, '\\') === false && $options['preferApp'] && class_exists($appname) && self::verifyInstanceOf($options, $name) ) { diff --git a/tests/system/Config/FactoriesTest.php b/tests/system/Config/FactoriesTest.php index dc56c6535ab6..1b3a68bbe707 100644 --- a/tests/system/Config/FactoriesTest.php +++ b/tests/system/Config/FactoriesTest.php @@ -255,7 +255,7 @@ class_alias(SomeWidget::class, $class); $this->assertInstanceOf(SomeWidget::class, $result); } - public function testPreferAppOverridesConfigClassname() + public function testShortnameReturnsConfigInApp() { // Create a config class in App $file = APPPATH . 'Config/TestRegistrar.php'; @@ -267,10 +267,30 @@ class TestRegistrar EOL; file_put_contents($file, $source); - $result = Factories::config(TestRegistrar::class); + $result = Factories::config('TestRegistrar'); $this->assertInstanceOf('Config\TestRegistrar', $result); + // Delete the config class in App + unlink($file); + } + + public function testFullClassnameIgnoresPreferApp() + { + // Create a config class in App + $file = APPPATH . 'Config/TestRegistrar.php'; + $source = <<<'EOL' + assertInstanceOf(TestRegistrar::class, $result); + Factories::setOptions('config', ['preferApp' => false]); $result = Factories::config(TestRegistrar::class); From da9ea306a692136945b21dc8c38352e87ed823bb Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 10:35:04 +0900 Subject: [PATCH 11/27] docs: introduce term "class alias" for docs --- system/Config/Factories.php | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index a5e085c1d8aa..14eb11e5483b 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -52,10 +52,14 @@ class Factories ]; /** - * Mapping of classnames (with or without namespace) to - * their instances. + * Mapping of class aliases to their true Full Qualified Class Name (FQCN). * - * [component => [name => FQCN]] + * Class aliases can be: + * - FQCN. E.g., 'App\Lib\SomeLib' + * - short classname. E.g., 'SomeLib' + * - short classname with sub-directories. E.g., 'Sub/SomeLib' + * + * [component => [alias => FQCN]] * * @var array> * @phpstan-var array> @@ -65,6 +69,7 @@ class Factories /** * Store for instances of any component that * has been requested as "shared". + * * A multi-dimensional array with components as * keys to the array of name-indexed instances. * @@ -79,7 +84,7 @@ class Factories * Define the class to load. You can *override* the concrete class. * * @param string $component Lowercase, plural component name - * @param string $name Classname. The first parameter of Factories magic method + * @param string $name Class alias. See the $basenames property. * @param string $classname FQCN to load * @phpstan-param class-string $classname FQCN to load */ @@ -114,7 +119,7 @@ public static function define(string $component, string $name, string $classname */ public static function __callStatic(string $component, array $arguments) { - // First argument is the name, second is options + // First argument is the class alias, second is options $name = trim(array_shift($arguments), '\\ '); $options = array_shift($arguments) ?? []; @@ -200,7 +205,7 @@ private static function isConfig(string $component): bool * Finds a component class * * @param array $options The array of component-specific directives - * @param string $name Class name, namespace optional + * @param string $name Class alias. See the $basenames property. */ protected static function locateClass(array $options, string $name): ?string { @@ -266,7 +271,7 @@ class_exists($name, false) * Verifies that a class & config satisfy the "preferApp" option * * @param array $options The array of component-specific directives - * @param string $name Class name, namespace optional + * @param string $name Class alias. See the $basenames property. */ protected static function verifyPreferApp(array $options, string $name): bool { @@ -287,7 +292,7 @@ protected static function verifyPreferApp(array $options, string $name): bool * Verifies that a class & config satisfy the "instanceOf" option * * @param array $options The array of component-specific directives - * @param string $name Class name, namespace optional + * @param string $name Class alias. See the $basenames property. */ protected static function verifyInstanceOf(array $options, string $name): bool { @@ -383,7 +388,7 @@ public static function reset(?string $component = null) * Helper method for injecting mock instances * * @param string $component Lowercase, plural component name - * @param string $name The name of the instance + * @param string $name Class alias. See the $basenames property. * * @internal For testing only */ From fd4d88c0b29abf285c62b075074876d9e3b86755 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 10:40:18 +0900 Subject: [PATCH 12/27] refactor: rename $basenames to $aliases --- system/Config/Factories.php | 48 ++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 14eb11e5483b..78d386820982 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -64,7 +64,7 @@ class Factories * @var array> * @phpstan-var array> */ - protected static $basenames = []; + protected static $aliases = []; /** * Store for instances of any component that @@ -84,19 +84,19 @@ class Factories * Define the class to load. You can *override* the concrete class. * * @param string $component Lowercase, plural component name - * @param string $name Class alias. See the $basenames property. + * @param string $name Class alias. See the $aliases property. * @param string $classname FQCN to load * @phpstan-param class-string $classname FQCN to load */ public static function define(string $component, string $name, string $classname): void { - if (isset(self::$basenames[$component][$name])) { - if (self::$basenames[$component][$name] === $classname) { + if (isset(self::$aliases[$component][$name])) { + if (self::$aliases[$component][$name] === $classname) { return; } throw new InvalidArgumentException( - 'Already defined in Factories: ' . $component . ' ' . $name . ' -> ' . self::$basenames[$component][$name] + 'Already defined in Factories: ' . $component . ' ' . $name . ' -> ' . self::$aliases[$component][$name] ); } @@ -108,7 +108,7 @@ public static function define(string $component, string $name, string $classname // Otherwise, getOptions() will reset the component. self::getOptions($component); - self::$basenames[$component][$name] = $classname; + self::$aliases[$component][$name] = $classname; } /** @@ -127,8 +127,8 @@ public static function __callStatic(string $component, array $arguments) $options = array_merge(self::getOptions(strtolower($component)), $options); if (! $options['getShared']) { - if (isset(self::$basenames[$component][$name])) { - $class = self::$basenames[$component][$name]; + if (isset(self::$aliases[$component][$name])) { + $class = self::$aliases[$component][$name]; return new $class(...$arguments); } @@ -141,8 +141,8 @@ public static function __callStatic(string $component, array $arguments) } // Check for an existing instance - if (isset(self::$basenames[$options['component']][$name])) { - $class = self::$basenames[$options['component']][$name]; + if (isset(self::$aliases[$options['component']][$name])) { + $class = self::$aliases[$options['component']][$name]; // Need to verify if the shared instance matches the request if (self::verifyInstanceOf($options, $class)) { @@ -160,8 +160,8 @@ public static function __callStatic(string $component, array $arguments) if (self::isConfig($options['component'])) { $basename = self::getBasename($name); - if (isset(self::$basenames[$options['component']][$basename])) { - $class = self::$basenames[$options['component']][$basename]; + if (isset(self::$aliases[$options['component']][$basename])) { + $class = self::$aliases[$options['component']][$basename]; // Need to verify if the shared instance matches the request if (self::verifyInstanceOf($options, $class)) { @@ -181,11 +181,11 @@ public static function __callStatic(string $component, array $arguments) } self::$instances[$options['component']][$class] = new $class(...$arguments); - self::$basenames[$options['component']][$name] = $class; + self::$aliases[$options['component']][$name] = $class; // If a short classname is specified, also register FQCN to share the instance. - if (! isset(self::$basenames[$options['component']][$class])) { - self::$basenames[$options['component']][$class] = $class; + if (! isset(self::$aliases[$options['component']][$class])) { + self::$aliases[$options['component']][$class] = $class; } return self::$instances[$options['component']][$class]; @@ -205,7 +205,7 @@ private static function isConfig(string $component): bool * Finds a component class * * @param array $options The array of component-specific directives - * @param string $name Class alias. See the $basenames property. + * @param string $name Class alias. See the $aliases property. */ protected static function locateClass(array $options, string $name): ?string { @@ -271,7 +271,7 @@ class_exists($name, false) * Verifies that a class & config satisfy the "preferApp" option * * @param array $options The array of component-specific directives - * @param string $name Class alias. See the $basenames property. + * @param string $name Class alias. See the $aliases property. */ protected static function verifyPreferApp(array $options, string $name): bool { @@ -292,7 +292,7 @@ protected static function verifyPreferApp(array $options, string $name): bool * Verifies that a class & config satisfy the "instanceOf" option * * @param array $options The array of component-specific directives - * @param string $name Class alias. See the $basenames property. + * @param string $name Class alias. See the $aliases property. */ protected static function verifyInstanceOf(array $options, string $name): bool { @@ -372,7 +372,7 @@ public static function reset(?string $component = null) if ($component) { unset( static::$options[$component], - static::$basenames[$component], + static::$aliases[$component], static::$instances[$component] ); @@ -380,7 +380,7 @@ public static function reset(?string $component = null) } static::$options = []; - static::$basenames = []; + static::$aliases = []; static::$instances = []; } @@ -388,7 +388,7 @@ public static function reset(?string $component = null) * Helper method for injecting mock instances * * @param string $component Lowercase, plural component name - * @param string $name Class alias. See the $basenames property. + * @param string $name Class alias. See the $aliases property. * * @internal For testing only */ @@ -402,13 +402,13 @@ public static function injectMock(string $component, string $name, object $insta $basename = self::getBasename($name); self::$instances[$component][$class] = $instance; - self::$basenames[$component][$name] = $class; + self::$aliases[$component][$name] = $class; if (self::isConfig($component)) { if ($name !== $basename) { - self::$basenames[$component][$basename] = $class; + self::$aliases[$component][$basename] = $class; } else { - self::$basenames[$component]['Config\\' . $basename] = $class; + self::$aliases[$component]['Config\\' . $basename] = $class; } } } From 0fac66c50337b4912142448d293e7efc5b8c1dba Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 10:42:26 +0900 Subject: [PATCH 13/27] refactor: rename $name to $alias --- system/Config/Factories.php | 90 ++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 78d386820982..727c0bbb5512 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -25,7 +25,7 @@ * instantiation checks. * * @method static BaseConfig|null config(...$arguments) - * @method static Model|null models(string $name, array $options = [], ?ConnectionInterface &$conn = null) + * @method static Model|null models(string $alias, array $options = [], ?ConnectionInterface &$conn = null) */ class Factories { @@ -84,19 +84,19 @@ class Factories * Define the class to load. You can *override* the concrete class. * * @param string $component Lowercase, plural component name - * @param string $name Class alias. See the $aliases property. + * @param string $alias Class alias. See the $aliases property. * @param string $classname FQCN to load * @phpstan-param class-string $classname FQCN to load */ - public static function define(string $component, string $name, string $classname): void + public static function define(string $component, string $alias, string $classname): void { - if (isset(self::$aliases[$component][$name])) { - if (self::$aliases[$component][$name] === $classname) { + if (isset(self::$aliases[$component][$alias])) { + if (self::$aliases[$component][$alias] === $classname) { return; } throw new InvalidArgumentException( - 'Already defined in Factories: ' . $component . ' ' . $name . ' -> ' . self::$aliases[$component][$name] + 'Already defined in Factories: ' . $component . ' ' . $alias . ' -> ' . self::$aliases[$component][$alias] ); } @@ -108,7 +108,7 @@ public static function define(string $component, string $name, string $classname // Otherwise, getOptions() will reset the component. self::getOptions($component); - self::$aliases[$component][$name] = $classname; + self::$aliases[$component][$alias] = $classname; } /** @@ -120,20 +120,20 @@ public static function define(string $component, string $name, string $classname public static function __callStatic(string $component, array $arguments) { // First argument is the class alias, second is options - $name = trim(array_shift($arguments), '\\ '); + $alias = trim(array_shift($arguments), '\\ '); $options = array_shift($arguments) ?? []; // Determine the component-specific options $options = array_merge(self::getOptions(strtolower($component)), $options); if (! $options['getShared']) { - if (isset(self::$aliases[$component][$name])) { - $class = self::$aliases[$component][$name]; + if (isset(self::$aliases[$component][$alias])) { + $class = self::$aliases[$component][$alias]; return new $class(...$arguments); } - if ($class = self::locateClass($options, $name)) { + if ($class = self::locateClass($options, $alias)) { return new $class(...$arguments); } @@ -141,8 +141,8 @@ public static function __callStatic(string $component, array $arguments) } // Check for an existing instance - if (isset(self::$aliases[$options['component']][$name])) { - $class = self::$aliases[$options['component']][$name]; + if (isset(self::$aliases[$options['component']][$alias])) { + $class = self::$aliases[$options['component']][$alias]; // Need to verify if the shared instance matches the request if (self::verifyInstanceOf($options, $class)) { @@ -158,7 +158,7 @@ public static function __callStatic(string $component, array $arguments) // Check for an existing Config instance with basename. if (self::isConfig($options['component'])) { - $basename = self::getBasename($name); + $basename = self::getBasename($alias); if (isset(self::$aliases[$options['component']][$basename])) { $class = self::$aliases[$options['component']][$basename]; @@ -176,12 +176,12 @@ public static function __callStatic(string $component, array $arguments) } // Try to locate the class - if (! $class = self::locateClass($options, $name)) { + if (! $class = self::locateClass($options, $alias)) { return null; } self::$instances[$options['component']][$class] = new $class(...$arguments); - self::$aliases[$options['component']][$name] = $class; + self::$aliases[$options['component']][$alias] = $class; // If a short classname is specified, also register FQCN to share the instance. if (! isset(self::$aliases[$options['component']][$class])) { @@ -205,21 +205,21 @@ private static function isConfig(string $component): bool * Finds a component class * * @param array $options The array of component-specific directives - * @param string $name Class alias. See the $aliases property. + * @param string $alias Class alias. See the $aliases property. */ - protected static function locateClass(array $options, string $name): ?string + protected static function locateClass(array $options, string $alias): ?string { // Check for low-hanging fruit if ( - class_exists($name, false) - && self::verifyPreferApp($options, $name) - && self::verifyInstanceOf($options, $name) + class_exists($alias, false) + && self::verifyPreferApp($options, $alias) + && self::verifyInstanceOf($options, $alias) ) { - return $name; + return $alias; } // Determine the relative class names we need - $basename = self::getBasename($name); + $basename = self::getBasename($alias); $appname = self::isConfig($options['component']) ? 'Config\\' . $basename : rtrim(APP_NAMESPACE, '\\') . '\\' . $options['path'] . '\\' . $basename; @@ -227,31 +227,31 @@ class_exists($name, false) // If an App version was requested then see if it verifies if ( // preferApp is used only for no namespace class. - strpos($name, '\\') === false + strpos($alias, '\\') === false && $options['preferApp'] && class_exists($appname) - && self::verifyInstanceOf($options, $name) + && self::verifyInstanceOf($options, $alias) ) { return $appname; } // If we have ruled out an App version and the class exists then try it - if (class_exists($name) && self::verifyInstanceOf($options, $name)) { - return $name; + if (class_exists($alias) && self::verifyInstanceOf($options, $alias)) { + return $alias; } // Have to do this the hard way... $locator = Services::locator(); // Check if the class was namespaced - if (strpos($name, '\\') !== false) { - if (! $file = $locator->locateFile($name, $options['path'])) { + if (strpos($alias, '\\') !== false) { + if (! $file = $locator->locateFile($alias, $options['path'])) { return null; } $files = [$file]; } // No namespace? Search for it // Check all namespaces, prioritizing App and modules - elseif (! $files = $locator->search($options['path'] . DIRECTORY_SEPARATOR . $name)) { + elseif (! $files = $locator->search($options['path'] . DIRECTORY_SEPARATOR . $alias)) { return null; } @@ -271,9 +271,9 @@ class_exists($name, false) * Verifies that a class & config satisfy the "preferApp" option * * @param array $options The array of component-specific directives - * @param string $name Class alias. See the $aliases property. + * @param string $alias Class alias. See the $aliases property. */ - protected static function verifyPreferApp(array $options, string $name): bool + protected static function verifyPreferApp(array $options, string $alias): bool { // Anything without that restriction passes if (! $options['preferApp']) { @@ -282,26 +282,26 @@ protected static function verifyPreferApp(array $options, string $name): bool // Special case for Config since its App namespace is actually \Config if (self::isConfig($options['component'])) { - return strpos($name, 'Config') === 0; + return strpos($alias, 'Config') === 0; } - return strpos($name, APP_NAMESPACE) === 0; + return strpos($alias, APP_NAMESPACE) === 0; } /** * Verifies that a class & config satisfy the "instanceOf" option * * @param array $options The array of component-specific directives - * @param string $name Class alias. See the $aliases property. + * @param string $alias Class alias. See the $aliases property. */ - protected static function verifyInstanceOf(array $options, string $name): bool + protected static function verifyInstanceOf(array $options, string $alias): bool { // Anything without that restriction passes if (! $options['instanceOf']) { return true; } - return is_a($name, $options['instanceOf'], true); + return is_a($alias, $options['instanceOf'], true); } /** @@ -388,24 +388,24 @@ public static function reset(?string $component = null) * Helper method for injecting mock instances * * @param string $component Lowercase, plural component name - * @param string $name Class alias. See the $aliases property. + * @param string $alias Class alias. See the $aliases property. * * @internal For testing only */ - public static function injectMock(string $component, string $name, object $instance) + public static function injectMock(string $component, string $alias, object $instance) { // Force a configuration to exist for this component $component = strtolower($component); self::getOptions($component); $class = get_class($instance); - $basename = self::getBasename($name); + $basename = self::getBasename($alias); self::$instances[$component][$class] = $instance; - self::$aliases[$component][$name] = $class; + self::$aliases[$component][$alias] = $class; if (self::isConfig($component)) { - if ($name !== $basename) { + if ($alias !== $basename) { self::$aliases[$component][$basename] = $class; } else { self::$aliases[$component]['Config\\' . $basename] = $class; @@ -418,13 +418,13 @@ public static function injectMock(string $component, string $name, object $insta * * @internal For testing only */ - public static function getBasename(string $name): string + public static function getBasename(string $alias): string { // Determine the basename - if ($basename = strrchr($name, '\\')) { + if ($basename = strrchr($alias, '\\')) { return substr($basename, 1); } - return $name; + return $alias; } } From d385e98ef1f3f9e3d32d87e48ea1e3d7aabebab9 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 10:47:30 +0900 Subject: [PATCH 14/27] docs: improve doc comments --- system/Config/Factories.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 727c0bbb5512..e48307920fbc 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -85,8 +85,8 @@ class Factories * * @param string $component Lowercase, plural component name * @param string $alias Class alias. See the $aliases property. - * @param string $classname FQCN to load - * @phpstan-param class-string $classname FQCN to load + * @param string $classname FQCN to be loaded + * @phpstan-param class-string $classname FQCN to be loaded */ public static function define(string $component, string $alias, string $classname): void { From 88803513cce5e039c95ff5d20b8345136250bb04 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 10:54:42 +0900 Subject: [PATCH 15/27] refactor: extract isNamespaced() --- system/Config/Factories.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index e48307920fbc..bcb249ba387b 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -242,8 +242,8 @@ class_exists($alias, false) // Have to do this the hard way... $locator = Services::locator(); - // Check if the class was namespaced - if (strpos($alias, '\\') !== false) { + // Check if the class alias was namespaced + if (self::isNamespaced($alias)) { if (! $file = $locator->locateFile($alias, $options['path'])) { return null; } @@ -267,6 +267,16 @@ class_exists($alias, false) return null; } + /** + * Is the class alias namespaced or not? + * + * @param string $alias Class alias. See the $aliases property. + */ + private static function isNamespaced(string $alias): bool + { + return strpos($alias, '\\') !== false; + } + /** * Verifies that a class & config satisfy the "preferApp" option * @@ -414,7 +424,7 @@ public static function injectMock(string $component, string $alias, object $inst } /** - * Gets a basename from a class name, namespaced or not. + * Gets a basename from a class alias, namespaced or not. * * @internal For testing only */ From a1fa03f642fbd09ecbe403dc67be95397998f8fb Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 11:04:59 +0900 Subject: [PATCH 16/27] docs: add @testTag --- system/Config/Factories.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index bcb249ba387b..7430ecdd0dad 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -322,6 +322,7 @@ protected static function verifyInstanceOf(array $options, string $alias): bool * @return array * * @internal For testing only + * @testTag */ public static function getOptions(string $component): array { @@ -401,6 +402,7 @@ public static function reset(?string $component = null) * @param string $alias Class alias. See the $aliases property. * * @internal For testing only + * @testTag */ public static function injectMock(string $component, string $alias, object $instance) { @@ -427,6 +429,7 @@ public static function injectMock(string $component, string $alias, object $inst * Gets a basename from a class alias, namespaced or not. * * @internal For testing only + * @testTag */ public static function getBasename(string $alias): string { From 5b05bf93f62a75ae9ad191f8516352ed67f8d0bc Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 16:20:48 +0900 Subject: [PATCH 17/27] refactor: use isNamespaced() --- system/Config/Factories.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 7430ecdd0dad..93a2193a12bf 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -410,17 +410,16 @@ public static function injectMock(string $component, string $alias, object $inst $component = strtolower($component); self::getOptions($component); - $class = get_class($instance); - $basename = self::getBasename($alias); + $class = get_class($instance); self::$instances[$component][$class] = $instance; self::$aliases[$component][$alias] = $class; if (self::isConfig($component)) { - if ($alias !== $basename) { - self::$aliases[$component][$basename] = $class; + if (self::isNamespaced($alias)) { + self::$aliases[$component][self::getBasename($alias)] = $class; } else { - self::$aliases[$component]['Config\\' . $basename] = $class; + self::$aliases[$component]['Config\\' . $alias] = $class; } } } From b48277466480b2252480fbec3de0ca9574f64948 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 25 Jul 2023 16:27:38 +0900 Subject: [PATCH 18/27] refactor: use isNamespaced() --- system/Config/Factories.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 93a2193a12bf..f75c87d59cd9 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -226,8 +226,8 @@ class_exists($alias, false) // If an App version was requested then see if it verifies if ( - // preferApp is used only for no namespace class. - strpos($alias, '\\') === false + // preferApp is used only for no namespaced class. + ! self::isNamespaced($alias) && $options['preferApp'] && class_exists($appname) && self::verifyInstanceOf($options, $alias) ) { From a3c768ab4b37a0082344ad4121ab260b4bea3bf7 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 26 Jul 2023 09:10:45 +0900 Subject: [PATCH 19/27] test: add test --- tests/system/Config/FactoriesTest.php | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/system/Config/FactoriesTest.php b/tests/system/Config/FactoriesTest.php index 1b3a68bbe707..ffbf8b7b8a83 100644 --- a/tests/system/Config/FactoriesTest.php +++ b/tests/system/Config/FactoriesTest.php @@ -321,7 +321,7 @@ public function testCanLoadTwoCellsWithSameShortName() $this->assertNotSame($cell1, $cell2); } - public function testDefineTwice() + public function testDefineSameAliasTwiceWithDifferentClasses() { $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage( @@ -340,6 +340,24 @@ public function testDefineTwice() ); } + public function testDefineSameAliasAndSameClassTwice() + { + Factories::define( + 'models', + 'CodeIgniter\Shield\Models\UserModel', + UserModel::class + ); + Factories::define( + 'models', + 'CodeIgniter\Shield\Models\UserModel', + UserModel::class + ); + + $model = model('CodeIgniter\Shield\Models\UserModel'); + + $this->assertInstanceOf(UserModel::class, $model); + } + public function testDefineNonExistentClass() { $this->expectException(InvalidArgumentException::class); From e3c223bbd96b2f3697e73c32bb71d78ac6c3f0de Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 26 Jul 2023 09:20:04 +0900 Subject: [PATCH 20/27] docs: update comments --- system/Config/Factories.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index f75c87d59cd9..94fa7cf71330 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -140,15 +140,17 @@ public static function __callStatic(string $component, array $arguments) return null; } - // Check for an existing instance + // Check for an existing definition if (isset(self::$aliases[$options['component']][$alias])) { $class = self::$aliases[$options['component']][$alias]; // Need to verify if the shared instance matches the request if (self::verifyInstanceOf($options, $class)) { + // Check for an existing instance if (isset(self::$instances[$options['component']][$class])) { return self::$instances[$options['component']][$class]; } + self::$instances[$options['component']][$class] = new $class(...$arguments); return self::$instances[$options['component']][$class]; @@ -156,7 +158,7 @@ public static function __callStatic(string $component, array $arguments) } } - // Check for an existing Config instance with basename. + // Check for an existing Config definition with basename. if (self::isConfig($options['component'])) { $basename = self::getBasename($alias); @@ -165,9 +167,11 @@ public static function __callStatic(string $component, array $arguments) // Need to verify if the shared instance matches the request if (self::verifyInstanceOf($options, $class)) { + // Check for an existing instance if (isset(self::$instances[$options['component']][$class])) { return self::$instances[$options['component']][$class]; } + self::$instances[$options['component']][$class] = new $class(...$arguments); return self::$instances[$options['component']][$class]; From 244c5fbe7b76bce549e868c588f7fa9195d3268f Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 26 Jul 2023 09:34:34 +0900 Subject: [PATCH 21/27] refactor: extract getDefinedInstance() --- system/Config/Factories.php | 61 +++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index 94fa7cf71330..e122a650d366 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -141,41 +141,18 @@ public static function __callStatic(string $component, array $arguments) } // Check for an existing definition - if (isset(self::$aliases[$options['component']][$alias])) { - $class = self::$aliases[$options['component']][$alias]; - - // Need to verify if the shared instance matches the request - if (self::verifyInstanceOf($options, $class)) { - // Check for an existing instance - if (isset(self::$instances[$options['component']][$class])) { - return self::$instances[$options['component']][$class]; - } - - self::$instances[$options['component']][$class] = new $class(...$arguments); - - return self::$instances[$options['component']][$class]; - - } + $instance = self::getDefinedInstance($options, $alias, $arguments); + if ($instance !== null) { + return $instance; } // Check for an existing Config definition with basename. if (self::isConfig($options['component'])) { $basename = self::getBasename($alias); - if (isset(self::$aliases[$options['component']][$basename])) { - $class = self::$aliases[$options['component']][$basename]; - - // Need to verify if the shared instance matches the request - if (self::verifyInstanceOf($options, $class)) { - // Check for an existing instance - if (isset(self::$instances[$options['component']][$class])) { - return self::$instances[$options['component']][$class]; - } - - self::$instances[$options['component']][$class] = new $class(...$arguments); - - return self::$instances[$options['component']][$class]; - } + $instance = self::getDefinedInstance($options, $basename, $arguments); + if ($instance !== null) { + return $instance; } } @@ -195,6 +172,32 @@ public static function __callStatic(string $component, array $arguments) return self::$instances[$options['component']][$class]; } + /** + * Gets the defined instance. If not exists, creates new one. + * + * @return object|null + */ + private static function getDefinedInstance(array $options, string $alias, array $arguments) + { + if (isset(self::$aliases[$options['component']][$alias])) { + $class = self::$aliases[$options['component']][$alias]; + + // Need to verify if the shared instance matches the request + if (self::verifyInstanceOf($options, $class)) { + // Check for an existing instance + if (isset(self::$instances[$options['component']][$class])) { + return self::$instances[$options['component']][$class]; + } + + self::$instances[$options['component']][$class] = new $class(...$arguments); + + return self::$instances[$options['component']][$class]; + } + } + + return null; + } + /** * Is the component Config? * From 340a930dcfcd088a6b094747a7b44e724c5dc855 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 26 Jul 2023 09:42:14 +0900 Subject: [PATCH 22/27] fix: remove logic for preferApp for no namespaced Config classname preferApp should work only for no namespaced (Config) classname. --- system/Config/Factories.php | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index e122a650d366..a33079bec4d5 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -146,16 +146,6 @@ public static function __callStatic(string $component, array $arguments) return $instance; } - // Check for an existing Config definition with basename. - if (self::isConfig($options['component'])) { - $basename = self::getBasename($alias); - - $instance = self::getDefinedInstance($options, $basename, $arguments); - if ($instance !== null) { - return $instance; - } - } - // Try to locate the class if (! $class = self::locateClass($options, $alias)) { return null; From a13c53d5c1ed72544d787df3978196cdaf44ea6c Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 26 Jul 2023 10:20:59 +0900 Subject: [PATCH 23/27] docs: update existing description --- user_guide_src/source/concepts/factories.rst | 48 +++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/user_guide_src/source/concepts/factories.rst b/user_guide_src/source/concepts/factories.rst index b3ec0a064176..0c97052c9a4d 100644 --- a/user_guide_src/source/concepts/factories.rst +++ b/user_guide_src/source/concepts/factories.rst @@ -54,9 +54,18 @@ by using the magic static method of the Factories class, ``Factories::models()`` The static method name is called *component*. -By default, Factories first searches in the ``App`` namespace for the path corresponding to the magic static method name. +.. _factories-passing-classname-without-namespace: + +Passing Classname without Namespace +----------------------------------- + +If you pass a classname without a namespace, Factories first searches in the +``App`` namespace for the path corresponding to the magic static method name. ``Factories::models()`` searches the **app/Models** directory. +Passing Short Classname +^^^^^^^^^^^^^^^^^^^^^^^ + In the following code, if you have ``App\Models\UserModel``, the instance will be returned: .. literalinclude:: factories/001.php @@ -68,31 +77,35 @@ you get back the instance as before: .. literalinclude:: factories/003.php -preferApp option ----------------- +Passing Short Classname with Sub-directories +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -You could also request a specific class: +If you want to load a class in sub directories, you use the ``/`` as a separator. +The following code loads **app/Libraries/Sub/SubLib.php** if it exists: -.. literalinclude:: factories/002.php +.. literalinclude:: factories/013.php :lines: 2- -If you have only ``Blog\Models\UserModel``, the instance will be returned. -But if you have both ``App\Models\UserModel`` and ``Blog\Models\UserModel``, -the instance of ``App\Models\UserModel`` will be returned. +Passing Full Qualified Classname +-------------------------------- -If you want to get ``Blog\Models\UserModel``, you need to disable the option ``preferApp``: +You could also request a full qualified classname: -.. literalinclude:: factories/010.php +.. literalinclude:: factories/002.php :lines: 2- -Loading a Class in Sub-directories -================================== +It returns the instance of ``Blog\Models\UserModel`` if it exists. -If you want to load a class in sub directories, you use the ``/`` as a separator. -The following code loads **app/Libraries/Sub/SubLib.php**: +.. note:: Prior to v4.4.0, when you requested a full qualified classname, + if you had only ``Blog\Models\UserModel``, the instance would be returned. + But if you had both ``App\Models\UserModel`` and ``Blog\Models\UserModel``, + the instance of ``App\Models\UserModel`` would be returned. -.. literalinclude:: factories/013.php - :lines: 2- + If you wanted to get ``Blog\Models\UserModel``, you needed to disable the + option ``preferApp``: + + .. literalinclude:: factories/010.php + :lines: 2- Convenience Functions ********************* @@ -154,6 +167,9 @@ preferApp boolean Whether a class with the same basename in the App name overrides other explicit class requests. ========== ============== ============================================================ =================================================== +.. note:: Since v4.4.0, ``preferApp`` works only when you request + :ref:`a classname without a namespace `. + Factories Behavior ****************** From 886fa8703a7be8312497be1cdaadbe78b9125546 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 26 Jul 2023 10:44:28 +0900 Subject: [PATCH 24/27] docs: add about Factories::define() --- user_guide_src/source/concepts/factories.rst | 23 +++++++++++++++++++ .../source/concepts/factories/014.php | 3 +++ .../source/concepts/factories/015.php | 3 +++ 3 files changed, 29 insertions(+) create mode 100644 user_guide_src/source/concepts/factories/014.php create mode 100644 user_guide_src/source/concepts/factories/015.php diff --git a/user_guide_src/source/concepts/factories.rst b/user_guide_src/source/concepts/factories.rst index 0c97052c9a4d..881f0fa1b1f1 100644 --- a/user_guide_src/source/concepts/factories.rst +++ b/user_guide_src/source/concepts/factories.rst @@ -128,6 +128,29 @@ The second function, :php:func:`model()` returns a new instance of a Model class .. literalinclude:: factories/009.php +.. _factories-defining-classname-to-be-loaded: + +Defining Classname to be Loaded +******************************* + +.. versionadded:: 4.4.0 + +You could define a classname to be loaded before loading the class with +the ``Factories::define()`` method: + +.. literalinclude:: factories/014.php + :lines: 2- + +The first parameter is a component. The second parameter is a class alias +(the first parameter to Factories magic static method), and the third parameter +is the true full qualified classname to be loaded. + +After that, if you load ``Myth\Auth\Models\UserModel`` with Factories, the +``App\Models\UserModel`` instance will be returned: + +.. literalinclude:: factories/015.php + :lines: 2- + Factory Parameters ****************** diff --git a/user_guide_src/source/concepts/factories/014.php b/user_guide_src/source/concepts/factories/014.php new file mode 100644 index 000000000000..992b0f7b2236 --- /dev/null +++ b/user_guide_src/source/concepts/factories/014.php @@ -0,0 +1,3 @@ + Date: Wed, 26 Jul 2023 11:28:42 +0900 Subject: [PATCH 25/27] docs: add changelog and upgrade guide --- user_guide_src/source/changelogs/v4.4.0.rst | 31 +++++++++++++++++++ .../source/installation/upgrade_440.rst | 15 +++++++++ 2 files changed, 46 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.4.0.rst b/user_guide_src/source/changelogs/v4.4.0.rst index 475d08e9eaea..6eb96418c6ae 100644 --- a/user_guide_src/source/changelogs/v4.4.0.rst +++ b/user_guide_src/source/changelogs/v4.4.0.rst @@ -29,6 +29,35 @@ or more was specified. See :ref:`upgrade-440-uri-setsegment`. The next segment (``+1``) of the current last segment can be set as before. +.. _v440-factories: + +Factories +--------- + +Passing Fully Qualified Classname +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Now ``preferApp`` works only when you request +:ref:`a classname without a namespace `. + +For example, when you call ``model(\Myth\Auth\Models\UserModel::class)`` or +``model('Myth\Auth\Models\UserModel')``: + + - before: + + - returns ``App\Models\UserModel`` if exists and ``preferApp`` is true (default) + - returns ``Myth\Auth\Models\UserModel`` if exists and ``preferApp`` is false + + - after: + + - returns ``Myth\Auth\Models\UserModel`` even if ``preferApp`` is true (default) + - returns ``App\Models\UserModel`` if you define ``Factories::define('models', 'Myth\Auth\Models\UserModel', 'App\Models\UserModel')`` before calling the ``model()`` + +Property Name +^^^^^^^^^^^^^ + +The property ``Factories::$basenames`` has been renamed to ``$aliases``. + .. _v440-interface-changes: Interface Changes @@ -164,6 +193,8 @@ Others - **RedirectException:** can also take an object that implements ResponseInterface as its first argument. - **RedirectException:** implements ResponsableInterface. - **DebugBar:** Now :ref:`view-routes` are displayed in *DEFINED ROUTES* on the *Routes* tab. +- **Factories:** You can now define the classname that will actually be loaded. + See :ref:`factories-defining-classname-to-be-loaded`. Message Changes *************** diff --git a/user_guide_src/source/installation/upgrade_440.rst b/user_guide_src/source/installation/upgrade_440.rst index a64e35813a4a..3fc02a179330 100644 --- a/user_guide_src/source/installation/upgrade_440.rst +++ b/user_guide_src/source/installation/upgrade_440.rst @@ -73,6 +73,21 @@ This bug was fixed and now URIs for underscores (**foo_bar**) is not accessible. If you have links to URIs for underscores (**foo_bar**), update them with URIs for dashes (**foo-bar**). +When Passing Fully Qualified Classnames to Factories +==================================================== + +The behavior of passing fully qualified classnames to Factories has been changed. +See :ref:`ChangeLog ` for details. + +If you have code like ``model('\Myth\Auth\Models\UserModel::class')`` or +``model('Myth\Auth\Models\UserModel')`` (the code may be in the third-party packages), +and you expect to load your ``App\Models\UserModel``, you need to define the +classname to be loaded before the first loading of that class:: + + Factories::define('models', 'Myth\Auth\Models\UserModel', 'App\Models\UserModel'); + +See :ref:`factories-defining-classname-to-be-loaded` for details. + Interface Changes ================= From c48b5239b4c6ace84bf754ca134255a79180d057 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 26 Jul 2023 11:29:54 +0900 Subject: [PATCH 26/27] docs: fix typo --- system/Config/Factories.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Config/Factories.php b/system/Config/Factories.php index a33079bec4d5..67a31728d65c 100644 --- a/system/Config/Factories.php +++ b/system/Config/Factories.php @@ -52,7 +52,7 @@ class Factories ]; /** - * Mapping of class aliases to their true Full Qualified Class Name (FQCN). + * Mapping of class aliases to their true Fully Qualified Class Name (FQCN). * * Class aliases can be: * - FQCN. E.g., 'App\Lib\SomeLib' From 89d6f3788741440b0b2035527bb883b957810a93 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 28 Jul 2023 11:21:54 +0900 Subject: [PATCH 27/27] docs: update existing descriptions --- user_guide_src/source/general/configuration.rst | 5 +++-- user_guide_src/source/general/modules.rst | 9 +++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/user_guide_src/source/general/configuration.rst b/user_guide_src/source/general/configuration.rst index 2e666c8245c8..e51ab1e8fa40 100644 --- a/user_guide_src/source/general/configuration.rst +++ b/user_guide_src/source/general/configuration.rst @@ -47,9 +47,10 @@ All of the configuration files that ship with CodeIgniter are namespaced with ``Config``. Using this namespace in your application will provide the best performance since it knows exactly where to find the files. -.. note:: ``config()`` finds the file in **app/Config/** when there is a class with the same shortname, +.. note:: Prior to v4.4.0, ``config()`` finds the file in **app/Config/** when there + is a class with the same shortname, even if you specify a fully qualified class name like ``config(\Acme\Blog\Config\Blog::class)``. - This is because ``config()`` is a wrapper for the ``Factories`` class which uses ``preferApp`` by default. See :ref:`factories-loading-class` for more information. + This behavior has been fixed in v4.4.0, and returns the specified instance. Getting a Config Property ========================= diff --git a/user_guide_src/source/general/modules.rst b/user_guide_src/source/general/modules.rst index ae79f5431779..92093d5bd5c1 100644 --- a/user_guide_src/source/general/modules.rst +++ b/user_guide_src/source/general/modules.rst @@ -189,14 +189,15 @@ with the ``new`` command: .. literalinclude:: modules/008.php -Config files are automatically discovered whenever using the :php:func:`config()` function that is always available. +Config files are automatically discovered whenever using the :php:func:`config()` function that is always available, and you pass a short classname to it. .. note:: We don't recommend you use the same short classname in modules. - Modules that need to override or add to known configurations in **app/Config/** should use :ref:`registrars`. + Modules that need to override or add to known configurations in **app/Config/** should use :ref:`Implicit Registrars `. -.. note:: ``config()`` finds the file in **app/Config/** when there is a class with the same shortname, +.. note:: Prior to v4.4.0, ``config()`` finds the file in **app/Config/** when there + is a class with the same shortname, even if you specify a fully qualified class name like ``config(\Acme\Blog\Config\Blog::class)``. - This is because ``config()`` is a wrapper for the ``Factories`` class which uses ``preferApp`` by default. See :ref:`factories-loading-class` for more information. + This behavior has been fixed in v4.4.0, and returns the specified instance. Migrations ==========