Prioritize legacy container #2428
Conversation
WalkthroughThe changes involve a modification to the Changes
Possibly related PRs
Poem
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
🧹 Outside diff range and nitpick comments (1)
dokan-class.php (1)
111-114: LGTM! Consider adding documentation about container priority.The change correctly implements the PR objective by prioritizing the legacy container. However, this significant behavioral change should be documented.
Consider adding a comment explaining the container priority:
public function __get( $prop ) { + // First check legacy container to prioritize overridden services if ( array_key_exists( $prop, $this->legacy_container ) ) { return $this->legacy_container[ $prop ]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
dokan-class.php(1 hunks)
🔇 Additional comments (1)
dokan-class.php (1)
111-114: Verify service override implementations.
Let's check how services are being overridden in the codebase to ensure compatibility with this change.
✅ Verification successful
Service container implementation is properly maintained
The verification shows that the service container implementation is consistent and follows the documented pattern. Key findings:
- The
get_container()method is used consistently throughout the codebase for service retrieval - Service container usage is well-documented in
docs/container.md - The
dokan_get_class_containerfilter is used only for legacy container modifications - No breaking changes are introduced by the reviewed code as it maintains the standard container access pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for service override implementations
echo "Searching for dokan_get_class_container filter usage..."
rg "add_filter\s*\(\s*['\"]dokan_get_class_container" -A 3
echo "Searching for potential service container usage..."
rg "->get_container\(\)->get\(" -A 2
Length of output: 738
Script:
#!/bin/bash
# Search for service container usage with escaped characters
echo "Searching for get_container usage..."
rg "get_container\(\)" -A 2
echo "Searching for dokan_get_class_container filter..."
rg "dokan_get_class_container" -A 2
# Search for container property access
echo "Searching for container property declarations..."
ast-grep --pattern 'protected $container;'
Length of output: 13367
|
|
||
| if ( $this->get_container()->has( $prop ) ) { | ||
| return $this->get_container()->get( $prop ); | ||
| } |
There was a problem hiding this comment.
Add missing return statement for undefined properties.
The method doesn't return anything when the property is not found in either container. This could lead to undefined behavior.
Apply this fix:
public function __get( $prop ) {
if ( array_key_exists( $prop, $this->legacy_container ) ) {
return $this->legacy_container[ $prop ];
}
if ( $this->get_container()->has( $prop ) ) {
return $this->get_container()->get( $prop );
}
+
+ return null; // or throw new \InvalidArgumentException("Property '$prop' not found");
}Committable suggestion skipped: line range outside the PR's diff.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Prioritize Legacy Container
If someone override the service through
dokan_get_class_containerfilter then the legacy container service will be used instead of Container Service.Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
These changes may lead to more reliable access to properties within the application, ensuring a smoother user experience.