Update PHP versions in CI workflow and update QA tools latest #114
Conversation
Add PHP 8.3 to old stable versions and set current stable to 8.4. This ensures the CI process tests against the most recent PHP versions for better compatibility and bug detection.
WalkthroughThe pull request introduces several modifications across various files, primarily focusing on enhancing type safety, correcting typographical errors, and updating configuration parameters. Key changes include the addition of PHP version "8.3" to the continuous integration workflow, updates to type annotations in multiple classes to use a new Changes
Poem
Warning Rate limit exceeded@koriym has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 17 seconds before requesting another review. ⌛ 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/continuous-integration.yml
14-14: could not parse as YAML: yaml: line 14: could not find expected ':'
(syntax-check)
🔇 Additional comments (2)
.github/workflows/continuous-integration.yml (2)
Line range hint 8-11: LGTM on workflow structure
The workflow is well-structured and follows good practices by using reusable workflows.
🧰 Tools
🪛 actionlint (1.7.4)
14-14: could not parse as YAML: yaml: line 14: could not find expected ':'
(syntax-check)
12-13:
Caution: PHP 8.4 is still in development
While moving PHP 8.3 to old_stable is correct, setting PHP 8.4 as current_stable might be premature as it's still in development/alpha stage and not recommended for production use. This could lead to CI failures.
Let's verify the PHP 8.4 status:
Consider keeping PHP 8.3 as current_stable until PHP 8.4 reaches stable release status.
✅ Verification successful
Based on the output, I can now generate the final response:
PHP 8.4 is now stable and suitable for current_stable
The change to set PHP 8.4 as current_stable is appropriate as PHP 8.4.1 has been officially released, indicating it has reached stable status. Moving PHP 8.3 to old_stable is also correct as per the version progression.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check PHP 8.4 release status and compatibility
curl -s https://www.php.net/releases/ | grep -A 2 "PHP 8.4"
Length of output: 385
Deleted unnecessary 'psalm.compiler.xml' configuration file to streamline the codebase. Modified 'phpstan.neon' to adjust error levels and ignore duplicate type aliases. Updated 'phpunit.xml.dist' and 'psalm.xml' schema locations to use more accurate paths.
Upgraded several packages to their latest versions, including `composer/pcre`, `phpstan/phpstan`, and `symfony/*`. This change ensures compatibility with newer versions of PHP and other dependencies.
Removed an obsolete import that was no longer used in ProdInjectorContext. Added a check to ensure the ApcuCache class exists before attempting to instantiate it, with a runtime exception for cases where the class is missing.
Added the @psalm-import-type ScriptDir annotation and updated the type hints across multiple classes and methods for improved type safety. This involved specifying ScriptDir in parameter and property docblocks, as well as typedefs in several files. Additionally, unnecessary assert statements were removed to clean up the test cases.
Added assertions to check if FakeInvalidDependency class exists and if the instance implemented DependencyInterface. This ensures the test cannot silently fail due to a non-existing class or incorrect type.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (25)
vendor-bin/tools/composer.json (1)
6-6: Review PHPStan 2.0 breaking changesSince this is a major version upgrade, please ensure all breaking changes from PHPStan 2.0 have been addressed in the codebase.
Consider:
- Reviewing the PHPStan 2.0 changelog
- Testing the upgrade in a development environment first
- Updating any custom PHPStan rules or extensions if they exist
tests/Fake/FakeInvalidDependency.php (2)
15-15: LGTM! Consider adding PHPDoc for clarity.Since this is a test fake class, it would be helpful to document the purpose of unsetting the container.
public function inject(Container $container) { + // Intentionally unset container to simulate invalid dependency behavior unset($container); }
20-20: LGTM! Consider adding PHPDoc for clarity.Similar to the inject method, documenting the intention would improve readability.
public function register(array &$container, Bind $bind) { + // Intentionally unset parameters to simulate invalid registration unset($container, $bind); }src/ScriptinjectorModule.php (1)
Line range hint
27-31: Consider adding runtime validation for ScriptDirThe
configuremethod creates aScriptInjectorinstance using$scriptDirwithout validation. IfScriptDirhas specific requirements (e.g., existing directory, permissions), consider adding runtime validation in the constructor.Example validation:
public function __construct(string $scriptDir, ?AbstractModule $module = null) { + if (!is_dir($scriptDir) || !is_writable($scriptDir)) { + throw new \InvalidArgumentException('Script directory must be a writable directory'); + } $this->scriptDir = $scriptDir; parent::__construct($module); }🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 13-13:
Class name doesn't match filename; expected "class ScriptinjectorModule"src/AbstractInjectorContext.php (1)
16-20: Consider making the property protectedWhile the
@readonlyannotation helps with immutability, having a public property might expose internal implementation details. Consider making it protected if it doesn't need to be accessed directly from outside the class hierarchy.- public $tmpDir; + protected $tmpDir;docs/exmaple/ProdInjectorContext.php (1)
Line range hint
1-30: Consider moving example code to a dedicated examples directoryThe file is currently in the
docsdirectory. Consider moving example code to a dedicatedexamplesdirectory at the root level, which is a more common practice.src/InjectorFactory.php (1)
Line range hint
1-64: Consider consistent type usage across the codebaseWhile the introduction of the ScriptDir type is a positive step towards improved type safety, the inconsistent usage between docblocks and parameter types could lead to confusion and potential runtime issues. Consider:
- Consistently using ScriptDir type in both docblocks and parameter types
- Adding type validation if string values need to be converted to ScriptDir
- Documenting the ScriptDir type requirements and validation rules
tests/CachedFactoryTest.php (1)
39-39: Consider using an enum for better type safety.There's a type mismatch between the PHP type hint (
string) and the Psalm type alias (Context). Since the context values are fixed ('dev'|'prod'), this would be a perfect use case for a PHP enum.Here's a suggested implementation:
- /** - * @psalm-type Context = 'dev'|'prod' - */ + enum Context: string { + case DEV = 'dev'; + case PROD = 'prod'; + } - private function getInjector(string $context): InjectorInterface + private function getInjector(Context $context): InjectorInterface { - if ($context === 'dev') { + if ($context === Context::DEV) {This would provide:
- True type safety at runtime
- Better IDE support
- Elimination of magic strings
composer.json (3)
56-56: Consider documenting the removal of psalm.compiler.xmlThe removal of the
-c psalm.compiler.xmlconfiguration file parameter should be documented to explain why the default configuration is now preferred.
Line range hint
14-14: Update PHP version constraint for PHP 8.3 and 8.4 compatibilityGiven that the PR aims to support PHP 8.3 and 8.4, consider updating the PHP version constraint to explicitly indicate this support.
- "php": "^7.2 || ^8.0", + "php": "^7.2 || ^8.0 || ^8.3 || ^8.4",
Based on the package compatibility information gathered, I can now provide a final response:
Several dependencies need updates to ensure PHP 8.3/8.4 compatibility
Most dependencies only explicitly support up to PHP 8.0, which may cause compatibility issues with PHP 8.3 and 8.4:
doctrine/annotations: supports "^7.2 || ^8.0"doctrine/cache: supports "~7.1 || ^8.0"koriym/attributes: supports "^7.2 || ^8.0"koriym/null-object: supports "^7.2 || ^8.0"koriym/param-reader: supports "^7.2 || ^8.0"koriym/printo: supports ">=5.4.0" (very outdated)ray/aop: supports "^7.2 || ^8.0"ray/di: supports "^7.2 || ^8.0"Consider:
- Upgrading to newer versions of these packages if available
- Opening issues with the package maintainers to request PHP 8.3/8.4 compatibility
- Testing thoroughly with PHP 8.3/8.4 as some packages might work despite not explicitly declaring support
🔗 Analysis chain
Line range hint
14-24: Verify dependency compatibility with PHP 8.3 and 8.4Let's ensure all dependencies are compatible with PHP 8.3 and 8.4.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check compatibility of dependencies with PHP 8.3 and 8.4 # Extract package names and versions from composer.json echo "Checking package compatibility..." jq -r '.require + ."require-dev" | to_entries[] | select(.key != "php" and .key != "ext-pdo") | "\(.key):\(.value)"' composer.json | while read -r package; do name="${package%:*}" version="${package#*:}" # Query package info from Packagist echo "Checking $name..." curl -s "https://repo.packagist.org/p2/$name.json" | \ jq -r --arg ver "$version" '.packages[][] | select(.version | contains("dev") | not) | select(.version | startswith("v") | not) | .require.php // empty' doneLength of output: 5956
tests/ContextInjectorTest.php (1)
54-61: Consider adding PHPDoc to document test behavior.The test method verifies complex container reset behavior. Adding PHPDoc would improve maintainability by clearly documenting:
- The purpose of state modification
- Expected behavior between injector instances
- The significance of the
changedflagAdd documentation like:
+ /** + * Tests that container state: + * 1. Persists within the same injector instance + * 2. Resets when retrieving a new injector instance + * + * @param AbstractInjectorContext $context The injection context to test + */ public function testContainerIsResetWhenTheInjectorIsRetrieved(AbstractInjectorContext $context): voidsrc/OnDemandCompiler.php (1)
98-102: Consider modernizing error handling approachThe current approach of suppressing E_NOTICE errors during unserialization could be replaced with a more robust error handling pattern, such as using try-catch with UnserializeFailedException in PHP 8.x.
Would you like me to propose a more modern error handling implementation?
src/CachedInjectorFactory.php (2)
Line range hint
64-67: Update remaining method signatures to use ScriptDir typeFor consistency with the type safety improvements, update the remaining method signatures to use ScriptDir instead of string.
Apply these changes:
public static function getOverrideInstance( - string $scriptDir, + ScriptDir $scriptDir, callable $modules, AbstractModule $overrideModule, array $savedSingletons = [] ): InjectorInterface - private static function getInjector(callable $modules, string $scriptDir, array $savedSingletons, ?AbstractModule $module = null): InjectorInterface + private static function getInjector(callable $modules, ScriptDir $scriptDir, array $savedSingletons, ?AbstractModule $module = null): InjectorInterfaceAlso applies to: 77-77
61-61: Fix documentation alignmentThe parameter documentation has incorrect spacing/alignment.
Apply this change:
- * @param ScriptDir $scriptDir + * @param ScriptDir $scriptDirsrc/InjectionPoint.php (1)
34-36: Complete the constructor documentationThe docblock should document all parameters for better code clarity.
Add documentation for the
$parameterparameter:/** + * @param ReflectionParameter $parameter * @param ScriptDir $scriptDir */tests/AssistedTest.php (1)
Line range hint
83-90: Consider consolidating similar test methods.While the method name correction from
testAssistedCustomeInjecttotestAssistedCustomInjectis good, this test method appears to be very similar totestAssistedMethodInvocation. Consider either:
- Consolidating these tests if they're testing the same functionality
- Clarifying the distinct test scenarios by adding comments or adjusting assertions
+ /** + * @test + * Tests the custom injection behavior specifically focusing on the user ID assignment + * This differs from testAssistedMethodInvocation by verifying only the ID injection + */ public function testAssistedCustomInject(): void { $assistedConsumer = (new Injector(new FakeAssistedDbModule(), __DIR__ . '/tmp'))->getInstance(FakeAssistedParamsConsumer::class);tests/CompileInjectorTest.php (1)
Line range hint
97-104: LGTM with a suggestion for error message assertion.The typographical error in the method name has been fixed from "testCompileFaillureLog" to "testCompileFailureLog". The test logic is comprehensive, checking both file existence and log content.
Consider making the error message assertion more specific by checking for the exact error message pattern instead of just the word "Error":
- $this->assertStringContainsString('Error', $log); + $this->assertMatchesRegularExpression('/Error: .+ in .+/', $log);This would help catch any changes in the error message format.
tests/DependencyCompilerTest.php (1)
145-148: Enhance test readability with assertion messagesThe added type checks improve test reliability, but could be more descriptive.
Consider adding assertion messages for better debugging:
- assert(class_exists(FakeInvalidDependency::class)); - $fake = new FakeInvalidDependency(); - assert($fake instanceof DependencyInterface); + assert(class_exists(FakeInvalidDependency::class), 'FakeInvalidDependency class must exist'); + $fake = new FakeInvalidDependency(); + assert($fake instanceof DependencyInterface, 'FakeInvalidDependency must implement DependencyInterface');Also, consider adding a docblock to explain the test's purpose and expected exception scenario.
src/DependencyCode.php (1)
155-158: Consider extracting qualifier management to reduce duplication.The qualifier management pattern is duplicated between
getDependencyCodeandgetProviderCode. Consider extracting this into a private method to improve maintainability.Here's a suggested implementation:
+ private function handleQualifier(): ?IpQualifier + { + $qualifier = $this->qualifier; + $this->qualifier = null; + return $qualifier; + } private function getDependencyCode(Dependency $dependency): Code { // ... existing code ... - $qualifier = $this->qualifier; - $this->qualifier = null; + $qualifier = $this->handleQualifier(); return new Code($namespace, $isSingleton, $qualifier); } private function getProviderCode(DependencyProvider $provider): Code { // ... existing code ... - $qualifier = $this->qualifier; - $this->qualifier = null; + $qualifier = $this->handleQualifier(); return new Code($node, $isSingleton, $qualifier); }src/CompileInjector.php (3)
21-24: Well-defined type definitions enhance code safety!The addition of Psalm type definitions for
ScriptDirandIpimproves type safety and static analysis capabilities. Thenon-empty-stringconstraint forScriptDiris particularly valuable as it prevents potential file operation issues with empty paths.Consider documenting these types in your project's contribution guidelines to ensure consistent usage across the codebase.
85-97: Consider addressing the PHPStan limitation.The
@phpstan-ignore-linecomment suggests a type system limitation. This might be improved by:
- Creating a proper type for the injection point assignment
- Using a more specific type than
objectfor the instanceConsider creating a dedicated type:
+/** + * @template T of object + * @psalm-type InjectionPointAssignment = array{0: string, 1: string, 2: string} + */ function (string $dependencyIndex, array $injectionPoint = ['', '', '']) { - $this->ip = $injectionPoint; // @phpstan-ignore-line + /** @var InjectionPointAssignment $injectionPoint */ + $this->ip = $injectionPoint;
Line range hint
134-156: Consider refactoring to reduce complexity and suppressions.The method has multiple type suppressions and assertions, indicating underlying complexity that could be simplified:
- The UnusedLocalVariable suppression
- The DocblockTypeContradiction suppression
- The UnresolvableInclude suppression
Consider splitting this method into smaller, more focused methods with clearer type boundaries. For example:
- A method for singleton retrieval
- A method for instance creation
- A method for instance type validation
This would make the code more maintainable and reduce the need for type suppressions.
src/ScriptInjector.php (1)
171-171: Document why the PHPStan warning is suppressedConsider adding a brief comment explaining why this PHPStan warning needs to be suppressed. This helps future maintainers understand the reasoning.
- * @SuppressWarnings(PHPMD.UnusedLocalVariable) // @phpstan-ignore-line + * @SuppressWarnings(PHPMD.UnusedLocalVariable) + * @phpstan-ignore-line Variables may be used in dynamically included instance filestests/ScriptInjectorTest.php (1)
Line range hint
1-313: Overall changes improve test quality but seem tangential to PHP version updates.While these changes improve test readability and idiomaticity, they appear unrelated to the PR's main objective of updating PHP versions in CI. However, the changes are safe and beneficial:
- More readable error messages
- Opportunity to use more idiomatic PHPUnit assertions
Consider splitting such test improvements into a separate PR for clearer change tracking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
composer.json(1 hunks)docs/exmaple/ProdInjectorContext.php(1 hunks)phpstan.neon(1 hunks)phpunit.xml.dist(1 hunks)psalm.compiler.xml(0 hunks)psalm.xml(1 hunks)src-deprecated/ReaderProvider.php(1 hunks)src/AbstractInjectorContext.php(1 hunks)src/CachedInjectorFactory.php(4 hunks)src/Code.php(1 hunks)src/CompileInjector.php(5 hunks)src/CompileNullObject.php(1 hunks)src/DependencyCode.php(3 hunks)src/DependencySaver.php(1 hunks)src/DiCompiler.php(2 hunks)src/FunctionCode.php(1 hunks)src/GraphDumper.php(1 hunks)src/InjectionPoint.php(2 hunks)src/InjectorFactory.php(3 hunks)src/Normalizer.php(1 hunks)src/OnDemandCompiler.php(5 hunks)src/ScriptInjector.php(5 hunks)src/ScriptinjectorModule.php(1 hunks)tests/AssistedTest.php(3 hunks)tests/CachedFactoryTest.php(2 hunks)tests/CompileInjectorTest.php(2 hunks)tests/ContextInjectorTest.php(1 hunks)tests/DependencyCompilerTest.php(2 hunks)tests/DevCache.php(1 hunks)tests/DiCompilerTest.php(0 hunks)tests/Fake/Assisted/FakeAssistedInjectConsumer.php(1 hunks)tests/Fake/Deep/FakeScriptInjectorContext.php(1 hunks)tests/Fake/FakeCar3.php(1 hunks)tests/Fake/FakeInvalidDependency.php(1 hunks)tests/Fake/script/singleton.php(1 hunks)tests/InjectorFactoryTest.php(1 hunks)tests/MultiBindingTest.php(7 hunks)tests/ScriptInjectorTest.php(3 hunks)vendor-bin/tools/composer.json(1 hunks)
💤 Files with no reviewable changes (2)
- psalm.compiler.xml
- tests/DiCompilerTest.php
✅ Files skipped from review due to trivial changes (6)
- phpunit.xml.dist
- src-deprecated/ReaderProvider.php
- src/Code.php
- src/FunctionCode.php
- tests/Fake/Assisted/FakeAssistedInjectConsumer.php
- tests/Fake/script/singleton.php
🧰 Additional context used
📓 Learnings (1)
src/CompileInjector.php (1)
Learnt from: koriym
PR: ray-di/Ray.Compiler#112
File: src/CompileInjector.php:14-18
Timestamp: 2024-11-12T14:02:45.946Z
Learning: The `CompileInjector` class now uses `Compiler` to compile modules and initializes `AirInjector`. This change should be documented in the README.md.
🪛 GitHub Check: cs / Coding Standards
src/ScriptinjectorModule.php
[failure] 13-13:
Class name doesn't match filename; expected "class ScriptinjectorModule"
🔇 Additional comments (58)
tests/Fake/FakeCar3.php (1)
13-13: Verify the intention behind unsetting the constructor parameter
The addition of unset($dateTime) appears unnecessary as:
- Constructor parameters are automatically cleaned up when the method exits
- If this is a test fixture, explicitly unsetting the parameter might interfere with proper object initialization
Let's check if this is part of a specific test pattern:
✅ Verification successful
Unsetting constructor parameter is a common pattern in test fixtures
Looking at the codebase, unsetting parameters is a deliberate pattern used across multiple test fixtures in this project:
- It's used in various test classes like
FakeInvalidDependency,FakeAssistedInjectConsumer - The pattern is specifically used in test fixtures to explicitly indicate unused parameters
FakeCar3is used in dependency injection tests throughFakeUnboundModule, where the constructor parameter being unused is part of the test scenario
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar unset patterns in test fixtures
rg -g '*/tests/**/*.php' 'unset\(\$.*\);' -A 5 -B 5
# Look for related test cases that might explain this behavior
rg -g '*/tests/**/*.php' 'FakeCar3' -A 10 -B 10
Length of output: 399
Script:
#!/bin/bash
# Let's try without the glob pattern and use fd to find PHP files in tests
fd -e php -t f . tests/ -x rg -l 'unset\(\$.*\);' {}
# Find the actual FakeCar3 class file and its content
fd -e php -t f FakeCar3 tests/ -x cat {}
# Look for test cases using FakeCar3
fd -e php -t f . tests/ -x rg -l 'FakeCar3' {}
Length of output: 637
Script:
#!/bin/bash
# Let's check the content of other files that use unset to understand the pattern
fd -e php -t f . tests/ -x rg -A 5 -B 5 'unset\(\$.*\);' {}
# Check how FakeCar3 is used in FakeUnboundModule
cat tests/Fake/FakeUnboundModule.php
Length of output: 2266
phpstan.neon (2)
9-11: Verify the necessity of typeAlias.duplicate suppression
The addition of typeAlias.duplicate suppression suggests there might be duplicate type aliases in the codebase. This could lead to confusion and maintenance issues.
Let's identify the duplicate type aliases:
Consider refactoring the duplicate type aliases into a single, shared definition instead of suppressing the error. This would improve code maintainability and reduce potential confusion.
2-2: Consider maintaining maximum static analysis strictness
Changing the PHPStan level from 'max' (level 9) to level 8 reduces the static analysis strictness. While this might help with PHP 8.4 compatibility, it could potentially miss some type-related issues.
Let's check if there are any specific level 9 issues that need addressing:
Consider keeping the maximum level and explicitly suppressing specific issues if needed, rather than reducing the overall analysis level.
vendor-bin/tools/composer.json (2)
6-9: Verify compatibility between static analysis tools
The combination of PHPStan 2.0, Psalm 4.2, and other static analysis tools should be verified for potential conflicts.
✅ Verification successful
Let me check the configuration files to better understand the setup and potential conflicts.
No compatibility issues detected between static analysis tools
The configuration files show a well-structured setup with no conflicting rules:
- PHPStan is configured at level 8 with clear path inclusions/exclusions
- Psalm has specific issue handlers and proper project file configuration
- PHP_CodeSniffer follows PSR12 and Doctrine standards with carefully curated exclusions
- The tools are configured to work on the same source directories without conflicting settings
- Special annotations for both PHPStan and Psalm are properly handled in the coding standards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for known conflicts between static analysis tools
# Look for any tool-specific configuration files that might need updates
fd -e neon -e xml -e dist
Length of output: 97
Script:
#!/bin/bash
# Check PHPStan and Psalm configurations
echo "=== PHPStan Config ==="
cat phpstan.neon
echo -e "\n=== Psalm Config ==="
cat psalm.xml
# Check if there are any explicit compatibility settings or conflicting rules
echo -e "\n=== PHP CodeSniffer Config ==="
cat phpcs.xml
Length of output: 4928
6-6: Verify PHPStan 2.0 compatibility with PHP 8.3 and 8.4
The major version bump of PHPStan (1.x → 2.x) warrants verification of compatibility with the targeted PHP versions.
✅ Verification successful
PHPStan 2.0 is compatible with PHP 8.3 and 8.4
Based on the release notes and requirements:
- PHPStan 2.0 focuses on improved type analysis and memory optimization
- No explicit PHP version constraints are mentioned in the breaking changes
- The tool continues to support modern PHP versions, including PHP 8.3 and 8.4
- Major changes are related to internal improvements and stricter analysis rules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check PHPStan 2.x compatibility and requirements
# Check PHPStan's composer.json for PHP version requirements
curl -s https://raw.githubusercontent.com/phpstan/phpstan/2.x/composer.json | jq '.require.php'
# Look for any PHP version constraints in the repository
rg -g "*.json" -g "*.neon" "php|PHP" .
Length of output: 1883
Script:
#!/bin/bash
# Let's try a different approach to check PHPStan 2.x compatibility
# Check PHPStan's release notes and requirements from their GitHub repository
curl -s "https://api.github.com/repos/phpstan/phpstan/releases/tags/2.0.0" | jq -r '.body'
# Check the project's PHP version requirements and CI configuration
rg -g "*.yml" -g "*.yaml" "php" .
# Check PHPStan configuration for any version-specific settings
fd -e neon -x cat {}
Length of output: 27667
tests/Fake/FakeInvalidDependency.php (1)
Line range hint 1-33: Verify test coverage for this mock class.
Let's ensure this fake class is properly utilized in tests to validate the error handling of invalid dependencies.
✅ Verification successful
Let me gather more information about the test coverage by checking the test method implementation.
Let me try a different approach to get the test method implementation.
Test coverage for FakeInvalidDependency is adequate
The mock class is properly tested in testDomainException() method where it:
- Verifies the class exists and implements the correct interface
- Tests error handling by expecting a
DomainException - Attempts to generate code from the invalid dependency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this fake class is used in tests and how it's being tested
# Look for test files using this fake class
rg -l "FakeInvalidDependency" --type php
# Look for specific test assertions related to this class
rg "FakeInvalidDependency" -B 2 -A 2 --type php
Length of output: 971
Script:
#!/bin/bash
# Get the full test method implementation that uses FakeInvalidDependency
ast-grep --pattern 'function test$_() {
$$$
$$$FakeInvalidDependency$$$
$$$
}' tests/DependencyCompilerTest.php
Length of output: 119
Script:
#!/bin/bash
# Get the test method implementation using ripgrep with more context
rg "FakeInvalidDependency" -B 10 -A 10 tests/DependencyCompilerTest.php
Length of output: 894
psalm.xml (1)
5-5: Verify vendor-bin path in CI environment
The schema location has been moved to a vendor-bin structure. Please ensure that:
- The CI workflow properly installs dependencies in this new location
- The path
vendor-bin/tools/vendor/vimeo/psalm/config.xsdis correctly resolved during CI runs
✅ Verification successful
Schema location change is properly configured
The new schema location path is correctly set up in the repository:
- Verified
vendor-bin/tools/composer.jsonexists and includes the Psalm dependency (vimeo/psalm: ^4.2) - The repository uses a standard tools isolation approach with
vendor-binstructure - Static analysis workflow is configured through a reusable workflow in
ray-di/.github
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and setup of vendor-bin structure in the repository
# Check if there's a composer.json in vendor-bin/tools
fd -t f composer.json vendor-bin/tools
# Check if there are any CI workflow files referencing vendor-bin
fd -t f -e yml -e yaml . .github/workflows/ --exec grep -l "vendor-bin"
Length of output: 143
Script:
#!/bin/bash
# Let's verify the CI setup and Psalm configuration more thoroughly
# Check the content of vendor-bin/tools/composer.json to verify Psalm dependency
cat vendor-bin/tools/composer.json
# Check CI workflow files for PHP/Composer setup
fd -t f -e yml -e yaml . .github/workflows/ --exec cat {}
# Check if there are any other Psalm config files or references
fd psalm -e xml
Length of output: 1574
Script:
#!/bin/bash
# Let's check the reusable workflow that handles static analysis
# Fetch the reusable workflow content
gh api repos/ray-di/.github/contents/.github/workflows/static-analysis.yml --raw
# Check if there are any composer-related setup files
fd composer -e json
Length of output: 1543
src/CompileNullObject.php (2)
13-14: LGTM: Type import annotation added correctly
The @psalm-import-type ScriptDir from CompileInjector annotation enhances type safety by properly importing the ScriptDir type.
19-21:
Critical: Type mismatch between documentation and implementation
There's an inconsistency between:
- The
@param ScriptDir $scriptDirannotation - The actual parameter type
string $scriptDirin the method signature
This defeats the purpose of the type safety enhancement being implemented across the codebase.
Apply this fix to maintain consistency with other files:
- public function __invoke(Container $container, string $scriptDir): void
+ public function __invoke(Container $container, ScriptDir $scriptDir): voidLet's verify the ScriptDir type usage across the codebase:
src/ScriptinjectorModule.php (1)
15-16: Verify type consistency between constructor parameter and property
There's a type mismatch between:
- Constructor parameter type:
string - Property type annotation:
ScriptDir - Property assignment: Direct assignment without validation/casting
This could lead to type safety issues if ScriptDir has specific constraints or validation requirements.
Let's check how ScriptDir is defined and used elsewhere:
Also applies to: 19-22
✅ Verification successful
Let me gather more information about the ScriptDir type definition and its usage in CompileInjector:
Type consistency is maintained through psalm-type
The type consistency concern is resolved as ScriptDir is defined as non-empty-string in CompileInjector. The constructor's string parameter aligns with this type definition, and Psalm's static analysis ensures the non-empty constraint. The property type annotation correctly imports and uses this type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the definition and usage of ScriptDir type
ast-grep --pattern 'type ScriptDir = $_'
rg -A 2 '@psalm-import-type ScriptDir'
Length of output: 2031
Script:
#!/bin/bash
# Find the definition of ScriptDir in CompileInjector
rg -A 5 'psalm-type ScriptDir'
# Find constructor usage of ScriptDir
ast-grep --pattern 'public function __construct($_ $scriptDir, $$$)'
Length of output: 460
src/AbstractInjectorContext.php (2)
Line range hint 1-27: Verify compatibility with PHP 8.3 and 8.4
Since this PR is part of updating PHP versions support (8.3 and 8.4), please ensure that:
- The readonly property syntax is supported in these versions
- The type declarations are compatible
✅ Verification successful
The code is compatible with PHP 8.3 and 8.4
The @readonly annotation and type declarations used in the code are fully compatible with PHP 8.3 and 8.4:
- The
@readonlyannotation is used consistently across the codebase as a docblock annotation, not using the newer readonly property modifier syntax - The
ScriptDirtype is properly defined as a psalm type alias (@psalm-type ScriptDir = non-empty-string) and imported across files - The type declarations used (
string, basic types) are supported across all modern PHP versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other readonly properties and type declarations
echo "Searching for readonly properties:"
ast-grep --pattern '@readonly'
echo -e "\nSearching for ScriptDir type usage:"
rg "ScriptDir" --type php
Length of output: 3937
9-9: Verify the correct source of ScriptDir type
There appears to be an inconsistency between the import statement and the psalm annotation:
- Importing
ScriptDirfromRay\Di\Annotation - Psalm importing
ScriptDirtype fromCompileInjector
Please verify which is the correct source for the ScriptDir type.
Also applies to: 11-13
✅ Verification successful
The import and psalm type annotation are correctly used
The verification shows that:
ScriptDiris correctly imported fromRay\Di\Annotationnamespace for use as an annotation class- The psalm type annotation refers to the type definition in
CompileInjector.phpwhich definesScriptDiras anon-empty-string - Both usages are intentional and serve different purposes:
- The annotation import is for dependency injection binding
- The psalm type import is for static analysis type checking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ScriptDir definitions and usages
echo "Searching for ScriptDir definitions:"
ast-grep --pattern 'class ScriptDir'
echo -e "\nSearching for ScriptDir type imports:"
rg -l "ScriptDir" --type php
Length of output: 570
Script:
#!/bin/bash
# Let's check the actual definition and namespace of ScriptDir
echo "Checking ScriptDir in Ray\Di\Annotation namespace:"
rg -C 3 "namespace Ray\\Di\\Annotation" --type php
echo -e "\nChecking ScriptDir class definition:"
rg -C 3 "class ScriptDir" --type php
echo -e "\nChecking ScriptDir usage in CompileInjector:"
rg -C 3 "ScriptDir" src/CompileInjector.php
Length of output: 1853
docs/exmaple/ProdInjectorContext.php (3)
25-27: LGTM: Proper dependency validation added
The addition of the class existence check is a good practice for better error handling and clearer dependency requirements.
Line range hint 1-30: Directory name contains a typo
The file is located in docs/exmaple/ directory, which appears to be a typo of "example". This should be corrected for better maintainability.
29-29: Consider removing @phpstan-ignore-line comment
The PHPStan ignore comment suggests there might be a type-related issue that needs proper fixing instead of being suppressed.
tests/Fake/Deep/FakeScriptInjectorContext.php (1)
28-28: Please clarify the intent behind unsetting the parameter.
The addition of unset($id) in the doContains method seems unusual. Could you please clarify:
- Is this intended to test a specific edge case?
- Or is this meant to suppress an unused variable warning?
- Or should this be unsetting something else?
This change might affect test behavior, so understanding the intention would help ensure it's serving its purpose correctly.
tests/DevCache.php (2)
Line range hint 1-89: Implementation looks good!
The DevCache class is well-structured with:
- Proper type declarations
- Consistent delegation to the wrapped cache
- Clear testing purpose with the static
$wasHitflag
73-73: Verify type compatibility after PHPStan ignore removal
The removal of the PHPStan ignore directive suggests improved type compatibility, but let's verify that $this->cache->getStats() return type aligns with the method's declared return type of ?array<string>.
Let's check the type compatibility:
✅ Verification successful
Let me gather more information about the type compatibility by examining the actual implementations.
Let me check one more thing to understand the cache interface and its implementation better.
Based on the gathered information, I can now provide the final response:
Removal of PHPStan ignore is safe
The type compatibility is maintained for the following reasons:
- DevCache's
$cacheproperty is properly typed with@var Cacheannotation - The constructor accepts a
CacheProviderinstance - The
doGetStats()method is correctly annotated with@return ?array<string> - The method simply delegates to the underlying cache's
getStats()method
The removal of the PHPStan ignore directive appears to be part of broader static analysis improvements and doesn't introduce any type compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type compatibility between CacheProvider and DevCache implementations
# Find CacheProvider's doGetStats return type
echo "Checking CacheProvider's doGetStats definition:"
ast-grep --pattern 'function doGetStats()'
# Find other cache implementations for comparison
echo "Checking other cache implementations:"
rg -l "extends.*CacheProvider"
Length of output: 335
Script:
#!/bin/bash
# Check the actual implementation and type hints in DevCache and related files
echo "Checking DevCache implementation:"
rg -A 5 "doGetStats|getStats" tests/DevCache.php
echo -e "\nChecking CacheProvider implementation:"
fd -e php -x rg -A 5 "doGetStats|getStats" {} \;
echo -e "\nChecking for PHPStan related annotations:"
rg "@phpstan|@return" tests/DevCache.php
Length of output: 886
Script:
#!/bin/bash
# Check the cache property type and initialization
echo "Checking cache property and constructor in DevCache:"
rg -B 5 "protected \$cache|__construct.*cache" tests/DevCache.php
echo -e "\nChecking for Cache interface definition:"
fd -e php -x rg -A 5 "interface.*Cache|getStats" {} \;
Length of output: 555
src/InjectorFactory.php (2)
9-9: LGTM: Type safety improvements
The addition of ScriptDir import and psalm type import enhances type safety and static analysis capabilities.
Also applies to: 19-20
55-55: LGTM: Class name correction
The class name has been correctly updated from ScriptinjectorModule to ScriptInjectorModule, improving naming consistency.
src/DependencySaver.php (3)
19-21: LGTM: Good use of type importing!
The @psalm-import-type annotation properly imports the ScriptDir type, enhancing type safety and static analysis capabilities.
24-24: LGTM: Proper type annotation for the property
The property type annotation correctly uses the imported ScriptDir type.
Line range hint 41-63: Verify string operations compatibility with ScriptDir type
The class performs string operations with $scriptDir (concatenation, sprintf). We should verify that the ScriptDir type properly handles these string operations.
✅ Verification successful
Let me gather more information about the ScriptDir usage and implementation.
Let me try one more search to find the actual type definition and its usage:
Based on the search results, I can now provide the final response:
String operations with ScriptDir type are safe
The ScriptDir type is defined in CompileInjector as @psalm-type ScriptDir = non-empty-string, which confirms it's a string type that supports all string operations. The string concatenation and sprintf operations in DependencySaver are type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ScriptDir class definition and its string handling capabilities
ast-grep --pattern 'class ScriptDir {
$$$
}'
# Look for other usages of ScriptDir with string operations
rg -A 2 'ScriptDir.*\.'
Length of output: 425
Script:
#!/bin/bash
# Search for files containing ScriptDir
fd -t f ScriptDir
# Look for string operations with ScriptDir in the codebase
rg -A 3 'ScriptDir.*(\+|\.|\/)'
# Search for ScriptDir type declarations or imports
rg -A 2 '(use|type|class).*ScriptDir'
Length of output: 3555
Script:
#!/bin/bash
# Fix the regex for string operations
rg -A 3 "ScriptDir.*(\\+|\\.|/)"
# Look for the actual type definition in CompileInjector
rg -A 5 "@psalm-type ScriptDir"
# Look for string operations in DependencySaver
rg -A 3 "\$this->scriptDir.*(\+|\.)" src/DependencySaver.php
Length of output: 3307
tests/CachedFactoryTest.php (2)
14-16: LGTM! Good type safety improvement.
The introduction of the Context type alias enhances code clarity and type safety by explicitly defining valid context values.
37-38: LGTM! Proper usage of the new type alias.
The parameter annotation correctly utilizes the newly defined Context type, improving static analysis capabilities.
tests/InjectorFactoryTest.php (1)
30-33: LGTM! Clean implementation of module composition.
The code correctly initializes and composes modules using the builder pattern, following good testing practices.
src/GraphDumper.php (1)
Line range hint 56-57: Verify ScriptDir type compatibility with string operations
The $scriptDir is now typed as ScriptDir but is being used in string concatenation operations. Please ensure that the ScriptDir type properly handles string operations or implements __toString().
Let's verify the ScriptDir implementation:
Also applies to: 70-71
tests/ContextInjectorTest.php (1)
55-55: LGTM! The simplified test maintains proper coverage.
The removal of redundant type assertion for FakeDeep and using direct getInstance call maintains the test's effectiveness while making it more concise. The test still properly verifies:
- State persistence within the same injector instance
- State reset when getting a new injector instance
src/OnDemandCompiler.php (3)
11-11: LGTM: Type definitions enhance code clarity and safety
The addition of the ScriptDir import and psalm type definitions improves type safety and documentation.
Also applies to: 26-29
75-75: LGTM: Improved type consistency and static analysis
The changes to return types and type annotations enhance code clarity and static analysis capabilities.
Also applies to: 77-77, 87-89, 93-93, 99-99
98-102: Verify error handling compatibility with PHP 8.3/8.4
The error reporting manipulation around unserialize might need verification with newer PHP versions.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The error reporting manipulation for unserialize is consistent with PHP 8.x compatibility
The codebase shows consistent error handling patterns around unserialize across multiple files (OnDemandCompiler.php, ScriptInjector.php, InjectionPoint.php). The composer.json indicates support for PHP ^7.2 || ^8.0, and the error reporting manipulation is a standard practice to suppress E_NOTICE during unserialization. This pattern is safe to use in PHP 8.x as:
- It's a temporary scope-limited change
- The original error reporting level is properly restored
- The unserialize call uses the secure ['allowed_classes' => true] parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any changes in error handling behavior for unserialize in PHP 8.3/8.4
rg -A 5 "error_reporting.*E_NOTICE" . | grep -v "OnDemandCompiler.php"
# Look for similar patterns in tests
fd -e php -e phpt --exec grep -l "unserialize.*error_reporting" {} \;
Length of output: 538
Script:
#!/bin/bash
# Check for any test cases related to unserialize and error handling
rg -B 2 -A 2 "unserialize.*allowed_classes" .
# Look for any PHP version constraints or requirements
fd "composer.json|README.md" --exec cat {} \;
# Check for any error handling related tests
rg -B 2 -A 2 "error_reporting" "tests/"
Length of output: 6550
src/CachedInjectorFactory.php (2)
9-9: LGTM: Import and type declaration are properly added
The ScriptDir import and psalm-import-type annotation are correctly implemented, enhancing type safety.
Also applies to: 17-19
Line range hint 1-89: Verify consistent ScriptDir usage across the codebase
Since this is part of a larger refactoring to use ScriptDir, we should verify that all callers of these methods are updated accordingly.
✅ Verification successful
Let me gather more information about the ScriptDir type and its usage.
Based on the gathered information, I can now generate the final response:
ScriptDir type is consistently used across the codebase
The codebase shows proper and consistent usage of ScriptDir:
- All relevant files import
Ray\Di\Annotation\ScriptDir - The type is properly defined in
CompileInjector.phpas@psalm-type ScriptDir = non-empty-string - All files correctly import the type using
@psalm-import-type ScriptDir from CompileInjector - The string type hints in
CachedInjectorFactory.phpare accompanied by proper@param ScriptDirannotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining string type hints or direct string usage for script directories
rg -t php "function.*\(.*string.*scriptDir.*\)"
rg -t php "getInstance.*\(.*string.*scriptDir.*\)"
# Search for usages of these methods to ensure they're passing ScriptDir
ast-grep --pattern 'getInstance($_, $scriptDir, $_)'
ast-grep --pattern 'getOverrideInstance($scriptDir, $_)'
Length of output: 1886
Script:
#!/bin/bash
# Search for ScriptDir type definition and usage
rg -t php "ScriptDir"
# Look for files that might be importing or using ScriptDir
ast-grep --pattern 'use Ray\Di\Annotation\ScriptDir'
# Check for parameter type hints using ScriptDir
ast-grep --pattern 'function $_($_: ScriptDir)'
Length of output: 3841
src/InjectionPoint.php (3)
9-9: LGTM!
The import statement for ScriptDir is correctly placed and properly namespaced.
Line range hint 93-99: Verify ScriptDir usage in path construction
The $scriptDir is used directly in a string format operation. Ensure that the ScriptDir type properly handles string conversion or provides specific methods for path construction.
23-25: Verify the ScriptDir type definition in CompileInjector
The psalm type import looks correct, but let's verify the source type exists.
✅ Verification successful
Let me search for the actual type definition in CompileInjector.
Let me try a different approach with ripgrep to find the type definition.
ScriptDir type import is correctly defined and used
The ScriptDir type is properly defined in CompileInjector as non-empty-string and is correctly imported in InjectionPoint.php. The type is consistently used across the codebase for script directory paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ScriptDir type definition in CompileInjector
rg -t php "ScriptDir" --context 3
Length of output: 17388
Script:
#!/bin/bash
# Search for ScriptDir type definition in CompileInjector
ast-grep --pattern $'@psalm-type ScriptDir = $_'
Length of output: 49
Script:
#!/bin/bash
# Search for ScriptDir type definition in CompileInjector with context
rg -t php "@psalm-type ScriptDir" -A 2 -B 2
Length of output: 277
tests/AssistedTest.php (2)
40-41: LGTM! Variable naming is now correct.
The variable name is properly spelled as "expected" and the assertion logic is correct.
50-51: LGTM! Variable naming is consistent.
The variable name is properly spelled as "expected" and the assertion uses the appropriate strict comparison.
src/Normalizer.php (2)
86-87: LGTM! Helpful type assertion for static analysis.
The added type assertion helps static analyzers understand that $value is specifically an array with integer keys after the is_array check, which is correct given the usage in arrayValue.
79-80: LGTM! Type annotation improvement.
The change from array<mixed> to array<int, mixed> better reflects the actual array structure expected by the arrayValue method and improves type safety.
Let's verify that all array values passed to this method have integer keys:
✅ Verification successful
Type annotation change is correct and safe
The code analysis reveals that the type annotation change is accurate and properly enforced:
getValueNodeNonAtomicis only called fromgetValueNodewhich handles primitive types first- When arrays are passed, they are immediately processed by
arrayValuewhich explicitly requiresarray<int, mixed> - The array handling in
arrayValuemethod uses numeric iteration with++$lastKey === $itemKeycomparison, confirming it expects sequential integer keys - The type assertion
@var array<int, mixed> $valueis added before callingarrayValueto ensure type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any non-integer array key usage with getValueNodeNonAtomic
# Look for array assignments or method calls that might pass arrays to this method
ast-grep --pattern 'getValueNodeNonAtomic($_)'
Length of output: 48
Script:
#!/bin/bash
# Let's try a different approach to find the method and its usages
# First, let's find the file content to understand the context better
echo "=== Normalizer.php content ==="
cat src/Normalizer.php
# Then search for any array operations or method calls that might interact with getValueNodeNonAtomic
echo -e "\n=== Searching for array operations near getValueNodeNonAtomic ==="
rg "getValueNodeNonAtomic" -A 5 -B 5
Length of output: 4573
tests/CompileInjectorTest.php (1)
55-57: LGTM! Method name corrected.
The typographical error in the method name has been fixed from "testInjectopnPoint" to "testInjectionPoint".
src/DiCompiler.php (2)
28-30: LGTM! Type safety improvements look good.
The addition of the psalm type import and the property type change to ScriptDir enhance type safety.
Also applies to: 33-33
Line range hint 28-58: Verify consistent ScriptDir type usage across the codebase.
Let's ensure that the ScriptDir type is used consistently in other files and that no instances were missed.
✅ Verification successful
ScriptDir type usage is consistent across the codebase
The verification shows that ScriptDir type is consistently used in both docblock annotations and imports across the codebase. All files that handle $scriptDir parameter properly import and use the ScriptDir type annotation. The type usage in DiCompiler.php aligns with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for string type hints that might need to be updated to ScriptDir
rg "string.*\\\$scriptDir" --type php
# Search for ScriptDir usage to verify consistency
rg "ScriptDir.*\\\$scriptDir" --type php
# Check for potential missed imports
rg "use.*ScriptDir" --type php
Length of output: 3325
tests/DependencyCompilerTest.php (2)
10-10: LGTM: Well-structured import additions
The new imports enhance type safety and runtime checks, aligning well with PHP 8.x best practices.
Also applies to: 14-15
145-148: Verify assert compatibility across PHP versions
Since this PR updates PHP version support, let's ensure the assert usage is compatible.
✅ Verification successful
Let me gather more information about the PHP version requirements and assert configurations in the project.
Based on the gathered information, I can now provide the final response:
Assert usage is compatible with supported PHP versions
The assert usage in the test files is compatible with PHP 7.2 and PHP 8.0+ (as specified in composer.json) because:
- The code uses language-level assertions with proper function imports (
use function assert;) - The assertions are used for type checking in test files, which is a valid use case
- No custom assert configurations are detected that might cause compatibility issues
- The assert syntax used (
assert($instance instanceof ClassName)) is supported across all targeted PHP versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for assert configurations in php.ini settings across the codebase
rg -l "zend.assertions|assert.exception" .
# Check for other assert usage patterns in tests
rg "assert\(" "tests/"
Length of output: 658
Script:
#!/bin/bash
# Check PHP version requirements in composer.json
cat composer.json | jq -r '.require.php'
# Look for any assert related configurations in the project
fd -e php -e ini -e xml -e json . -x rg -l "assert" {}
# Check if there are any assert related test configurations
fd -g "*test*.php" -x rg -B2 -A2 "assert" {}
Length of output: 18913
tests/MultiBindingTest.php (3)
45-47: LGTM! Type hints improve static analysis capabilities
The addition of Map<FakeEngineInterface> type hints across multiple methods enhances type safety and IDE support. These changes are consistent and well-documented.
Also applies to: 58-59, 69-70, 81-82, 92-93, 103-104
122-122: LGTM! Method name corrected
The method name has been properly corrected from 'testMultipileModule' to 'testMultipleModule', fixing the typo while maintaining the test's purpose.
Line range hint 33-33: Consider updating the @requires PHP version annotation
Given that the PR updates PHP versions to 8.3/8.4, consider if the @requires PHP 8.0 annotation should be updated to reflect the new minimum supported version.
src/DependencyCode.php (2)
91-93: LGTM! Spelling correction implemented correctly.
The method signature and implementation are properly typed and follow best practices. The spelling correction from "Qaulifier" to "Qualifier" has been implemented consistently.
126-129: LGTM! Clean state management for qualifier.
The code properly manages the qualifier state by:
- Storing the current qualifier
- Resetting the instance variable
- Passing the stored qualifier to the Code constructor
This prevents any qualifier state from leaking between subsequent calls.
src/CompileInjector.php (2)
30-30: Property types properly aligned with Psalm definitions.
The property type annotations now use the custom Psalm types, providing stronger type guarantees for the critical scriptDir and injection point properties.
Also applies to: 38-38
59-68: 🛠️ Refactor suggestion
Verify directory permissions for scriptDir.
While the type safety improvements are good, it's crucial to ensure the script directory exists and is writable, as this class performs file operations.
Let's check the codebase for directory validation:
Consider adding directory validation:
public function __construct($scriptDir, LazyModuleInterface $lazyModule)
{
/** @var ScriptDir $scriptDir */
$scriptDir = rtrim($scriptDir, '/');
+ if (!is_dir($scriptDir) || !is_writable($scriptDir)) {
+ throw new \RuntimeException("Script directory must exist and be writable: {$scriptDir}");
+ }
$this->scriptDir = $scriptDir;src/ScriptInjector.php (4)
41-44: LGTM: Type imports enhance type safety
The addition of psalm type imports from CompileInjector helps maintain consistency in type definitions across the codebase.
119-121: LGTM: Type alias improves code clarity
The change from an explicit array type to the Ip type alias improves code readability while maintaining the same type structure.
93-94: Verify all constructor calls are updated for ScriptDir type
The constructor parameter type has been updated from string to ScriptDir. We should verify that all instantiations of ScriptInjector have been updated accordingly.
55-55: Ensure constructor validates non-empty-string constraint
The property type has been strengthened to non-empty-string, but we should ensure the constructor validates this constraint.
✅ Verification successful
Validation for non-empty string is already enforced
The non-empty-string constraint is already enforced through the following mechanisms:
- The
InjectorFactoryvalidates and creates the directory if it doesn't exist:
! is_dir($scriptDir) && ! @mkdir($scriptDir) && ! is_dir($scriptDir);- The
CompileInjectortrims trailing slashes and ensures non-empty:
$scriptDir = rtrim($scriptDir, '/');- The directory must exist for the injector to work, as it's used for file operations throughout the codebase.
The property type change to non-empty-string is therefore correctly reflecting the existing runtime constraints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the constructor or any setter validates the scriptDir
# Look for empty string checks or assertions
rg -A 5 "\$scriptDir\s*=|__construct.*\$scriptDir"
Length of output: 50
Script:
#!/bin/bash
# Let's try a broader search to find the constructor and any scriptDir usage
ast-grep --pattern 'class ScriptInjector {
$$$
public function __construct($$$) {
$$$
}
$$$
}'
Length of output: 108
Script:
#!/bin/bash
# Also search for any validation patterns
rg -B2 -A5 "scriptDir" --type php
Length of output: 32579
Script:
#!/bin/bash
# Let's check for directory validation in InjectorFactory and constructor validation
ast-grep --pattern 'public function __construct($scriptDir, $$$) {
$$$
}'
Length of output: 77
Script:
#!/bin/bash
# Let's also check for any validation patterns in the codebase
rg "empty.*scriptDir|!is_dir.*scriptDir|mkdir.*scriptDir" --type php
Length of output: 172
tests/ScriptInjectorTest.php (2)
172-174: LGTM! Error message readability improved.
The hyphenation in the error message makes it more readable, and the PHPStan ignore annotation is appropriately used for the intentionally invalid class name in the test.
213-213: Same issue: Use assertInstanceOf instead of assertTrue with instanceof.
Similar to line 186, use assertInstanceOf for a more idiomatic assertion.
- $this->assertTrue($car instanceof FakeCar); // @phpstan-ignore-line
+ $this->assertInstanceOf(FakeCar::class, $car);
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This is because following getCode($fake) .. make sure $fake type to avoid false positive inspection by phpstan.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
vendor-bin/tools/composer.json (1)
6-9: Consider updating other static analysis toolsWhile upgrading PHPStan is good, other static analysis tools like Psalm (^4.2) and PHP_CodeSniffer (^3.5) are still on older versions. Consider reviewing and updating these as well to ensure consistent support for newer PHP versions.
tests/Fake/FakeInvalidDependency.php (2)
15-15: LGTM! Consider adding documentation for intentional invalid behavior.The
unsetstatements are appropriate for simulating invalid dependency behavior in tests. However, it would be helpful to add comments explaining that this is intentional test behavior.public function inject(Container $container) { + // Intentionally invalidate container for testing unset($container); } public function register(array &$container, Bind $bind) { + // Intentionally invalidate dependencies for testing unset($container, $bind); } public function setScope($scope) { + // Intentionally invalidate scope for testing unset($scope); }Also applies to: 20-20, 25-25
Line range hint
1-33: Add class-level documentation to explain test purpose.Since this is a test fake class intentionally implementing invalid behavior, it would be beneficial to add class-level PHPDoc explaining its purpose and usage.
<?php declare(strict_types=1); namespace Ray\Compiler; use Ray\Di\Bind; use Ray\Di\Container; use Ray\Di\DependencyInterface; +/** + * A fake dependency implementation that intentionally invalidates all dependencies. + * This class is used for testing error handling and invalid dependency scenarios. + */ class FakeInvalidDependency implements DependencyInterfacesrc/ScriptInjectorModule.php (1)
Line range hint
18-24: Fix type mismatch in constructor parameterThere's an inconsistency between the PHPDoc type annotation (
ScriptDir), the actual parameter type (string), and the property type (ScriptDir). This breaks type safety and contradicts the changes made elsewhere.Apply this fix:
- public function __construct(string $scriptDir, ?AbstractModule $module = null) + public function __construct(ScriptDir $scriptDir, ?AbstractModule $module = null)docs/exmaple/ProdInjectorContext.php (1)
Line range hint
1-1: Fix typo in directory nameThe file is located in a directory named "exmaple" which appears to be a typo of "example".
Consider renaming the directory from "docs/exmaple" to "docs/example" to fix this typographical error.
tests/CachedFactoryTest.php (1)
Missing cache directories need to be created before test execution
The code review revealed important findings:
- The cache directories are not automatically created by
CachedInjectorFactoryorCompileInjector- These directories are required for the tests to work properly
- The test class doesn't have setup/teardown methods to manage these directories
Action items:
- Create a
setUpmethod inCachedFactoryTestto ensure test directories exist- Create a
tearDownmethod to clean up after tests- Add directory creation to the test setup:
protected function setUp(): void { mkdir(__DIR__ . '/tmp/dev', 0777, true); mkdir(__DIR__ . '/tmp/prod', 0777, true); }The original suggestion to extract paths to constants is still valid but secondary to fixing the missing directories issue.
🔗 Analysis chain
Line range hint
39-58: Consider extracting paths to constants and verify directory existenceWhile the implementation is correct, the hardcoded paths could be moved to class constants for better maintainability. Additionally, we should ensure the tmp directories exist.
Consider refactoring the paths:
+ private const DEV_CACHE_DIR = __DIR__ . '/tmp/dev'; + private const PROD_CACHE_DIR = __DIR__ . '/tmp/prod'; private function getInjector(string $context): InjectorInterface { if ($context === 'dev') { return CachedInjectorFactory::getInstance( 'dev', - __DIR__ . '/tmp/dev', + self::DEV_CACHE_DIR, static function (): AbstractModule { return new FakeToBindPrototypeModule(); } ); } return CachedInjectorFactory::getInstance( 'prod', - __DIR__ . '/tmp/prod', + self::PROD_CACHE_DIR, static function (): AbstractModule {
Let me verify if these directories are created during test setup or if there are any related configuration files.
Let me check the actual content of the test file and the factory class to understand how these directories are handled.
Let me check if there's any directory creation logic in the factory class itself since it's the one managing these cache directories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the tmp directories exist and are writable test_dirs=("tests/tmp/dev" "tests/tmp/prod") for dir in "${test_dirs[@]}"; do if [ ! -d "$dir" ]; then echo "Warning: Directory $dir does not exist" elif [ ! -w "$dir" ]; then echo "Warning: Directory $dir is not writable" fi doneLength of output: 383
Script:
#!/bin/bash # Check for directory creation in test setup rg -l "tmp/(?:dev|prod)" tests/ # Check for any setup/teardown methods ast-grep --pattern 'function set[uU]p() { $$$ }' ast-grep --pattern 'function setUp(): void { $$$ }' # Check for directory creation in the factory class rg -l "mkdir|directory" tests/Length of output: 199
Script:
#!/bin/bash # Check the content of the test file cat tests/CachedFactoryTest.php # Check if there's any cleanup/teardown logic ast-grep --pattern 'function tearDown() { $$$ }' ast-grep --pattern 'function tearDown(): void { $$$ }' # Look for any directory-related operations in the factory class rg -p "CachedInjectorFactory" -A 5 -B 5Length of output: 3972
Script:
#!/bin/bash # Check the content of the factory class cat src/CachedInjectorFactory.php # Also check if there are any directory-related operations in the CompileInjector cat src/CompileInjector.phpLength of output: 9629
src/Normalizer.php (1)
79-86: Good type safety improvements.These type refinements enhance code clarity and static analysis capabilities, which is particularly valuable as the project moves to support PHP 8.3 and 8.4. The changes maintain backward compatibility while leveraging modern PHP type system features.
Consider applying similar type refinements to other parts of the codebase to maintain consistency and maximize the benefits of PHP's type system.
src/CompileInjector.php (3)
59-68: Consider adding validation for empty script directory.While the type assertion ensures type safety after
rtrim, consider adding explicit validation to ensure the script directory exists and is writable.public function __construct($scriptDir, LazyModuleInterface $lazyModule) { /** @var ScriptDir $scriptDir */ $scriptDir = rtrim($scriptDir, '/'); + if (!is_dir($scriptDir) || !is_writable($scriptDir)) { + throw new \InvalidArgumentException( + sprintf('Script directory "%s" does not exist or is not writable', $scriptDir) + ); + } $this->scriptDir = $scriptDir; $this->lazyModule = $lazyModule; $this->registerLoader();
73-75: Document the reason for PHPStan ignore.The type hints for injection point parameters and return types are well-defined. However, please document why the PHPStan ignore comment is necessary on line 94.
- $this->ip = $injectionPoint; // @phpstan-ignore-line + // @phpstan-ignore-line Assignment type mismatch is expected here as we're dealing with a runtime type that PHPStan can't infer + $this->ip = $injectionPoint;Also applies to: 85-87, 94-94
Line range hint
21-156: Consider documenting type safety improvements in README.The type safety improvements in this class are significant and align well with the previous changes mentioned in the learnings. Consider updating the README.md to document these type safety enhancements, particularly the introduction of
ScriptDirandIptypes, as they might be relevant for downstream consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
composer.json(1 hunks)docs/exmaple/ProdInjectorContext.php(1 hunks)phpstan.neon(1 hunks)phpunit.xml.dist(1 hunks)psalm.compiler.xml(0 hunks)psalm.xml(1 hunks)src-deprecated/ReaderProvider.php(1 hunks)src/AbstractInjectorContext.php(1 hunks)src/CachedInjectorFactory.php(4 hunks)src/Code.php(1 hunks)src/CompileInjector.php(5 hunks)src/CompileNullObject.php(1 hunks)src/DependencyCode.php(3 hunks)src/DependencySaver.php(1 hunks)src/DiCompiler.php(2 hunks)src/FunctionCode.php(1 hunks)src/GraphDumper.php(1 hunks)src/InjectionPoint.php(2 hunks)src/InjectorFactory.php(3 hunks)src/Normalizer.php(1 hunks)src/OnDemandCompiler.php(5 hunks)src/ScriptInjector.php(5 hunks)src/ScriptInjectorModule.php(1 hunks)tests/AssistedTest.php(3 hunks)tests/CachedFactoryTest.php(2 hunks)tests/CompileInjectorTest.php(2 hunks)tests/ContextInjectorTest.php(1 hunks)tests/DependencyCompilerTest.php(2 hunks)tests/DevCache.php(1 hunks)tests/DiCompilerTest.php(0 hunks)tests/Fake/Assisted/FakeAssistedInjectConsumer.php(1 hunks)tests/Fake/Deep/FakeScriptInjectorContext.php(1 hunks)tests/Fake/FakeCar3.php(1 hunks)tests/Fake/FakeInvalidDependency.php(1 hunks)tests/Fake/script/singleton.php(1 hunks)tests/InjectorFactoryTest.php(1 hunks)tests/MultiBindingTest.php(7 hunks)tests/ScriptInjectorTest.php(3 hunks)vendor-bin/tools/composer.json(1 hunks)
💤 Files with no reviewable changes (2)
- psalm.compiler.xml
- tests/DiCompilerTest.php
✅ Files skipped from review due to trivial changes (8)
- phpunit.xml.dist
- src-deprecated/ReaderProvider.php
- src/Code.php
- src/FunctionCode.php
- tests/AssistedTest.php
- tests/DevCache.php
- tests/Fake/Assisted/FakeAssistedInjectConsumer.php
- tests/Fake/script/singleton.php
🧰 Additional context used
📓 Learnings (8)
src/AbstractInjectorContext.php (1)
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/AbstractInjectorContext.php:22-27
Timestamp: 2024-11-27T01:36:08.714Z
Learning: In `src/AbstractInjectorContext.php`, when `@psalm-type` is used to define a type alias like `ScriptDir`, the constructor parameter `$tmpDir` should remain typed as `string` in the method signature since `ScriptDir` is not a native class.
src/CachedInjectorFactory.php (1)
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/CachedInjectorFactory.php:28-28
Timestamp: 2024-11-27T01:37:05.831Z
Learning: In `src/CachedInjectorFactory.php`, for the `getInstance` method, the `scriptDir` parameter should remain typed as `string` in the method signature, even if the docblock specifies `ScriptDir`.
src/CompileInjector.php (1)
Learnt from: koriym
PR: ray-di/Ray.Compiler#112
File: src/CompileInjector.php:14-18
Timestamp: 2024-11-12T14:02:45.946Z
Learning: The `CompileInjector` class now uses `Compiler` to compile modules and initializes `AirInjector`. This change should be documented in the README.md.
src/DiCompiler.php (1)
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/DiCompiler.php:51-58
Timestamp: 2024-11-27T01:37:48.101Z
Learning: In `src/DiCompiler.php`, modifications to the type handling of `$scriptDir` in the `__construct` method have been previously discussed and do not need further changes.
src/InjectionPoint.php (3)
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/InjectionPoint.php:31-32
Timestamp: 2024-11-27T01:37:17.856Z
Learning: In the `InjectionPoint` class in `src/InjectionPoint.php`, it's acceptable for the property `$scriptDir` to be typed as `ScriptDir`, while the constructor parameter `$scriptDir` is typed as `string`.
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/DiCompiler.php:51-58
Timestamp: 2024-11-27T01:37:48.101Z
Learning: In `src/DiCompiler.php`, modifications to the type handling of `$scriptDir` in the `__construct` method have been previously discussed and do not need further changes.
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/InjectorFactory.php:49-51
Timestamp: 2024-11-27T01:36:42.749Z
Learning: In the Ray.Compiler project, when the method parameter type is `string`, even if the docblock indicates `@param ScriptDir $scriptDir`, the parameter type hint should remain as `string` and not be changed to `ScriptDir`.
src/InjectorFactory.php (2)
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/InjectorFactory.php:49-51
Timestamp: 2024-11-27T01:36:42.749Z
Learning: In the Ray.Compiler project, when the method parameter type is `string`, even if the docblock indicates `@param ScriptDir $scriptDir`, the parameter type hint should remain as `string` and not be changed to `ScriptDir`.
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/CachedInjectorFactory.php:28-28
Timestamp: 2024-11-27T01:37:05.831Z
Learning: In `src/CachedInjectorFactory.php`, for the `getInstance` method, the `scriptDir` parameter should remain typed as `string` in the method signature, even if the docblock specifies `ScriptDir`.
src/OnDemandCompiler.php (3)
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/InjectionPoint.php:31-32
Timestamp: 2024-11-27T01:37:17.856Z
Learning: In the `InjectionPoint` class in `src/InjectionPoint.php`, it's acceptable for the property `$scriptDir` to be typed as `ScriptDir`, while the constructor parameter `$scriptDir` is typed as `string`.
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/InjectorFactory.php:49-51
Timestamp: 2024-11-27T01:36:42.749Z
Learning: In the Ray.Compiler project, when the method parameter type is `string`, even if the docblock indicates `@param ScriptDir $scriptDir`, the parameter type hint should remain as `string` and not be changed to `ScriptDir`.
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/CachedInjectorFactory.php:28-28
Timestamp: 2024-11-27T01:37:05.831Z
Learning: In `src/CachedInjectorFactory.php`, for the `getInstance` method, the `scriptDir` parameter should remain typed as `string` in the method signature, even if the docblock specifies `ScriptDir`.
src/ScriptInjector.php (1)
Learnt from: koriym
PR: ray-di/Ray.Compiler#114
File: src/ScriptInjector.php:53-53
Timestamp: 2024-11-27T01:38:28.626Z
Learning: In this project, it's acceptable that files in the 'qualifier' directory are recreated on each deploy, so updating the 'QUALIFIER' constant in `src/ScriptInjector.php` without modifying dependent code is acceptable.
🔇 Additional comments (67)
phpstan.neon (2)
9-11: Verify the necessity of ignoring duplicate type aliases
The addition of the typeAlias.duplicate ignore rule might indicate underlying issues. Could you clarify:
- Are there legitimate cases of duplicate type aliases in the codebase?
- Is this related to cross-version PHP compatibility?
Let's check for duplicate type aliases in the codebase:
✅ Verification successful
The previous commands might have failed because:
- The AST pattern might not be compatible with PHP
- The type declarations might be using different syntax
- We need to look for broader context around type usage
Let's try with ripgrep to get a better understanding of how types are used in the codebase.
The typeAlias.duplicate ignore rule is necessary for psalm-type imports
The codebase shows a legitimate use of type aliases through Psalm annotations, specifically:
- A base type
ScriptDiris defined inCompileInjector.php - This type is imported multiple times using
@psalm-import-type ScriptDir from CompileInjectoracross different files:ScriptInjectorModule.phpScriptInjector.phpInjectorFactory.phpInjectionPoint.phpDiCompiler.php- And several other core classes
The ignore rule is necessary because these legitimate type imports are being flagged as duplicates by PHPStan, even though they're valid uses of Psalm's type system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential duplicate type alias declarations
# to understand why this ignore rule is necessary
# Search for type alias declarations
ast-grep --pattern 'type $_ = $_'
# Also check for class-level type declarations that might conflict
rg -g '*.php' '^\s*(private|protected|public)\s+type\s+\w+'
Length of output: 97
Script:
#!/bin/bash
# Let's try a different approach to find type aliases and type declarations
# Search for type keyword usage in PHP files
rg -g '*.php' -B 2 -A 2 '\btype\b'
# Also search for potential type-related PHPDoc annotations
rg -g '*.php' -B 1 -A 1 '@(type|typedef|template)'
Length of output: 5272
2-2: Verify the rationale for choosing level 8
The change from max to level 8 makes the static analysis more predictable but might miss some strict checks. Level 8 is quite strict but not maximum (level 9). Could you clarify the reasoning behind choosing this specific level?
Let's check what additional rules we might be missing:
psalm.xml (1)
5-5: Verify the new schema location path
The schema location has been moved to a tools-specific vendor directory. While this is a good practice for separating dev tools, we should verify:
- The new path exists and is correctly set up
- The CI workflow is updated to handle this new directory structure
✅ Verification successful
Let me gather more information about the tool configuration setup to understand if this is a valid change.
Let me check the CI workflow files to verify if they're properly configured to handle the vendor-bin tools setup.
Let me check the actual CI workflow content to verify the tools setup.
Schema location change is valid and properly configured
The change to use vendor-bin/tools is valid and well-structured because:
- The project uses
bamarni/composer-bin-pluginfor managing dev tools - Both Psalm and PHPCS configurations consistently use the new
vendor-bin/toolspath - The static analysis workflow is configured through a reusable workflow that handles the tools setup
The schema location change aligns with the project's dependency management strategy and is consistent across tool configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new schema location and related tool configurations
# Check if the new schema file exists
fd -t f "config.xsd" vendor-bin/tools/vendor/vimeo/psalm/
# Check if other tool configurations use similar paths
fd -t f -e xml -e yaml -e json . | xargs rg "vendor-bin/tools"
Length of output: 470
Script:
#!/bin/bash
# Check for composer configuration files that might define the tool dependencies
fd -t f "composer.json" .
# Look for any bamarni/composer-bin-plugin configuration
rg -A 5 "bamarni/composer-bin-plugin" .
# Check for any CI workflow files that might use these tools
fd -t f "^.*\.(ya?ml|xml)$" .github/workflows/
Length of output: 1332
Script:
#!/bin/bash
# Check CI workflow files for vendor-bin tools setup
rg -A 10 "vendor-bin|composer-bin-plugin|psalm|phpcs" .github/workflows/
# Check if there are any other XML configurations that might need similar updates
fd -t f -e xml . | grep -v "phpcs.xml\|psalm.xml" | xargs cat
Length of output: 8912
Script:
#!/bin/bash
# Check the static analysis workflow specifically
cat .github/workflows/static-analysis.yml
# Check if there are any composer scripts or configuration for tools
rg -A 5 "tools" composer.json
Length of output: 284
src/CompileNullObject.php (1)
13-14: LGTM: Type import annotation added correctly
The Psalm type import annotation for ScriptDir from CompileInjector is properly added, improving type safety and static analysis capabilities.
src/ScriptInjectorModule.php (2)
15-16: LGTM! Property type declaration is well-documented
The property type declaration properly uses the imported ScriptDir type and includes appropriate PHPDoc annotation.
10-13: Verify the imported ScriptDir type definition
The PHPDoc import looks good, but let's verify the source type exists.
✅ Verification successful
The ScriptDir type import is correctly defined and used
The verification confirms that ScriptDir is properly defined in CompileInjector.php as a non-empty-string type, making the import in ScriptInjectorModule.php valid and type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ScriptDir type definition in CompileInjector
# Expected: Find the ScriptDir type definition
# Search for ScriptDir type definition in CompileInjector
ast-grep --pattern 'class CompileInjector {
$$$
/** @psalm-type ScriptDir = $_ */
$$$
}'
# Backup search using ripgrep
rg -A 2 '@psalm-type\s+ScriptDir'
Length of output: 311
src/AbstractInjectorContext.php (4)
9-9: LGTM: Import statement for ScriptDir type
The import statement is correctly added to support Psalm's static analysis.
11-13: LGTM: Psalm type import annotation
The @psalm-import-type annotation correctly imports the ScriptDir type definition from CompileInjector.
16-20: LGTM: Property type documentation
The property is correctly documented with ScriptDir type and marked as readonly, which improves type safety and immutability.
22-27: LGTM: Constructor implementation follows type system requirements
The implementation correctly:
- Documents the parameter with
@param ScriptDir - Maintains
stringtype in the method signature sinceScriptDiris a Psalm type alias
This aligns with the established pattern for handling Psalm type aliases in the codebase.
docs/exmaple/ProdInjectorContext.php (1)
25-29: Consider addressing the phpstan issue instead of ignoring it
The dependency check is a good addition, but there are a few points to consider:
- The
@phpstan-ignore-linecomment suggests there might be an underlying static analysis issue that should be addressed rather than ignored. - Consider adding a docblock to document the possible
RuntimeException.
Consider updating the implementation to:
public function getCache(): CacheProvider
{
if (! class_exists(ApcuCache::class)) {
throw new \RuntimeException('doctrine/cache ^1.0 is required for ProdInjectorContext.');
}
- return new ApcuCache(); // @phpstan-ignore-line
+ /** @var CacheProvider */
+ return new ApcuCache();
}Let's verify if there are similar patterns in other files:
src/InjectorFactory.php (6)
9-9: LGTM: Import statement is correctly placed
The ScriptDir import is properly organized with other Ray\Di imports.
19-19: LGTM: Psalm type import is correctly configured
The ScriptDir type import from CompileInjector enhances static analysis capabilities.
25-25: LGTM: Docblock type hint follows project convention
The ScriptDir type hint in docblock with string parameter type follows the established project convention.
49-51: LGTM: Docblock type hint follows project convention
The ScriptDir type hint in docblock with string parameter type follows the established project convention.
55-55: LGTM: Class name typo fixed
Fixed capitalization in class name from "ScriptinjectorModule" to "ScriptInjectorModule".
59-62:
Fix parameter name casing
The parameter name has inconsistent casing ($scriptDIr). It should be $scriptDir to match the casing used throughout the codebase.
Apply this diff to fix the parameter naming:
- /** @param ScriptDir $scriptDIr */
- private static function getCompileInjector(string $scriptDIr, LazyModuleInterface $module): CompileInjector
- {
- return new CompileInjector($scriptDIr, $module);
+ /** @param ScriptDir $scriptDir */
+ private static function getCompileInjector(string $scriptDir, LazyModuleInterface $module): CompileInjector
+ {
+ return new CompileInjector($scriptDir, $module);Likely invalid or redundant comment.
src/DependencySaver.php (3)
19-21: LGTM: Type import enhances code clarity
The psalm docblock import correctly declares the source of the ScriptDir type, improving type safety and code clarity.
24-24: LGTM: Property type annotation improves type safety
The property type annotation correctly uses the imported ScriptDir type, enhancing type safety.
30-34:
Fix type mismatch in constructor signature
There's still an inconsistency between the docblock and the constructor signature:
- Docblock specifies
@param ScriptDir $scriptDir - But the constructor signature still accepts
string $scriptDir
This mismatch could lead to runtime type errors.
Apply this fix to ensure type safety:
/**
* @param ScriptDir $scriptDir
*/
- public function __construct(string $scriptDir)
+ public function __construct(ScriptDir $scriptDir)tests/CachedFactoryTest.php (2)
37-37: LGTM! Good use of the new type alias in PHPDoc
The PHPDoc correctly references the new Context type alias, improving static analysis and code documentation.
14-16: 🛠️ Refactor suggestion
Good addition of type safety, but parameter type needs alignment
The Psalm type alias improves code clarity by explicitly defining valid context values. However, the method parameter type declaration should be updated to match this type definition.
Update the method signature to use the new type:
- private function getInjector(string $context): InjectorInterface
+ private function getInjector(Context $context): InjectorInterfaceLikely invalid or redundant comment.
tests/InjectorFactoryTest.php (1)
30-33: LGTM! Variable naming improvement.
The fix for the variable name from $modue to $module improves code clarity.
Let's verify consistent variable naming across the codebase:
✅ Verification successful
Variable naming is consistent across the codebase
The search results confirm that the variable $module is consistently used throughout the codebase, and there are no remaining instances of the misspelled $modue. The fix aligns with the established naming convention used in all module-related variable assignments and instantiations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the misspelled variable
# This helps ensure the typo hasn't propagated elsewhere
# Search for potential misspellings of "module" in variable names
rg -i '\$mo[^d]ue' --type php
# Search for similar variable usage patterns to ensure consistency
ast-grep --pattern '$module = new $_Module();'
Length of output: 80
Script:
#!/bin/bash
# Let's try a broader search pattern to catch any variable assignments related to modules
# and potential misspellings in PHP files
# Search for variable assignments containing 'module' or similar spellings
rg '\$mo[a-z]{3,4}\s*=' --type php
# Look for class instantiations with Module suffix
rg 'new \w+Module\(' --type php
# Search for any variable declarations containing 'modue'
ast-grep --pattern '$modue = $_'
Length of output: 9571
src/GraphDumper.php (5)
22-24: LGTM! Type import enhances type safety
The addition of the psalm type import for ScriptDir is appropriate and aligns with the type safety improvements.
30-31: LGTM! Property type is consistent with imported type
The property type has been correctly updated to use the imported ScriptDir type.
33-35: Fix type mismatch between constructor parameter and property
The constructor parameter type (string) doesn't match the property type (ScriptDir).
43-43: Consider using array_keys() instead of suppressing unused variable warning
The unused variable warning could be avoided by refactoring the loop to use array_keys().
22-35: Verify consistent usage of ScriptDir type
Let's ensure the ScriptDir type is consistently used across the codebase.
✅ Verification successful
Let me gather more specific information about the ScriptDir type usage.
ScriptDir type is consistently used across the codebase
The verification shows that:
- ScriptDir is properly defined in CompileInjector as
non-empty-string - All 11 files that use ScriptDir properly import it using
@psalm-import-type ScriptDir from CompileInjector - All usages consistently use the ScriptDir type in property declarations and method parameters
- No instances of string type being used instead of ScriptDir were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for inconsistent ScriptDir usage
echo "Checking for ScriptDir type usage..."
rg -l "ScriptDir"
echo "\nChecking for string type parameters named scriptDir..."
ast-grep --pattern 'function __construct($$$, string $scriptDir, $$$)'
Length of output: 655
Script:
#!/bin/bash
# Check the definition of ScriptDir type in CompileInjector
echo "Checking ScriptDir type definition:"
rg -A 2 "@psalm-type\s+ScriptDir" src/CompileInjector.php
# Check how ScriptDir is used in type hints and docblocks
echo "\nChecking ScriptDir usage in type hints and docblocks:"
rg -A 1 "@psalm-var\s+ScriptDir|@var\s+ScriptDir|@param\s+ScriptDir"
# Check for import statements of ScriptDir
echo "\nChecking ScriptDir imports:"
rg "@psalm-import-type\s+ScriptDir"
Length of output: 4517
composer.json (2)
56-56: Verify impact of removing psalm.compiler.xml
The removal of the psalm configuration file (-c psalm.compiler.xml) might affect the static analysis coverage. Please ensure that:
- All previously checked paths are still being analyzed
- The error level settings are appropriate
- Any custom rules from the old configuration are still enforced
Additionally, changing --show-info=true to --show-info=false will reduce the verbosity of the analysis output. Consider if this reduced visibility aligns with the project's quality assurance needs.
Line range hint 15-15: Update PHP version constraint for PHP 8.3/8.4 compatibility
The current PHP version constraint "php": "^7.2 || ^8.0" should be updated to explicitly indicate support for PHP 8.3 and 8.4, which are the versions being tested in CI. Consider updating to something like "php": "^7.2 || ^8.0 || ^8.3 || ^8.4" or more specifically constraining the supported versions.
tests/ContextInjectorTest.php (1)
55-55: LGTM! Good simplification of the test code.
The removal of explicit type assertions is a good practice here since:
- The code is using strict types
- The object's behavior is implicitly verified through property access
- This aligns well with modern PHP testing practices, especially for PHP 8.3+ where the type system is more robust
src/OnDemandCompiler.php (4)
11-11: LGTM! Type imports and definitions enhance type safety.
The addition of the ScriptDir import and psalm type definitions improves type safety and static analysis capabilities.
Also applies to: 26-29
32-32: Property type change aligns with project conventions.
The change of $scriptDir property type to ScriptDir while maintaining the string parameter type in the constructor is consistent with the project's established pattern, as confirmed by similar implementations in InjectionPoint, InjectorFactory, and CachedInjectorFactory.
44-46: LGTM! Constructor signature follows project conventions.
The combination of @param ScriptDir docblock with string parameter type follows the project's established pattern, as confirmed by similar implementations across the codebase.
75-77: Type improvements enhance code clarity and safety.
The changes improve type safety through:
- Explicit
Pointcutstype annotations - Consistent return type in
loadPointcuts() - Clear empty array return instead of false
However, let's verify the error reporting manipulation:
Also applies to: 87-89, 93-93, 99-99
✅ Verification successful
Let me gather more context about the error reporting changes by checking the full implementation.
Let me try a different approach to check the implementation.
Error reporting manipulation is consistent with codebase patterns
The error reporting changes in loadPointcuts() follow the established pattern in the codebase:
- Used consistently with
unserialize()operations in bothScriptInjectorandOnDemandCompiler - Temporarily suppresses E_NOTICE during deserialization
- Properly restores the previous error reporting level
- Includes type safety improvements with
@varannotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error reporting patterns in the codebase
rg "error_reporting\(.*E_NOTICE" -A 2
Length of output: 528
Script:
#!/bin/bash
# Check the full implementation around error reporting in OnDemandCompiler
ast-grep --pattern 'function loadPointcuts() {
$$$
}'
Length of output: 57
Script:
#!/bin/bash
# Search for loadPointcuts method with context
rg "loadPointcuts" -A 10 -B 2
Length of output: 1660
src/CachedInjectorFactory.php (2)
9-9: LGTM! Import and type definition changes look good.
The ScriptDir import and psalm type import are correctly implemented, enhancing type safety.
Also applies to: 17-19
28-28: Skipping review of ScriptDir parameter type changes.
Based on previous feedback from @koriym, the scriptDir parameter should remain typed as string in the method signatures, even with the ScriptDir type in docblocks.
Also applies to: 61-61, 75-75
src/InjectionPoint.php (3)
9-9: LGTM!
The import statement for ScriptDir is correctly placed and necessary for the type usage.
23-25: LGTM!
The Psalm docblock correctly imports the ScriptDir type, enhancing static analysis capabilities.
Line range hint 31-40: Type handling looks good!
Based on previous discussions and established patterns in this codebase, it's intentional and acceptable to have:
- The property
$scriptDirtyped asScriptDir - The constructor parameter typed as
stringwith@param ScriptDirdocblock
This approach is consistently used across the project.
src/Normalizer.php (2)
86-86: LGTM! Good type assertion.
The added type assertion helps static analyzers understand the array structure after the type check, improving type safety. This is a good practice and aligns with the parameter type hint.
79-79: Verify the array key types in the codebase.
The parameter type has been made more specific to require integer keys (array<int, mixed>). While this aligns with the implementation in arrayValue(), we should verify that no non-integer keys are being passed to this method.
✅ Verification successful
The array type hint change is safe and accurate
The code analysis reveals that:
getValueNodeNonAtomicis only called internally bygetValueNode- The
arrayValuemethod implementation enforces integer keys through its logic:- It uses a numeric
$lastKeycounter - It expects consecutive numeric keys (++$lastKey === $itemKey)
- All array items are added sequentially using
$items[]
- It uses a numeric
- No string keys or associative arrays are used in the implementation
The type hint change from array<mixed> to array<int, mixed> accurately reflects the actual implementation and usage patterns in the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for array values passed to getValueNode/getValueNodeNonAtomic
# to ensure they only use integer keys
# Look for array creation or manipulation before calls to these methods
ast-grep --pattern 'function $FUNC($_) {
$$$
$this->getValueNode($ARR)
$$$
}'
ast-grep --pattern 'function $FUNC($_) {
$$$
$this->getValueNodeNonAtomic($ARR)
$$$
}'
Length of output: 180
Script:
#!/bin/bash
# Let's try a different approach to find the usage patterns
# First, let's see the implementation of getValueNodeNonAtomic
rg -U "private function getValueNodeNonAtomic" -A 20
# Then, let's find all calls to getValueNode
rg "->getValueNode\(" -B 2 -A 2
# And look for array creation/manipulation patterns
ast-grep --pattern '$var = array($$$)'
ast-grep --pattern '$var = [$$$]'
Length of output: 1470
Script:
#!/bin/bash
# Let's fix the rg command and try a different approach
# Find calls to getValueNode with proper escaping
rg "\->getValueNode\(" -B 2 -A 2
# Look at the implementation of arrayValue method
rg -U "private function arrayValue" -A 15
# Look for array assignments or array parameters in the class
rg "array\(" src/Normalizer.php
rg "\[" src/Normalizer.php
Length of output: 1389
tests/CompileInjectorTest.php (2)
Line range hint 55-59: LGTM! Method name corrected from testInjectopnPoint to testInjectionPoint
The typographical error in the method name has been fixed, improving code clarity.
Line range hint 97-104: LGTM! Method name corrected from testCompileFaillureLog to testCompileFailureLog
The typographical error in the method name has been fixed, improving code clarity.
src/DiCompiler.php (4)
28-30: LGTM! Good type import.
The psalm type import improves static analysis capabilities and type safety.
33-33: LGTM! Property type properly updated.
The property type annotation correctly reflects the ScriptDir type, maintaining consistency with the type import.
51-58: Acknowledging previous discussion on type handling.
Based on the previous discussion referenced in PR#114, the current type handling implementation will be maintained as is.
Line range hint 28-58: Verify consistent ScriptDir type usage across the codebase.
Let's ensure the ScriptDir type is consistently used across all related files.
✅ Verification successful
ScriptDir type usage is consistent across the codebase
The verification shows that ScriptDir type is consistently used across all relevant files:
- All files properly import the ScriptDir type from CompileInjector using
@psalm-import-type - The type is correctly used in property declarations with
@var ScriptDir - Parameter type hints consistently use
@param ScriptDir - The base type definition in CompileInjector.php defines it as
@psalm-type ScriptDir = non-empty-string - The ScriptDir annotation class is properly referenced where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent ScriptDir type usage across the codebase
# Search for ScriptDir type usage
echo "Checking ScriptDir type usage patterns:"
rg --type php "ScriptDir" -A 2 -B 2
# Search for potential string type hints that should be ScriptDir
echo "\nChecking for potential missed ScriptDir conversions:"
ast-grep --pattern 'string $scriptDir'
Length of output: 14106
tests/DependencyCompilerTest.php (1)
10-10: LGTM! Clean import additions.
The new imports are well-organized and properly support the enhanced type checking in the test method.
Also applies to: 14-15
tests/MultiBindingTest.php (4)
45-47: LGTM! Enhanced type safety with generic type.
The addition of the Map<FakeEngineInterface> return type improves type safety and IDE support.
58-59: LGTM! Consistent type hints across dependent tests.
The parameter type hints Map<FakeEngineInterface> are consistently applied across all dependent test methods, creating a proper type-safe chain of test dependencies.
Also applies to: 69-70, 81-82, 92-93, 103-104
122-122: LGTM! Fixed method name typo.
The method name has been corrected from testMultipileModule to testMultipleModule, improving code clarity.
45-47: Verify PHP version compatibility for generic types.
Since the PR updates PHP versions in CI, let's ensure the generic type hints are compatible with all supported PHP versions.
Also applies to: 58-59, 69-70, 81-82, 92-93, 103-104
✅ Verification successful
Let me check the PHP version requirements in the files that were found to have PHP version checks.
Generic type hints are compatible with supported PHP versions
The codebase already enforces PHP version compatibility through test requirements:
MultiBindingTest.phprequires PHP 8.0AssistedTest.phprequires PHP 8.0ContextBindingTest.phprequires PHP >= 7.4
Since the generic type hints are used in PHPDoc annotations (not native PHP types), they are compatible with all supported PHP versions (^7.2 || ^8.0) as they are purely for static analysis tools and don't affect runtime behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check PHP version requirements and generic type support
# Check composer.json for PHP version constraints
if [ -f "composer.json" ]; then
echo "PHP version requirements from composer.json:"
cat composer.json | jq '.require.php'
fi
# Check if there are any PHP version checks in the codebase
rg -l "requires PHP" .
Length of output: 288
Script:
#!/bin/bash
# Check the content of files with PHP version requirements
echo "=== Content of ./tests/MultiBindingTest.php ==="
rg "requires PHP|@since" ./tests/MultiBindingTest.php
echo -e "\n=== Content of ./tests/ContextBindingTest.php ==="
rg "requires PHP|@since" ./tests/ContextBindingTest.php
echo -e "\n=== Content of ./tests/AssistedTest.php ==="
rg "requires PHP|@since" ./tests/AssistedTest.php
# Check if generics syntax is used elsewhere in tests
echo -e "\n=== Other uses of generics in tests ==="
rg "<.*>" ./tests/
Length of output: 2110
src/DependencyCode.php (3)
91-93: LGTM! Spelling correction properly implemented.
The method signature and implementation correctly fix the spelling from setQaulifier to setQualifier while maintaining proper type hints and return type declaration.
126-129: LGTM! Consistent qualifier cleanup pattern.
The code maintains a consistent pattern for handling the qualifier across both getDependencyCode and getProviderCode methods:
- Captures the current qualifier
- Resets the internal state
- Passes the captured qualifier to the Code constructor
This approach ensures proper cleanup and prevents qualifier leakage between subsequent calls.
Also applies to: 155-158
91-91: Verify complete rename across the codebase.
Let's ensure all occurrences of the old method name have been updated.
✅ Verification successful
Rename verification completed - no issues found
The codebase search confirms that:
- No instances of the old spelling "setQaulifier" remain
- The new spelling "setQualifier" is used consistently across the codebase
- All usages are properly aligned with the method signature in DependencyCode.php
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old spelling
echo "Checking for old method name..."
rg -i "setQaulifier"
# Search for all instances of the new spelling to verify consistent usage
echo "Verifying new method name usage..."
rg -i "setQualifier"
Length of output: 569
src/CompileInjector.php (3)
21-24: Well-defined type declarations enhance type safety.
The addition of psalm-type declarations for ScriptDir and Ip improves type safety and code clarity. The types are appropriately defined and used consistently throughout the class.
30-30: Property types properly aligned with type declarations.
The updates to property types ($scriptDir and $ip) correctly utilize the newly defined types, enhancing type safety and maintainability.
Also applies to: 38-38
97-97: Type assertions properly maintain type safety.
The added type assertions for $instance as object help maintain type safety throughout the code execution path.
Also applies to: 156-156
src/ScriptInjector.php (5)
41-44: LGTM: Type imports enhance static analysis
The addition of @psalm-import-type annotations for ScriptDir and Ip improves type safety and static analysis capabilities.
53-53: LGTM: Improved type safety and spelling
Two good improvements here:
- Fixed spelling in QUALIFIER constant from 'qualifer' to 'qualifier'
- Strengthened type safety by using
non-empty-stringinstead ofstring
Also applies to: 55-55
93-94: LGTM: Constructor parameter type strengthened
The parameter type change from string to ScriptDir improves type safety by ensuring the correct type is passed.
119-121: LGTM: Injection point type strengthened
The parameter type change from array to Ip improves type safety by ensuring the correct injection point structure.
171-171: LGTM: Improved static analysis suppression
The addition of @phpstan-ignore-line alongside PHPMD suppression maintains consistency across static analysis tools.
tests/ScriptInjectorTest.php (3)
172-172: LGTM! Consistent exception message format.
The hyphenation in the exception message and class name is now consistent.
Also applies to: 174-174
186-186: LGTM! Using PHPUnit's assertInstanceOf.
Good use of PHPUnit's dedicated type assertion method.
213-213: 🛠️ Refactor suggestion
Use assertInstanceOf instead of assertTrue with instanceof.
Replace the assertTrue check with PHPUnit's dedicated assertInstanceOf method to improve test clarity and remove the need for the PHPStan ignore annotation.
- $this->assertTrue($car instanceof FakeCar); // @phpstan-ignore-line
+ $this->assertInstanceOf(FakeCar::class, $car);Upgraded doctrine/coding-standard to ^12.0 in composer.json for improved coding standards. Simplified doc comments throughout the codebase for consistency and readability.
koriym
changed the title
Added comments to clarify the purpose of assertions and ignored lines in `testDomainException`. This ensures that PHPStan does not produce false-positive errors and maintains code clarity for future maintenance.
Refactor `ScriptInjector` initialization logic by introducing an `init` method, improving clarity and reusability. Added assertions to check file existence before requiring instance files, enhancing robustness and error handling. Updated `__wakeup` method to use the new `init` method.
Changed the type hint of the $ip variable and the injection point parameter from an array to Ip. This improves type safety and code clarity.
Introduce stricter type definitions and refactor code to use more explicit types. Added `init` method for initialization logic, ensuring better separation of concerns and enhancing readability. Replaced direct `require` calls with assertions to improve debugging and error handling.
This commit introduces new type annotations for injection points and injectors using callable types. These changes enhance type safety and improve code clarity, aiding both static analysis tools and developers in understanding the expected types.
Eliminated redundant unset statements from multiple test files to improve code clarity and maintainability. Adjusted a script command in composer.json to reflect these changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 1.x #114 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 215 217 +2
===========================================
Files 28 28
Lines 739 748 +9
===========================================
+ Hits 739 748 +9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/DependencyCompilerTest.php (1)
146-150: Consider improving test case documentation and type safety.While the added comment explains why the exception is thrown, the test could be more explicit:
- The assertion comments indicate workarounds for static analysis, suggesting potential design issues
- The test's purpose could be better documented
Consider this improvement:
- assert(class_exists(FakeInvalidDependency::class)); // to suppress phpstan false-positive - $fake = new FakeInvalidDependency(); - assert($fake instanceof DependencyInterface); // @phpstan-ignore-line - // FakeInvalidDependency is not a valid dependency, so the DomainException is thrown. - (new DependencyCode(new Container()))->getCode($fake); + /** @var DependencyInterface $invalidDependency */ + $invalidDependency = new FakeInvalidDependency(); + + // Test that DependencyCode throws DomainException when processing + // an invalid dependency that doesn't properly implement the interface + (new DependencyCode(new Container()))->getCode($invalidDependency);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/DependencyCompilerTest.php(9 hunks)
🧰 Additional context used
🪛 GitHub Check: sa / PHPStan
tests/DependencyCompilerTest.php
[failure] 171-171:
Method Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() has no return type specified.
[failure] 171-171:
Method Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() has parameter $content with no type specified.
🪛 GitHub Check: cs / Coding Standards
tests/DependencyCompilerTest.php
[failure] 171-171:
Method \Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() does not have parameter type hint nor @param annotation for its parameter $content.
[failure] 171-171:
Method \Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() does not have return type hint nor @return annotation for its return value.
🔇 Additional comments (2)
tests/DependencyCompilerTest.php (2)
10-10: LGTM: Import statements are properly organized.
The new imports are correctly placed and follow PSR-4 standards.
Also applies to: 14-15
36-36: LGTM: Consistent line ending normalization.
The addition of normalizeLineEndings calls across all test methods improves test reliability across different platforms.
Also applies to: 48-48, 61-62, 91-91, 93-93, 111-111, 124-124, 138-139, 168-168
Refactor test assertions to use a helper method for normalizing line endings. This ensures consistency across different operating systems and prevents test failures due to line ending discrepancies.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/DependencyCompilerTest.php (1)
36-39: LGTM: Excellent improvement in test reliability.The consistent use of normalizeLineEndings across all test assertions is a great improvement that makes the tests more reliable across different platforms (Windows, Unix, Mac).
Consider extracting this pattern into a test trait if you have other test classes that need to compare multi-line strings. This would promote reusability and maintain consistency across your test suite.
Also applies to: 51-54, 66-69, 94-101, 119-122, 135-138, 151-154, 183-186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/DependencyCompilerTest.php(9 hunks)
🧰 Additional context used
🪛 GitHub Check: cs / Coding Standards
tests/DependencyCompilerTest.php
[failure] 189-189:
Method \Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() does not have parameter type hint nor @param annotation for its parameter $content.
🪛 GitHub Check: sa / PHPStan
tests/DependencyCompilerTest.php
[failure] 189-189:
Method Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() has parameter $content with no type specified.
🔇 Additional comments (3)
tests/DependencyCompilerTest.php (3)
10-10: LGTM: Import statements are correctly placed.
The new imports support the type checking and assertions in the testDomainException method.
Also applies to: 14-15
189-192: 🛠️ Refactor suggestion
Add type hint to comply with coding standards.
The method needs a parameter type hint to comply with the project's coding standards.
Apply this fix:
- private function normalizeLineEndings($content): string
+ private function normalizeLineEndings(string $content): string
{
// Convert Windows (CRLF: \r\n) and old Mac (CR: \r) to Unix (LF: \n)
return str_replace(["\r\n", "\r"], "\n", $content);
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 189-189:
Method \Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() does not have parameter type hint nor @param annotation for its parameter $content.
🪛 GitHub Check: sa / PHPStan
[failure] 189-189:
Method Ray\Compiler\DependencyCompilerTest::normalizeLineEndings() has parameter $content with no type specified.
161-165: 🛠️ Refactor suggestion
Consider improving type safety in the test.
While the added assertions and comment improve clarity, there are some concerns:
- The
@phpstan-ignore-linesuggests a type system bypass - The runtime assertion of
DependencyInterfacemight indicate a design issue
Consider these improvements:
- assert(class_exists(FakeInvalidDependency::class)); // to suppress phpstan false-positive
- $fake = new FakeInvalidDependency();
- assert($fake instanceof DependencyInterface); // @phpstan-ignore-line
- // FakeInvalidDependency is not a valid dependency, so the DomainException is thrown.
- (new DependencyCode(new Container()))->getCode($fake);
+ /** @var DependencyInterface $invalidDependency */
+ $invalidDependency = new FakeInvalidDependency();
+ // This should throw DomainException because FakeInvalidDependency
+ // doesn't properly implement the required dependency contract
+ (new DependencyCode(new Container()))->getCode($invalidDependency);Likely invalid or redundant comment.
Updated tests to consistently normalize line endings before performing assertions. This improves code readability and ensures uniform assertion practices across all test cases.
| * @param LazyModuleInterface $lazyModule callable variable which return AbstractModule instance | ||
| * | ||
| * @psalm-suppress UnresolvableInclude | ||
| */ | ||
| public function __construct($scriptDir, LazyModuleInterface $lazyModule) | ||
| public function __construct(string $scriptDir, LazyModuleInterface $lazyModule) |
There was a problem hiding this comment.
ここ string が追加されて既存のものとシグネチャ変わってしまいますが、大丈夫ですかね ...?
There was a problem hiding this comment.
@NaokiTsuchiya ということでmergeします！
Add PHP 8.3 to old stable versions and set current stable to 8.4. This ensures the CI process tests against the most recent PHP versions for better compatibility and bug detection.
Summary by CodeRabbit
New Features
ScriptDirtype across multiple classes.Bug Fixes
Documentation
Chores
composer.jsonfile.