From 6fbdcae142acbcd2439ef558074cf56a5c595d7f Mon Sep 17 00:00:00 2001 From: Alexandre Gomes Gaigalas Date: Sat, 21 Mar 2026 01:22:36 -0300 Subject: [PATCH] Fix bugs, remove dead code, and refactor Config internals - Fix empty("0") treating "0" as null in IniLoader::parseValue - Fix pre-existing bug: matchConstructorParam no longer pollutes constructor array with null placeholders, allowing params with non-nullable defaults (e.g. EntityFactory::$style) to work - Add numeric coercion in parseConstants for strict_types compat - Add declare(strict_types=1) to Instantiator.php - Guard Autowire::cleanupParams to only autowire constructor params, not method-call arguments - Remove Container::__invoke and __call (unused by consumers) - Extract shouldCache() to replace $forceNew param in getInstance; Factory overrides to return false instead of wrapping parent call - Extract stripTrailingNulls helper, deduplicating Autowire/Instantiator - Refactor keyHasStateInstance to existingKeyFrom (no side-effect) - Cache plain callables consistently in lazyLoad - Remove func_get_args(), stripos, and dead imports - Use parent::offsetGet in has() for consistency --- src/Autowire.php | 45 +++++++-------- src/Container.php | 85 +++++----------------------- src/Factory.php | 6 +- src/IniLoader.php | 53 +++++++++++------- src/Instantiator.php | 119 ++++++++++++++++++++++++---------------- tests/AutowireTest.php | 41 ++++++++++++++ tests/ContainerTest.php | 82 ++++++--------------------- tests/FactoryTest.php | 10 ---- tests/IniLoaderTest.php | 34 ++++++++++++ 9 files changed, 236 insertions(+), 239 deletions(-) diff --git a/src/Autowire.php b/src/Autowire.php index 079f82a..75da9f1 100644 --- a/src/Autowire.php +++ b/src/Autowire.php @@ -8,8 +8,6 @@ use ReflectionNamedType; use function array_key_exists; -use function end; -use function key; class Autowire extends Instantiator { @@ -21,41 +19,44 @@ public function setContainer(ContainerInterface $container): void } /** @inheritDoc */ - protected function cleanupParams(array $params): array + protected function cleanupParams(array $params, bool $forConstructor = true): array { $constructor = $this->reflection()->getConstructor(); - if ($constructor && $this->container) { + $container = $this->container; + if ($forConstructor && $constructor && $container) { foreach ($constructor->getParameters() as $param) { $name = $param->getName(); if (array_key_exists($name, $this->params)) { $value = $params[$name] ?? null; if ($value instanceof Ref) { - $params[$name] = $this->container->get($value->id); + $params[$name] = $container->get($value->id); } else { + $this->propagateContainer($value); $params[$name] = $this->lazyLoad($value); } - - continue; - } - - $type = $param->getType(); - if ( - !($type instanceof ReflectionNamedType) || $type->isBuiltin() - || !$this->container->has($type->getName()) - ) { - continue; + } else { + $type = $param->getType(); + if ( + $type instanceof ReflectionNamedType && !$type->isBuiltin() + && $container->has($type->getName()) + ) { + $params[$name] = $container->get($type->getName()); + } } - - $params[$name] = $this->container->get($type->getName()); - } - - while (end($params) === null && ($key = key($params)) !== null) { - unset($params[$key]); } - return $params; + return $this->stripTrailingNulls($params); } return parent::cleanupParams($params); } + + protected function propagateContainer(mixed $value): void + { + if (!$value instanceof self || $value->container !== null || $this->container === null) { + return; + } + + $value->setContainer($this->container); + } } diff --git a/src/Container.php b/src/Container.php index 13fd4b3..ff161b6 100644 --- a/src/Container.php +++ b/src/Container.php @@ -7,20 +7,11 @@ use ArrayObject; use Closure; use Psr\Container\ContainerInterface; -use ReflectionClass; use ReflectionException; -use ReflectionFunction; -use ReflectionNamedType; -use function array_filter; -use function array_map; -use function assert; use function call_user_func; use function class_exists; -use function func_get_args; -use function is_array; use function is_callable; -use function is_string; /** @extends ArrayObject */ class Container extends ArrayObject implements ContainerInterface @@ -39,7 +30,7 @@ public function has(string $id): bool return false; } - $entry = $this[$id]; + $entry = parent::offsetGet($id); if ($entry instanceof Instantiator) { return class_exists($entry->getClassName()); } @@ -49,12 +40,13 @@ public function has(string $id): bool public function getItem(string $name, bool $raw = false): mixed { - if (!isset($this[$name])) { + if (!parent::offsetExists($name)) { throw new NotFoundException('Item ' . $name . ' not found'); } - if ($raw || !is_callable($this[$name])) { - return $this[$name]; + $value = $this[$name]; + if ($raw || !is_callable($value)) { + return $value; } try { @@ -86,15 +78,20 @@ public function set(string $name, mixed $value): void protected function lazyLoad(string $name): mixed { $callback = $this[$name]; - if ($callback instanceof Instantiator && !$callback instanceof Factory) { - return $this[$name] = $callback(); + if ($callback instanceof Instantiator) { + $result = $callback(); + if ($callback->shouldCache()) { + $this[$name] = $result; + } + + return $result; } if ($callback instanceof Closure) { return $this[$name] = $callback($this); } - return call_user_func($callback); + return $this[$name] = call_user_func($callback); } public function __isset(string $name): bool @@ -102,60 +99,6 @@ public function __isset(string $name): bool return $this->has($name); } - public function __invoke(mixed $spec): mixed - { - if (is_callable($spec)) { - if (is_array($spec)) { - [$class, $method] = $spec; - $class = new ReflectionClass($class); - $object = $class->newInstance(); - $mirror = $class->getMethod($method); - } else { - $object = false; - assert($spec instanceof Closure || is_string($spec)); - $mirror = new ReflectionFunction($spec); - } - - $container = $this; - $arguments = array_map( - static function ($param) use ($container) { - $paramClass = $param->getType(); - if ($paramClass instanceof ReflectionNamedType) { - return $container->getItem($paramClass->getName()); - } - - return null; - }, - $mirror->getParameters(), - ); - if ($object) { - return $mirror->invokeArgs($object, $arguments); - } - - return $mirror->invokeArgs($arguments); - } - - if ((bool) array_filter(func_get_args(), 'is_object')) { - foreach (func_get_args() as $dependency) { - $this[$dependency::class] = $dependency; - } - } - - foreach ($spec as $name => $item) { - parent::offsetSet($name, $item); - } - - return $this; - } - - /** @param array $dict */ - public function __call(string $name, array $dict): mixed - { - $this->__invoke($dict[0]); - - return $this->getItem($name); - } - public function __get(string $name): mixed { return $this->getItem($name); @@ -164,6 +107,8 @@ public function __get(string $name): mixed public function __set(string $name, mixed $value): void { if (isset($this[$name]) && $this[$name] instanceof Instantiator) { + // Update the Instantiator's cached instance so other Instantiators + // holding a reference to it resolve to the new value $this[$name]->setInstance($value); } diff --git a/src/Factory.php b/src/Factory.php index e5561f3..ca296d2 100644 --- a/src/Factory.php +++ b/src/Factory.php @@ -6,10 +6,8 @@ class Factory extends Instantiator { - public function getInstance(bool $forceNew = false): mixed + public function shouldCache(): bool { - $this->instance = null; - - return parent::getInstance(true); + return false; } } diff --git a/src/IniLoader.php b/src/IniLoader.php index facd7fe..3b9dade 100644 --- a/src/IniLoader.php +++ b/src/IniLoader.php @@ -7,15 +7,13 @@ use Closure; use InvalidArgumentException; -use function array_filter; use function constant; use function count; -use function current; use function defined; use function explode; use function file_exists; use function is_array; -use function is_object; +use function is_numeric; use function is_string; use function parse_ini_file; use function parse_ini_string; @@ -23,6 +21,7 @@ use function preg_replace; use function preg_replace_callback; use function str_contains; +use function strstr; use function trim; class IniLoader @@ -82,14 +81,24 @@ public function fromFile(string $configurator): Container /** @param array $configurator */ public function fromArray(array $configurator): Container { - foreach ($this->state() + $configurator as $key => $value) { + foreach ($configurator as $key => $value) { + $stringKey = (string) $key; + + // State takes precedence: use existing non-Instantiator value instead + if ( + $this->container->offsetExists($stringKey) + && !$this->container[$stringKey] instanceof Instantiator + ) { + $value = $this->container[$stringKey]; + } + if ($value instanceof Closure) { - $this->container->offsetSet((string) $key, $value); + $this->container->offsetSet($stringKey, $value); continue; } if ($value instanceof Instantiator) { - $this->container->offsetSet((string) $key, $value); + $this->container->offsetSet($stringKey, $value); continue; } @@ -99,18 +108,11 @@ public function fromArray(array $configurator): Container return $this->container; } - /** @return array */ - protected function state(): array + protected function existingKeyFrom(string $key): string|false { - return array_filter( - $this->container->getArrayCopy(), - static fn($v): bool => !is_object($v) || !$v instanceof Instantiator, - ); - } + $k = strstr($key, ' ', true) ?: $key; - protected function keyHasStateInstance(string $key, mixed &$k): bool - { - return $this->container->offsetExists($k = current(explode(' ', $key))); + return $this->container->offsetExists($k) ? $k : false; } protected function keyHasInstantiator(string $key): bool @@ -122,8 +124,9 @@ protected function parseItem(string|int $key, mixed $value): void { $key = trim((string) $key); if ($this->keyHasInstantiator($key)) { - if ($this->keyHasStateInstance($key, $k)) { - $this->container->offsetSet($key, $this->container[$k]); + $existingKey = $this->existingKeyFrom($key); + if ($existingKey !== false) { + $this->container->offsetSet($key, $this->container[$existingKey]); } else { $this->parseInstantiator($key, $value); } @@ -146,7 +149,7 @@ protected function parseSubValues(array &$value): array return $value; } - protected function parseStandardItem(string $key, mixed &$value): void + protected function parseStandardItem(string $key, mixed $value): void { if (is_array($value)) { $this->parseSubValues($value); @@ -159,6 +162,10 @@ protected function parseStandardItem(string $key, mixed &$value): void protected function removeDuplicatedSpaces(string $string): string { + if (!str_contains($string, ' ')) { + return $string; + } + return (string) preg_replace('/\s+/', ' ', $string); } @@ -211,7 +218,7 @@ protected function parseValue(mixed $value): mixed return $this->parseSubValues($value); } - if (empty($value)) { + if ($value === '' || $value === null) { return null; } @@ -243,6 +250,12 @@ protected function parseConstants(string $value): mixed return constant($value); } + if (is_numeric($value)) { + $isFloat = str_contains($value, '.') || str_contains($value, 'e') || str_contains($value, 'E'); + + return $isFloat ? (float) $value : (int) $value; + } + return $value; } diff --git a/src/Instantiator.php b/src/Instantiator.php index 9c29627..4537d29 100644 --- a/src/Instantiator.php +++ b/src/Instantiator.php @@ -1,22 +1,23 @@ |null */ protected array|null $constructor = null; + /** @var list|null */ + private array|null $constructorParamNames = null; + /** @var array */ protected array $params = []; @@ -56,9 +60,14 @@ public function getClassName(): string return $this->className; } - public function getInstance(bool $forceNew = false): mixed + public function shouldCache(): bool + { + return true; + } + + public function getInstance(): mixed { - if ($this->instance && !$forceNew) { + if ($this->shouldCache() && $this->instance) { return $this->instance; } @@ -79,7 +88,7 @@ static function (mixed $result) use ($className, &$instance, $staticMethods): vo ); } - if (empty($instance)) { + if ($instance === null) { $constructorParams = $this->cleanupParams($this->constructor ?? []); if (empty($constructorParams)) { $instance = $this->reflection()->newInstance(); @@ -96,7 +105,11 @@ static function (mixed $result) use ($className, &$instance, $staticMethods): vo $this->performMethodCalls($instance, $methodCalls); } - return $this->instance = $instance; + if ($this->shouldCache()) { + $this->instance = $instance; + } + + return $instance; } public function getParam(string $name): mixed @@ -111,14 +124,12 @@ public function setInstance(mixed $instance): void public function setParam(string $name, mixed $value): void { - $value = $this->processValue($value); - - if ($this->matchStaticMethod($name)) { - $this->staticMethodCalls[] = [$name, $value]; + if ($this->matchFullConstructor($name)) { + $this->constructor = is_array($value) ? $value : []; } elseif ($this->matchConstructorParam($name)) { $this->constructor[$name] = $value; - } elseif ($this->matchFullConstructor($name)) { - $this->constructor = is_array($value) ? $value : []; + } elseif ($this->matchStaticMethod($name)) { + $this->staticMethodCalls[] = [$name, $value]; } elseif ($this->matchMethod($name)) { $this->methodCalls[] = [$name, $value]; } else { @@ -145,17 +156,35 @@ protected function reflection(): ReflectionClass * * @return array */ - protected function cleanupParams(array $params): array + protected function cleanupParams(array $params, bool $forConstructor = true): array { - while (end($params) === null && ($key = key($params)) !== null) { - unset($params[$key]); + $result = []; + foreach ($params as $key => $value) { + $result[$key] = $this->lazyLoad($value); } - foreach ($params as &$p) { - $p = $this->lazyLoad($p); + return $this->stripTrailingNulls($result); + } + + /** + * @param array $params + * + * @return array + */ + protected function stripTrailingNulls(array $params): array + { + $lastNonNull = -1; + $i = 0; + + foreach ($params as $value) { + if ($value !== null) { + $lastNonNull = $i; + } + + ++$i; } - return $params; + return array_slice($params, 0, $lastNonNull + 1, true); } protected function lazyLoad(mixed $value): mixed @@ -167,36 +196,26 @@ protected function lazyLoad(mixed $value): mixed return $value instanceof self ? $value->getInstance() : $value; } - protected function processValue(mixed $value): mixed - { - if (is_array($value)) { - foreach ($value as $valueKey => $subValue) { - $value[$valueKey] = $this->processValue($subValue); - } - } - - return $value; - } - protected function matchConstructorParam(string $name): bool { - if ($this->constructor === null) { - $this->constructor = []; + if ($this->constructorParamNames === null) { + $this->constructorParamNames = []; + $this->constructor ??= []; $ctor = $this->reflection()->getConstructor(); if ($ctor) { foreach ($ctor->getParameters() as $param) { - $this->constructor[$param->getName()] = null; + $this->constructorParamNames[] = $param->getName(); } } } - return array_key_exists($name, $this->constructor); + return in_array($name, $this->constructorParamNames, true); } protected function matchFullConstructor(string $name): bool { return $name === '__construct' - || ($name === $this->className && stripos($this->className, '\\') !== false); + || ($name === $this->className && str_contains($this->className, '\\')); } protected function matchMethod(string $name): bool @@ -206,8 +225,11 @@ protected function matchMethod(string $name): bool protected function matchStaticMethod(string $name): bool { - return $this->reflection()->hasMethod($name) - && $this->reflection()->getMethod($name)->isStatic(); + try { + return $this->reflection()->getMethod($name)->isStatic(); + } catch (ReflectionException) { + return false; + } } /** @param array{string, mixed} $methodCalls */ @@ -217,28 +239,29 @@ protected function performMethodCalls( callable|null $resultCallback = null, ): void { [$methodName, $calls] = $methodCalls; - $resultCallback ??= static function (): void { - }; $callable = [$class, $methodName]; assert(is_callable($callable)); foreach ($calls as $arguments) { if (is_array($arguments)) { - $resultCallback(call_user_func_array( - $callable, - $this->cleanupParams($arguments), - )); + $result = call_user_func_array($callable, $this->cleanupParams($arguments, false)); } elseif ($arguments !== null) { - $resultCallback(call_user_func($callable, $this->lazyLoad($arguments))); + $result = call_user_func($callable, $this->lazyLoad($arguments)); } else { - $resultCallback(call_user_func($callable)); + $result = call_user_func($callable); + } + + if ($resultCallback === null) { + continue; } + + $resultCallback($result); } } public function __invoke(): mixed { - return $this->getInstance(...func_get_args()); + return $this->getInstance(); } } diff --git a/tests/AutowireTest.php b/tests/AutowireTest.php index ecb5048..4782942 100644 --- a/tests/AutowireTest.php +++ b/tests/AutowireTest.php @@ -297,6 +297,40 @@ public function testRefResolvesArrayDependency(): void $this->assertEquals(['/path/one', '/path/two'], $instance->paths); } + public function testNestedAutowireReceivesContainer(): void + { + $container = new Container(); + $dep = new AutowireDependency('shared'); + $container[AutowireDependency::class] = $dep; + + $autowire = new Autowire(AutowireWrapper::class, [ + 'inner' => new Autowire(AutowireTypedConsumer::class), + ]); + $autowire->setContainer($container); + + $instance = $autowire->getInstance(); + $this->assertInstanceOf(AutowireTypedConsumer::class, $instance->inner); + $this->assertSame($dep, $instance->inner->dep); + } + + public function testDeeplyNestedAutowirePropagatesContainer(): void + { + $container = new Container(); + $dep = new AutowireDependency('deep'); + $container[AutowireDependency::class] = $dep; + + $autowire = new Autowire(AutowireWrapper::class, [ + 'inner' => new Autowire(AutowireWrapper::class, [ + 'inner' => new Autowire(AutowireTypedConsumer::class), + ]), + ]); + $autowire->setContainer($container); + + $instance = $autowire->getInstance(); + $this->assertInstanceOf(AutowireWrapper::class, $instance->inner); + $this->assertSame($dep, $instance->inner->inner->dep); + } + /** @return array */ private static function parseIni(string $ini): array { @@ -363,3 +397,10 @@ public function __construct(public array $paths) { } } + +class AutowireWrapper +{ + public function __construct(public mixed $inner) + { + } +} diff --git a/tests/ContainerTest.php b/tests/ContainerTest.php index ef875a8..0bd0ff2 100644 --- a/tests/ContainerTest.php +++ b/tests/ContainerTest.php @@ -440,49 +440,6 @@ public function testDependenciesDoesNotAffectFactories(): void $this->assertSame($result, $result2); } - public function testByInstanceCallback(): void - { - $ini = <<<'INI' -[instanceof DateTime] -datetime = now -INI; - $c = IniLoader::load(self::parseIni($ini)); - $called = false; - $result = $c(static function (DateTime $date) use (&$called) { - $called = true; - - return $date; - }); - $result2 = $c['DateTime']; - $this->assertInstanceOf(DateTime::class, $result); - $this->assertInstanceOf(DateTime::class, $result2); - $this->assertTrue($called); - } - - public function testByInstanceCallback2(): void - { - $c = new Container(); - $c(new DateTime()); - $called = false; - $result = $c(static function (DateTime $date) use (&$called) { - $called = true; - - return $date; - }); - $result2 = $c['DateTime']; - $this->assertInstanceOf(DateTime::class, $result); - $this->assertInstanceOf(DateTime::class, $result2); - $this->assertTrue($called); - } - - public function testByMethodCallback(): void - { - $c = new Container(); - $c(new DateTime()); - $result = $c([__NAMESPACE__ . '\\Foo', 'hey']); - $this->assertInstanceOf(DateTime::class, $result); - } - public function testClassConstants(): void { $ini = <<<'INI' @@ -549,13 +506,6 @@ public function testMagicGet(): void $this->assertEquals('bar', $c->__get('foo')); } - public function testMagicCall(): void - { - $c = new Container(['bar' => 'Hello']); - $result = $c->__call('bar', [['extra' => 'val']]); - $this->assertEquals('Hello', $result); - } - public function testLoadString(): void { $ini = <<<'INI' @@ -709,17 +659,6 @@ public function testOffsetSetWithAutowire(): void $this->assertInstanceOf(stdClass::class, $result); } - public function testInvokeCallbackWithUntypedParam(): void - { - $c = new Container(); - $c(new DateTime()); - $result = $c(static function ($untyped, DateTime $date) { - return [$untyped, $date]; - }); - $this->assertNull($result[0]); - $this->assertInstanceOf(DateTime::class, $result[1]); - } - public function testParseValuePassesInstantiatorThrough(): void { $inner = new Instantiator('stdClass'); @@ -746,12 +685,25 @@ public function testAutowireModifierInContainerCreatesAutowire(): void $this->assertInstanceOf(Autowire::class, $raw); } - public function testInvokeWithArrayInjectsValues(): void + public function testPlainCallableIsCached(): void { + $count = 0; + $callable = new class ($count) { + public function __construct(private int &$count) + { + } + + public function __invoke(): int + { + return ++$this->count; + } + }; $c = new Container(); - $result = $c(['foo' => 'bar']); - $this->assertSame($c, $result); - $this->assertEquals('bar', $c->getItem('foo')); + $c['counter'] = [$callable, '__invoke']; + $first = $c->getItem('counter'); + $second = $c->getItem('counter'); + $this->assertSame(1, $first); + $this->assertSame($first, $second); } protected function tearDown(): void diff --git a/tests/FactoryTest.php b/tests/FactoryTest.php index 8eb6500..7af2ee6 100644 --- a/tests/FactoryTest.php +++ b/tests/FactoryTest.php @@ -24,16 +24,6 @@ public function testAlwaysCreatesNewInstance(): void $this->assertNotSame($first, $second); } - public function testForceNewIsIgnoredAlwaysNew(): void - { - $factory = new Factory(stdClass::class); - - $first = $factory->getInstance(false); - $second = $factory->getInstance(false); - - $this->assertNotSame($first, $second); - } - public function testFactoryWithConstructorParams(): void { $factory = new Factory('DateTime'); diff --git a/tests/IniLoaderTest.php b/tests/IniLoaderTest.php index 65734b6..c5f626c 100644 --- a/tests/IniLoaderTest.php +++ b/tests/IniLoaderTest.php @@ -369,6 +369,40 @@ public function testClassConstantResolution(): void $this->assertEquals(IniLoaderTestConstant::VALUE, $container->getItem('foo')); } + public function testNumericCoercionInteger(): void + { + $c = IniLoader::load(self::parseIni('val = 42')); + $this->assertSame(42, $c->getItem('val')); + } + + public function testNumericCoercionZero(): void + { + $c = IniLoader::load(self::parseIni('val = 0')); + $this->assertSame(0, $c->getItem('val')); + } + + public function testNumericCoercionFloat(): void + { + $c = IniLoader::load(self::parseIni('val = 3.14')); + $this->assertSame(3.14, $c->getItem('val')); + } + + public function testNumericCoercionScientific(): void + { + $c = IniLoader::load(self::parseIni('val = 1e3')); + $this->assertSame(1000.0, $c->getItem('val')); + } + + public function testNumericCoercionNegative(): void + { + $ini = <<<'INI' +[obj DateTime] +setTimestamp[] = -1 +INI; + $c = IniLoader::load(self::parseIni($ini)); + $this->assertSame(-1, $c->getItem('obj')->getTimestamp()); + } + /** @return array */ private static function parseIni(string $ini): array {