feat: Enhance DI container type inference and testing.
#37
Conversation
WalkthroughThis change refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant ServiceMap as ServiceMap
participant Config as Config File
Test->>ServiceMap: new ServiceMap(configPath)
ServiceMap->>Config: Load config
ServiceMap->>ServiceMap: Process services/components
Test->>ServiceMap: getServiceById(serviceId)
ServiceMap->>ServiceMap: Lookup service class/definition
ServiceMap-->>Test: Return class name or null
Test->>ServiceMap: getComponentDefinitionByClassName(className)
ServiceMap->>ServiceMap: Reverse lookup component ID/definition
ServiceMap-->>Test: Return definition or null
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
===========================================
Coverage 100.00% 100.00%
+ Complexity 60 54 -6
===========================================
Files 1 1
Lines 91 91
===========================================
Hits 91 91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php (2)
10-10: Same note onuse function dirname;– align with the convention decided in the previous file.
49-50: Config-file path duplication – see earlier remark.
🧹 Nitpick comments (18)
tests/extension-tests.neon (1)
5-6: Bootstrap file path is ambiguous
bootstrapFilesentries are resolved relative to the current NEON file, not to%rootDir%.
Iftests/bootstrap.phpis intended, make the path explicit to avoid surprises when the file is moved or the NEON file is relocated.- - bootstrap.php + - %rootDir%/tests/bootstrap.phpCHANGELOG.md (1)
5-8: Minor markdown formatting – missing back-tick and closing parenthesis- - Enh #37: Enhance `DI container type inference and testing (@terabytesoftw) + - Enh #37: Enhance `DI` container type inference and testing (@terabytesoftw)Correcting this prevents broken inline-code formatting in rendered docs.
composer.json (1)
10-11:minimum-stability: devwithoutprefer-stablemay pull unstable packagesAdding
"prefer-stable": truebalances flexibility with safety."minimum-stability": "dev", + "prefer-stable": true,tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php (2)
10-10: Import of globaldirname()is fine but inconsistent across codebaseIf you want to optimise for opcode caching, keep the
use functionimport everywhere; otherwise drop it for brevity.
Stick to one convention to avoid churn.
47-48: Hard-coded config path duplicated in several test classesConsider extracting the value to a constant or helper to avoid future drift across tests.
tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php (2)
11-12: Unnecessary import
dirname()is already in the global namespace; the explicituse function dirname;adds no value and will be flagged by most linters as an unused import.-use function dirname;
47-47: Hard-coded path separator may break on non-POSIX toolingUsing a literal
'/'inside the path string is fine on runtime PHP, but some static tools running on Windows treat it as a literal. For full portability you can rely onDIRECTORY_SEPARATORorsprintf:- return [dirname(__DIR__) . '/extension-tests.neon']; + return [dirname(__DIR__) . DIRECTORY_SEPARATOR . 'extension-tests.neon'];tests/type/ContainerDynamicMethodReturnTypeExtensionTest.php (2)
7-10: Remove unused import
yii\di\Containerisn’t referenced in this class. Keeping only the necessary imports keeps the test surface lean.-use yii\di\Container;
38-54: Duplicated helper logic across test classes
dataFileAsserts()/getAdditionalConfigFiles()blocks are identical in several classes. Consider extracting them into a shared trait to DRY the test suite.tests/ServiceMapComponentTest.php (2)
108-111: Assertion message typoThe message contains stray back-ticks and back-quote:
'ServiceMap should return \'' f`or a non-existent component class.'`- 'ServiceMap should return \'`\' f`or a non-existent component class.', + 'ServiceMap should return null for a non-existent component class.',
38-65: Reduce boilerplate withsetUp()Every test duplicates the
ServiceMapconstruction. Initialising it insetUp()(or using a data provider) will shorten the file and run marginally faster.tests/fixture/data/types/ContainerDynamicMethodReturnType.php (2)
70-78:random_int()call is unnecessaryThe fixture is analysed, not executed; however, some static analysers still warn on unused pseudo-random logic. A ternary on a literal
true/*false*/would express the intent without side noise.
189-196: Consider registering services to avoid runtime surprisesAlthough fixture files are parsed only by PHPStan, newcomers may attempt to run them. Registering the service IDs before calling
$container->get()would make the examples executable and self-documenting.src/ServiceMap.php (3)
285-293: Preferinstanceoffor Closure detection
is_object($definition) && $definition::class === Closure::classworks but is unnecessarily indirect;
$definition instanceof Closureis the idiomatic, slightly faster, and more readable PHP way.-if (is_object($definition) && $definition::class === Closure::class) { +if ($definition instanceof Closure) {
303-305: Avoid autoload side-effects inis_subclass_ofguardCalling
is_subclass_of($id, BaseObject::class)on an arbitrary service ID may trigger an unnecessary
autoload attempt for non-class strings (e.g."db").
Guard the check withclass_exists($id, /*autoload*/ false)first to prevent accidental autoloading
and the associated I/O cost.-if (is_subclass_of($id, BaseObject::class)) { +if (class_exists($id, false) && is_subclass_of($id, BaseObject::class)) {
258-279: Docblock / signature drift
normalizeDefinition()’s PHP type allowsint, yet the accompanying@phpstan-param DefinitionType
excludes it. Either dropintfrom the signature or extendDefinitionTypeto keep static analysis
in sync with runtime behaviour.tests/ServiceMapServiceTest.php (2)
330-353: Duplicate test block
testThrowExceptionWhenDefinitionNotArray()andtestThrowExceptionWhenDefinitionTypeInteger()use the
same fixture and expect the same exception message, providing no additional coverage. Consider
removing one or merging them to keep the suite lean.
39-52: Strengthen “no-assertion” smoke testsUsing
expectNotToPerformAssertions()verifies absence of exceptions, but asserting the created object
improves intent without extra cost:$serviceMap = new ServiceMap(); self::assertInstanceOf(ServiceMap::class, $serviceMap);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.md(1 hunks)composer.json(1 hunks)src/ServiceMap.php(6 hunks)src/type/ContainerDynamicMethodReturnTypeExtension.php(3 hunks)tests/ServiceMapComponentTest.php(1 hunks)tests/ServiceMapServiceTest.php(4 hunks)tests/extension-tests.neon(1 hunks)tests/fixture/data/types/ContainerDynamicMethodReturnType.php(1 hunks)tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php(2 hunks)tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php(2 hunks)tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php(2 hunks)tests/type/ContainerDynamicMethodReturnTypeExtensionTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/type/ContainerDynamicMethodReturnTypeExtension.php (2)
src/ServiceMap.php (2)
ServiceMap(59-479)getServiceById(201-204)src/reflection/ComponentPropertyReflection.php (1)
getType(145-148)
tests/type/ContainerDynamicMethodReturnTypeExtensionTest.php (3)
tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php (3)
DataProvider(50-54)dataFileAsserts(38-43)getAdditionalConfigFiles(45-48)tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php (3)
DataProvider(50-54)dataFileAsserts(38-43)getAdditionalConfigFiles(45-48)tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php (3)
DataProvider(52-56)dataFileAsserts(38-45)getAdditionalConfigFiles(47-50)
tests/fixture/data/types/ContainerDynamicMethodReturnType.php (1)
tests/stub/MyActiveRecord.php (1)
MyActiveRecord(29-54)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: phpunit-compatibility / PHP 8.4-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
🔇 Additional comments (1)
src/type/ContainerDynamicMethodReturnTypeExtension.php (1)
104-106: Early-return guard will hide service-map mistakesWhen
$serviceClassisnullyou immediately returnMixedType. Falling back to the method signature type (as you do for the no-arg branch) is often more useful to the user. Verify this change is intentional.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/ServiceMapServiceTest.php (2)
26-32: Docblock bullets lose their formattingThe “Key features” list isn’t recognised by most IDE / api-doc generators because the lines don’t start with
*.
Replacing the plain lines with proper*bullets keeps PhpDoc tooling happy.- * Key features. - * - Ensures compatibility with fixture-based configuration files. - * - Resolves service class by ID for valid and initialized services. - * - Retrieves service definitions by ID and class name. - * - Returns `null` for non-existent or non-class service IDs. - * - Throws exceptions for invalid service ID types and non-array service definitions. - * - Validates error handling for unsupported or malformed configuration files. + * Key features + * - Ensures compatibility with fixture-based configuration files. + * - Resolves service class by ID for valid and initialised services. + * - Retrieves service definitions by ID and class name. + * - Returns `null` for non-existent or non-class service IDs. + * - Throws exceptions for invalid service-ID types and non-array service definitions. + * - Validates error handling for unsupported or malformed configuration files.
68-173: High duplication – consider a data-providerFrom
testReturnNullWhenServiceNonExistent()throughtestReturnServiceClassWhenSingletonServiceValid()every method:
- Builds
$fixturePaththe same way.- Instantiates
ServiceMap.- Performs a single
assertSame.Consolidating into one parametrised test cuts ~150 lines and removes hidden drift risk.
Illustration:
/** * @dataProvider serviceCases */ public function testGetServiceById(string $serviceId, ?string $expected): void { $serviceMap = new ServiceMap($this->fixturePath()); self::assertSame($expected, $serviceMap->getServiceById($serviceId)); } public static function serviceCases(): array { return [ ['non-existent-service', null], ['closure', SplStack::class], ['nested-service-class', SplFileInfo::class], // … ]; } private function fixturePath(): string { return __DIR__ . DIRECTORY_SEPARATOR . 'fixture' . DIRECTORY_SEPARATOR . 'config' . DIRECTORY_SEPARATOR . 'config.php'; }Reduces maintenance noise and accelerates future additions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)composer.json(1 hunks)tests/ServiceMapComponentTest.php(1 hunks)tests/ServiceMapServiceTest.php(5 hunks)tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php(1 hunks)tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php(1 hunks)tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php(1 hunks)tests/type/ContainerDynamicMethodReturnTypeExtensionTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- CHANGELOG.md
- tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php
- tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php
- tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php
- composer.json
- tests/type/ContainerDynamicMethodReturnTypeExtensionTest.php
- tests/ServiceMapComponentTest.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/ServiceMapServiceTest.php (2)
src/ServiceMap.php (2)
ServiceMap(59-477)getServiceById(199-202)tests/stub/MyActiveRecord.php (1)
MyActiveRecord(29-54)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: phpunit / PHP 8.4-windows-latest
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: phpunit / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.4-windows-latest
- GitHub Check: phpunit / PHP 8.1-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-windows-latest
- GitHub Check: phpunit / PHP 8.1-windows-latest
- GitHub Check: phpunit / PHP 8.3-windows-latest
Summary by CodeRabbit
New Features
get()method, improving accuracy when retrieving services or classes.Bug Fixes
Documentation
Refactor
Tests