feat: add Yii::$app->params type inference from configuration for precise array shape typing.#85
Conversation
…recise array shape typing.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds config-derived type inference for Yii::$app->params: ServiceMap extracts/validates a top-level Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration File
participant SM as ServiceMap
participant SFE as StubFilesExtension
participant PS as PHPStan
Config->>SM: load config (including `params`)
activate SM
SM->>SM: processDefinition(config)
SM->>SM: processParams(config) -> store $params
SM->>SM: processSingletons(config)
deactivate SM
SFE->>SM: getParams()
activate SFE
SM-->>SFE: return params array
SFE->>SFE: inferTypeString(params)
SFE->>SFE: buildParamsPropertyDeclaration()
SFE-->>PS: provide generated stub with `@var` annotation
deactivate SFE
activate PS
PS->>PS: analyze Yii::$app->params usages
PS-->>PS: infer precise array-shape types
deactivate PS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 81 109 +28
===========================================
Files 2 2
Lines 156 224 +68
===========================================
+ Hits 156 224 +68 ☔ View full report in Codecov by Sentry. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/StubFilesExtension.php`:
- Around line 253-314: The inferTypeString method is too complex; split it into
small private helpers: move the scalar checks (null/string/int/float/bool) into
a new private inferScalarType(mixed): ?string that returns the scalar type or
null, extract the key formatting/preg_match logic into formatArrayKey(string):
string, and extract the array-shape assembly into a private
inferArrayType(array): string which handles empty arrays, all-string-key maps
(using formatArrayKey and recursive inferTypeString calls) and non-string-key
arrays (collecting inferred value types). Then simplify inferTypeString(mixed
$value) to first call inferScalarType and return if non-null, otherwise delegate
arrays to inferArrayType and default to 'mixed'. Ensure to use the same method
names (inferTypeString, inferScalarType, formatArrayKey, inferArrayType) so
callers are easy to update.
- Around line 280-311: The array-shape fallback drops keys when some keys are
non-string; update the logic in the same method (the inferTypeString handling of
arrays where $value is iterated) to preserve keys for sparse/mixed-key arrays by
iterating over $value with foreach ($value as $k => $v) and formatting each key
(use the same key-formatting logic used for string keys: quote strings that need
quoting, render integers as their literal digits) and push entries like
$formattedKey . ': ' . $this->inferTypeString($v); then return 'array{' .
implode(', ', $entries) . '}'; so both string-only and mixed/sparse arrays
include explicit key => type pairs rather than value-only entries.
- Around line 295-299: The array-shape key quoting in StubFilesExtension.php
currently wraps non-identifier keys as '$k' without escaping, which can produce
invalid PHPDoc when keys contain single quotes or backslashes; modify the branch
that assigns $formattedKey (the ternary using preg_match and the else that
builds "'$k'") to escape backslashes then single quotes in $k before wrapping
(so keys like O'Reilly become 'O\'Reilly' and paths like C\path become
'C\\\\path'), keeping the rest of the entry construction ($entries[] =
$formattedKey . ': ' . $this->inferTypeString($v)) unchanged.
In `@tests/ServiceMapParamsTest.php`:
- Around line 67-80: Add a regression test to assert that a params => null
configuration is treated as invalid (same as non-array scalars): in
tests/ServiceMapParamsTest.php add a new test (or extend
testThrowExceptionWhenParamsNotArray) that loads a fixture like
params-unsupported-is-null.php and expects RuntimeException with the same
message "Configuration file '{...}' must contain a valid 'params' 'array'."
Reference the ServiceMap constructor to instantiate ServiceMap with the null
fixture and ensure the test mirrors the existing
expectException/expectExceptionMessage assertions so null is rejected,
preventing silent coercion.
In `@tests/web/data/property/ApplicationParamsType.php`:
- Around line 30-58: Add a focused test that directly asserts the dotted key
access path for the quoted key (e.g. assertType('string',
Yii::$app->params['turnstile.siteKey'])); locate an appropriate test method near
testReturnStringFromParamsKey or create a new method (e.g.
testReturnTurnstileSiteKeyFromParamsKey) and use assertType with the exact
dotted key to ensure regressions in quoted-key handling are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ff19fcc-1a32-46c5-9cc9-959ea615e894
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mdcomposer.jsonsrc/ServiceMap.phpsrc/StubFilesExtension.phptests/ServiceMapParamsTest.phptests/config/params-config.phptests/config/params-unsupported-is-not-array.phptests/config/phpstan-config.phptests/config/phpstan-params-config.phptests/support/extension-params-test.neontests/web/data/property/ApplicationParamsType.phptests/web/property/ApplicationParamsTypeTest.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: phpunit / PHP 8.1-windows-2022
- GitHub Check: phpunit / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.2-windows-2022
- GitHub Check: phpunit / PHP 8.1-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: phpunit-compatibility / PHP 8.5-windows-2022
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-2022
- GitHub Check: phpunit-compatibility / PHP 8.2-windows-2022
- GitHub Check: phpunit-compatibility / PHP 8.1-ubuntu-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-ubuntu-latest
- GitHub Check: phpunit-compatibility / PHP 8.4-windows-2022
- GitHub Check: phpunit-compatibility / PHP 8.4-ubuntu-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-2022
- GitHub Check: linter / Super Linter
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: phpunit / PHP 8.4-ubuntu-latest
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: terabytesoftw
Repo: yii2-extensions/phpstan PR: 40
File: src/ServiceMap.php:0-0
Timestamp: 2025-06-14T17:41:48.820Z
Learning: The yii2-extensions/phpstan repository contains a PHPStan extension specifically designed for static analysis of Yii framework applications.
Learnt from: terabytesoftw
Repo: yii2-extensions/phpstan PR: 84
File: CHANGELOG.md:12-12
Timestamp: 2026-04-03T00:19:36.123Z
Learning: In the `yii2-extensions/phpstan` repository, Conventional Commits-style CHANGELOG entries do not include issue/PR number references. The maintainer considers issue numbers non-mandatory per the Conventional Commits specification. Do not suggest adding issue/PR numbers to changelog entries as a consistency requirement.
📚 Learning: 2025-06-14T17:41:48.820Z
Learnt from: terabytesoftw
Repo: yii2-extensions/phpstan PR: 40
File: src/ServiceMap.php:0-0
Timestamp: 2025-06-14T17:41:48.820Z
Learning: The yii2-extensions/phpstan repository contains a PHPStan extension specifically designed for static analysis of Yii framework applications.
Applied to files:
tests/support/extension-params-test.neontests/config/phpstan-params-config.phpsrc/ServiceMap.phptests/web/property/ApplicationParamsTypeTest.phpREADME.mdtests/web/data/property/ApplicationParamsType.php
📚 Learning: 2025-10-10T12:54:50.152Z
Learnt from: terabytesoftw
Repo: yii2-extensions/phpstan PR: 77
File: .github/linters/actionlint.yml:5-7
Timestamp: 2025-10-10T12:54:50.152Z
Learning: In the yii2-extensions/phpstan repository, the maintainer prefers to keep the actionlint.yml configuration with the generic ignore entry `"section is alias node but mapping node is expected"` as it works correctly for their workflows, and previous suggestions to change it have failed in other repositories.
Applied to files:
tests/support/extension-params-test.neon
📚 Learning: 2026-04-03T00:19:36.123Z
Learnt from: terabytesoftw
Repo: yii2-extensions/phpstan PR: 84
File: CHANGELOG.md:12-12
Timestamp: 2026-04-03T00:19:36.123Z
Learning: In the `yii2-extensions/phpstan` repository, Conventional Commits-style CHANGELOG entries do not include issue/PR number references. The maintainer considers issue numbers non-mandatory per the Conventional Commits specification. Do not suggest adding issue/PR numbers to changelog entries as a consistency requirement.
Applied to files:
README.md
📚 Learning: 2025-06-12T16:41:20.240Z
Learnt from: terabytesoftw
Repo: yii2-extensions/phpstan PR: 36
File: src/type/ActiveQueryDynamicMethodReturnTypeExtension.php:153-157
Timestamp: 2025-06-12T16:41:20.240Z
Learning: In PHPStan extensions for this repository, `instanceof` must not be used to compare `PHPStan\Type\*` objects (e.g., `ThisType`) because it is not compatible with PHPStan’s runtime mode; instead, use `$type::class === SomeType::class` for reliable checks.
Applied to files:
tests/web/data/property/ApplicationParamsType.php
🪛 PHPMD (2.15.0)
src/StubFilesExtension.php
[warning] 253-314: The method inferTypeString() has a Cyclomatic Complexity of 14. The configured cyclomatic complexity threshold is 10. (undefined)
(CyclomaticComplexity)
[warning] 253-314: The method inferTypeString() has an NPath complexity of 1568. The configured NPath complexity threshold is 200. (undefined)
(NPathComplexity)
🔇 Additional comments (12)
tests/config/params-unsupported-is-not-array.php (1)
1-7: LGTM!This test fixture correctly provides an invalid configuration (string instead of array) to verify that the
ServiceMapproperly validates theparamstype and throws an appropriate exception.composer.json (1)
55-55: LGTM!Appropriate approach for running PHPUnit with unlimited memory. Unlike PHPStan which has a native
--memory-limitflag, PHPUnit requires setting the PHP directive directly.tests/config/phpstan-config.php (1)
20-32: LGTM!Good coverage of diverse param types (string, int, bool, float, null, nested array, string array) for comprehensive type inference testing. The test values align with the other params config fixtures in this PR.
CHANGELOG.md (1)
13-13: LGTM!The changelog entry follows the project's Conventional Commits style and accurately describes the new feature.
tests/support/extension-params-test.neon (1)
1-6: LGTM!Well-structured neon configuration that mirrors the existing
extension-test.neonpattern while pointing to a dedicated params config for isolated feature testing.README.md (2)
73-78: LGTM!Clear and practical example for the Quick start section, demonstrating the params configuration.
145-162: LGTM!Excellent documentation of the new params type inference feature with clear examples showing:
- Full params array shape typing
- Individual key access with proper scalar types
- Nested array support
The nested example (
nested.db.host) appropriately demonstrates a realistic database config use case.tests/config/phpstan-params-config.php (1)
1-33: LGTM!Well-structured test fixture that combines realistic component configuration with the params section needed for type inference testing. The params structure is consistent with other test fixtures in this PR.
tests/config/params-config.php (1)
1-22: LGTM!Focused test fixture providing comprehensive type coverage (string, int, bool, float, null, nested array, list array) for unit testing the
ServiceMap::getParams()extraction logic.src/ServiceMap.php (2)
546-561: Good extraction flow forparamsinto the service map.
processParams()cleanly captures configuration data for downstream stub generation and keeps default behavior stable whenparamsis absent.
331-333: Technical observation is correct, but appears to be intentional design pattern.Line 331 uses
isset($config['params']), which returns false when the value isnull, allowing config like['params' => null]to bypass validation and silently coerce to[]via the null-coalescing operator on line 559. However, this same validation pattern is used consistently across all config keys (behaviors,components,container.definitions,container.singletons), suggesting this is intentional design rather than a validation bug. The functional outcome—treating missing or null values as empty arrays—is sound and non-breaking.If stricter validation for explicit
nullvalues is desired, the proposed fix usingarray_key_exists()is correct. However, this would require changes across all similar config validations for consistency.tests/web/property/ApplicationParamsTypeTest.php (1)
24-42: Test harness wiring looks solid.Data-provider and additional config setup are clear and correctly scoped for params-type inference assertions.
1 participant
Pull Request