Handle missing dependencies in setter methods with @Inject(optional=true) #292
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SetterMethod
participant Visitor
Client->>SetterMethod: call accept(visitor)
SetterMethod->>Visitor: visitSetterMethod()
alt Unbound Exception
Visitor-->>SetterMethod: throws Unbound
SetterMethod->>SetterMethod: check isOptional
alt isOptional is true
SetterMethod-->>Client: return (no error)
else isOptional is false
SetterMethod-->>Client: throw Unbound
end
else No Exception
SetterMethod-->>Client: proceed normally
end
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (1)
src/di/SetterMethod.php (1)
70-80: Consider refactoring duplicate error handling logic.The error handling pattern is duplicated between
accept()and__invoke()methods. Consider extracting this into a private method to maintain DRY principles.Here's a suggested refactoring:
+ private function handleUnboundException(Unbound $e): void + { + if ($this->isOptional) { + return; + } + + throw $e; + } public function __invoke($instance, Container $container): void { try { $parameters = $this->arguments->inject($container); } catch (Unbound $e) { - if ($this->isOptional) { - return; - } - - throw $e; + $this->handleUnboundException($e); + return; } // ... rest of the method } public function accept(VisitorInterface $visitor) { try { $visitor->visitSetterMethod($this->method, $this->arguments); } catch (Unbound $e) { - if ($this->isOptional) { - return; - } - - throw $e; + $this->handleUnboundException($e); + return; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/di/SetterMethod.php(1 hunks)
🔇 Additional comments (2)
src/di/SetterMethod.php (2)
70-80: LGTM! The error handling implementation aligns with the PR objectives.
The try-catch block correctly implements the intended behavior:
- Returns silently for @Inject(optional=true) when dependency is missing
- Throws exception for @Inject(optional=false) when dependency is missing
The code comments clearly document the behavior in both scenarios.
70-80: Verify error handling consistency across the codebase.
Let's ensure this error handling pattern is consistently applied across other dependency injection scenarios in the codebase.
✅ Verification successful
Based on the results, I can now generate the final response:
Error handling for Unbound exceptions is consistently implemented across the codebase
The verification shows that error handling for Unbound exceptions follows consistent patterns throughout the codebase:
- In
SetterMethod.php: Optional dependencies are handled by checkingisOptional - In
Arguments.php: Similar pattern usingisDefaultAvailable()for default values - Demo files only contain example error handling for demonstration purposes
The implementation in the reviewed code aligns with the codebase's established patterns for handling unbound dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Unbound exception handling patterns
rg -A 5 "catch \(.*Unbound" --type php
# Search for other optional injection patterns
ast-grep --pattern 'if ($this->isOptional) {
$$$
}'
Length of output: 2092
Removed unnecessary type suppression comments and refined type annotations to enhance code clarity across multiple classes. Updated several development dependencies in the composer.lock to their latest versions, ensuring compatibility and consistency. This update primarily affects internal documentation and tooling without altering functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/di/AspectBind.php (1)
Based on the results from our searches, I can now provide the final response:
Add type structure tests for AspectBind::inject method
The codebase lacks specific test coverage for verifying the return type structure of
AspectBind::inject. While there are several interceptor implementations and basic visitor tests, none explicitly verify that:
- The returned array has string keys (method names)
- The values are lists of
MethodInterceptorinstances- The overall structure matches
array<string, list<MethodInterceptor>>Suggested test locations:
- Add test cases in
tests/di/VisitorTest.phpor create a newAspectBindTest.php🔗 Analysis chain
Line range hint
25-41: Consider adding type structure verification tests.While the implementation correctly produces a list structure, it would be valuable to add tests that verify the return type structure matches the new stricter type declaration.
Let's check for existing tests that verify the return structure:
Let me gather more information about the test coverage for AspectBind.
Let me check for specific test cases that verify the AspectBind's inject method and its return type structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for test cases that verify the structure of AspectBind::inject return value # Search for test files related to AspectBind fd -e php AspectBind --exec rg -l "class.*Test.*AspectBind" # Search for specific test assertions about the return structure rg -A 3 "inject.*Container" "tests/"Length of output: 342
Script:
#!/bin/bash # Search for test files containing AspectBind tests with broader context rg -l "AspectBind" tests/ # Look for test assertions related to method interception or bindings rg -A 5 "MethodInterceptor|getBindings|AspectBind" tests/ # Check if there are any test files that might have been renamed fd -e php test --exec rg -l "AspectBind"Length of output: 9192
Script:
#!/bin/bash # Search for test cases specifically related to AspectBind's inject method rg -B 5 -A 10 "AspectBind.*inject" tests/ # Look for test assertions that verify array structure or types rg -B 2 -A 5 "assertInstanceOf.*MethodInterceptor|assert.*array.*MethodInterceptor" tests/ # Check if there are any test files in a different directory fd -t f -e php -e phpt . --exec rg -l "AspectBind.*test"Length of output: 195
src/di/ContainerFactory.php (1)
Line range hint
17-26: Consider adding runtime validation for non-empty string.While the docblock specifies
non-empty-string, it would be safer to add runtime validation for the$classDirparameter.public function __invoke($module, string $classDir): Container { + if ($classDir === '') { + throw new InvalidArgumentException('Class directory path cannot be empty'); + } $oneModule = $this->getModule($module); // install built-in module $appModule = (new BuiltinModule())($oneModule);src/di/Grapher.php (1)
Line range hint
29-35: Add runtime validation for non-empty class directory.While the
@param non-empty-stringannotation provides static analysis safety, the runtime type hint is stillstring. Consider adding a runtime check to ensure the contract is maintained.public function __construct(AbstractModule $module, string $classDir) { + if ($classDir === '') { + throw new \InvalidArgumentException('Class directory path cannot be empty'); + } $module->install(new AssistedModule()); $this->container = $module->getContainer(); $this->classDir = $classDir;src/di/Injector.php (1)
Line range hint
67-67: Consider adding runtime class validationWhile the
@psalm-var class-stringannotation helps with static analysis, consider adding runtime validation to ensure$interfaceis a valid class name.Consider adding validation:
/** @psalm-var class-string $interface */ +if (!class_exists($interface) && !interface_exists($interface)) { + throw new \InvalidArgumentException(sprintf('"%s" is not a valid class or interface name', $interface)); +} $this->bind($interface);src/di/AssistedInjectInterceptor.php (1)
66-67: Consider adding type parameter to invoke method for consistency.For consistency, consider adding the same type parameter to the public
invokemethod signature:- public function invoke(MethodInvocation $invocation) + public function invoke(MethodInvocation<object> $invocation)
📜 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 (9)
src/di/AspectBind.php(1 hunks)src/di/AssistedInjectInterceptor.php(1 hunks)src/di/ContainerFactory.php(1 hunks)src/di/Dependency.php(1 hunks)src/di/Grapher.php(1 hunks)src/di/Injector.php(2 hunks)src/di/MethodInvocationProvider.php(1 hunks)src/di/MultiBinding/LazyTo.php(1 hunks)src/di/SpyCompiler.php(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/di/Dependency.php
- src/di/MultiBinding/LazyTo.php
- src/di/SpyCompiler.php
🔇 Additional comments (11)
src/di/MethodInvocationProvider.php (4)
15-15: LGTM! Type safety improvement
The addition of the generic type parameter <object> enhances type safety by explicitly declaring that this MethodInvocation works with object types.
18-20: LGTM! Consistent type documentation
The parameter type documentation aligns with the property type, maintaining type consistency throughout the class.
Line range hint 1-36: Consider similar type improvements in related classes
The type safety improvements in this class are excellent. Consider applying similar generic type parameters to other classes that work with MethodInvocation to maintain consistency across the codebase.
Let's check for other classes that might benefit from similar improvements:
26-28: LGTM! Type-safe return value with proper error handling
The return type documentation ensures type consistency with the property, and the implementation properly handles the null case with an appropriate exception.
Let's verify the exception handling is consistent across the codebase:
✅ Verification successful
Exception handling is properly implemented and tested
The verification shows that:
- The exception is properly defined as a subclass of
Unbound - It's used consistently in
MethodInvocationProvider - Test coverage exists in both PHP 8 (
tests-php8/AssistedInjectTest.php) and earlier versions (tests/di/AssistedTest.php) - Tests explicitly verify the exception is thrown when method invocation is not available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of MethodInvocationNotAvailable exception
# and verify it's properly caught where needed
# Check for other places where this exception is thrown
rg "throw new MethodInvocationNotAvailable" -A 2
# Check for exception handling
rg "catch.*MethodInvocationNotAvailable" -A 2
Length of output: 271
Script:
#!/bin/bash
# Let's verify if the exception class exists and check its usage pattern
# First, find the exception class
fd "MethodInvocationNotAvailable.php"
# Then check where this exception is used (imported/referenced)
rg "use.*MethodInvocationNotAvailable"
# Also check for any try-catch blocks in files that might handle this type of exception
rg "try\s*{" -A 10 src/di/
Length of output: 484
Script:
#!/bin/bash
# Let's check the test files to understand how this exception is handled
rg -A 5 "MethodInvocationNotAvailable" tests-php8/AssistedInjectTest.php tests/di/AssistedTest.php
# Also check the exception class implementation
cat src/di/Exception/MethodInvocationNotAvailable.php
# Check for try blocks with fixed regex
rg "try {" -A 5 src/di/
Length of output: 1603
src/di/AspectBind.php (1)
25-25: LGTM! Type refinement accurately reflects the data structure.
The change from array<MethodInterceptor> to list<MethodInterceptor> better represents the sequential nature of the interceptors array, making the contract more precise. This aligns well with the implementation which uses array append operations.
src/di/ContainerFactory.php (2)
14-15: LGTM! Well-documented parameter constraints.
The docblock clearly specifies the type constraints for both parameters, particularly the non-empty-string requirement for $classDir which aligns with the type safety improvements across the codebase.
17-17: Verify all callers are updated with the new required parameter.
The method signature change requires all callers to provide a $classDir parameter. Let's verify this change doesn't break existing code.
✅ Verification successful
All callers have been updated with the new required parameter
The verification shows that all callers of ContainerFactory::__invoke() are already updated with the new $classDir parameter:
tests/di/VisitorTest.php:(new ContainerFactory())(new FakeCarModule(), __DIR__ . '/tmp')src/di/Injector.php:(new ContainerFactory())($module, $this->classDir)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to ContainerFactory::__invoke to ensure they're updated
# with the new required parameter
# Search for ContainerFactory instantiation and usage
rg -l "ContainerFactory" | while read -r file; do
if [[ "$file" != "src/di/ContainerFactory.php" ]]; then
echo "=== $file ==="
rg -A 2 "ContainerFactory" "$file"
fi
done
# Search for potential test files that might need updates
fd -e php -e phpt | rg -l "ContainerFactory"
Length of output: 1063
src/di/Grapher.php (2)
24-25: LGTM! Documentation matches the type constraint.
The PHPDoc annotation correctly reflects the non-empty-string type requirement for the $classDir parameter.
17-17: LGTM! Verify usage of non-empty-string type.
The stricter type annotation improves type safety by ensuring the class directory path is never empty. This is crucial as the property is used for class autoloading.
Let's verify this type is consistently used across the codebase:
✅ Verification successful
Let me gather more context about the usage of $classDir to ensure type consistency:
Let me try a different approach to check the usage:
Let me check one more thing to verify the property visibility and usage:
Type annotation change is consistent across the codebase
Based on the search results, the non-empty-string type annotation is consistently used across all related files (Grapher.php, Injector.php, and ContainerFactory.php) where $classDir is used. The constructor properly assigns the value, and the type constraint ensures the directory path can never be empty, which is crucial for class autoloading functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of non-empty-string type for classDir
# Look for similar class directory parameters in other files
# Search for class directory related parameters
rg -t php "(?i)(classdir|class_dir|directory)\s*[:=].*string"
# Search for non-empty-string usage
rg -t php "non-empty-string"
Length of output: 444
Script:
#!/bin/bash
# Search for $classDir usage in these files
ast-grep --pattern '$classDir'
# Also check constructor parameters and property declarations
ast-grep --pattern 'function __construct($$$classDir$$$)'
Length of output: 92
Script:
#!/bin/bash
# Search for classDir parameter in constructors and its usage
rg -t php "function\s+__construct.*classDir" -A 5
# Check property declarations
rg -t php "protected\s+\$classDir|private\s+\$classDir|public\s+\$classDir" -A 2
Length of output: 533
Script:
#!/bin/bash
# Search for all $classDir property declarations with any visibility
rg -t php "\s+\$classDir\s*;" -B 2
# Search for where $classDir is used
rg -t php "\$classDir" -B 1 -A 1
Length of output: 69
src/di/Injector.php (1)
21-21: LGTM: Improved type safety with non-empty-string
The change from string to non-empty-string enhances type safety by ensuring the class directory path can never be empty, which is crucial for proper class loading.
src/di/AssistedInjectInterceptor.php (1)
66-67: LGTM! Type parameter addition improves type safety.
The addition of <object> type parameter to MethodInvocation enhances type safety by explicitly indicating that the invocation operates on objects.
Add a check in the Injector class to verify that the temporary directory is writable before proceeding with the class directory initialization. Introduced a new exception, DirectoryNotWritable, to handle cases where the directory is not writable, enhancing error handling and code robustness.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/di/Exception/DirectoryNotWritable.php (1)
9-11: Add documentation and default messageWhile the implementation is correct, consider adding:
- A PHPDoc block describing when this exception is thrown
- A constructor with a meaningful default message
+/** + * Exception thrown when a directory required for dependency injection cache + * is not writable by the application. + */ class DirectoryNotWritable extends RuntimeException implements ExceptionInterface { + public function __construct( + string $message = 'The specified directory is not writable', + int $code = 0, + ?\Throwable $previous = null + ) { + parent::__construct($message, $code, $previous); + } }src/di/Injector.php (1)
Line range hint
23-41: Consider extracting directory management into a separate service.The
Injectorclass currently handles multiple responsibilities including directory management. Consider extracting the directory handling logic into a dedicated service (e.g.,ClassDirectoryManager) to:
- Improve separation of concerns
- Make directory management more testable
- Allow for different directory strategies
- Centralize directory-related error handling
This would make the
Injectorclass more focused on its primary responsibility of dependency injection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/di/Exception/DirectoryNotWritable.php(1 hunks)src/di/Injector.php(2 hunks)
🔇 Additional comments (8)
src/di/Exception/DirectoryNotWritable.php (3)
1-6: LGTM: Proper file structure and namespace declaration
The file follows PSR-4 standards with proper strict types declaration for enhanced type safety.
7-8: LGTM: Clean import statement
The import is properly used and necessary for the class implementation.
9-11: Verify exception usage patterns
Let's verify how this exception is used across the codebase to ensure consistent error handling.
✅ Verification successful
Exception usage is consistent but lacks test coverage
The DirectoryNotWritable exception is used consistently in a single location within src/di/Injector.php to handle unwritable directory scenarios during dependency injection. The implementation follows good practices:
- Extends
RuntimeExceptionand implementsExceptionInterface - Used in conjunction with proper directory permission check (
is_writable) - Clear, single responsibility for handling directory write permission failures
However, the code comment @CodeCoverageIgnore suggests this error path isn't tested, which could be improved.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of DirectoryNotWritable exception and their context
# Search for throw statements using this exception
echo "Searching for throw statements..."
rg "throw new DirectoryNotWritable" -B 2 -A 2
# Search for catch blocks handling this exception
echo "Searching for catch blocks..."
rg "catch \(DirectoryNotWritable" -B 2 -A 2
Length of output: 536
Script:
#!/bin/bash
# Let's verify if there are any other error handling patterns or related directory checks
# Search for related directory permission checks
echo "Searching for directory permission checks..."
rg "is_writable\(" -B 2 -A 2
# Search for similar exception patterns in the codebase
echo "Searching for similar RuntimeException usage..."
rg "extends RuntimeException" -B 1 -A 1
# Check if there are any tests for this exception
echo "Searching for tests related to DirectoryNotWritable..."
fd "test" --type f --exec rg "DirectoryNotWritable" -B 2 -A 2
Length of output: 1009
src/di/Injector.php (5)
9-9: LGTM: Import statement is correctly placed.
The import for DirectoryNotWritable exception is properly organized with other exception imports.
15-15: LGTM: Function import is correctly placed.
The import for is_writable function is properly organized with other function imports.
23-24: LGTM: Type safety improvement for $classDir property.
The property type has been strengthened to non-empty-string, ensuring the directory path is never empty at compile time.
Line range hint 30-32: LGTM: Improved type specification in docblock.
The parameter type specification is now more precise, clearly indicating valid input types for $module.
35-41: LGTM: Implemented directory writability check with specific exception.
The implementation follows the past review suggestion but improves upon it by:
- Using a specific
DirectoryNotWritableexception instead of genericRuntimeException - Adding proper type assertion for
$classDir
Add try-catch block to manage missing dependencies during method injection. When a method is annotated with @Inject(optional=true), the code now returns gracefully if no dependency is provided. For methods with @Inject(optional=false), an exception is thrown if the dependency is unbound.
Summary by CodeRabbit
New Features
Unboundexceptions based on theisOptionalproperty.Bug Fixes