Feature/small adjustments #33
Conversation
WalkthroughA new configuration mapping for access control method names was introduced, allowing aliasing of method names to permission strings. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Control
participant Config
participant Perimeter
User->>Control: calls applies(user, method, model)
Control->>Config: fetch 'methods' mapping
Control->>Control: map method via config (appliesMethod)
Control->>Perimeter: applyAllowedCallback(user, appliesMethod, model)
Perimeter-->>Control: returns permission result
Control-->>User: returns boolean
Estimated code review effort2 (~15 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements small adjustments to the access control system, primarily standardizing method naming conventions and improving configuration flexibility. The changes focus on normalizing the "viewAny" method to use "view" permissions and adding proper namespace escaping.
- Standardizes "viewAny" method to use "view" permissions through configuration mapping
- Adds leading backslashes for proper namespace escaping in generated code
- Improves command option handling and fixes minor typos
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| config/access-control.php | Adds method mapping configuration to standardize permission checking |
| src/Controls/Control.php | Implements configurable method mapping for permission checks |
| src/Console/stubs/control.stub | Adds leading backslash for proper namespace escaping |
| src/Console/PerimeterMakeCommand.php | Changes overlay option from VALUE_OPTIONAL to VALUE_NONE |
| tests/Unit/Console/MakeCommandsTest.php | Updates test assertion to expect proper namespace escaping |
| tests/Feature/PoliciesTest.php | Updates Gate definition to use standardized "view" method |
| tests/Feature/ControlsShouldTest.php | Updates test cases and adds new test for method configuration |
![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
🧹 Nitpick comments (1)
src/Controls/Control.php (1)
64-68: Configuration-based method mapping implementation looks solid.The implementation correctly introduces method name aliasing through configuration while maintaining backward compatibility. The use of null coalescing operator ensures graceful fallback to the original method name.
However, consider the consistency with hardcoded method names in
applyQueryControl()andapplyScoutQueryControl()methods (lines 137, 172) which still use'view'directly. Should these also respect the configuration mapping?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
config/access-control.php(1 hunks)src/Console/PerimeterMakeCommand.php(1 hunks)src/Console/stubs/control.stub(1 hunks)src/Controls/Control.php(1 hunks)tests/Feature/ControlsShouldTest.php(2 hunks)tests/Feature/PoliciesTest.php(1 hunks)tests/Unit/Console/MakeCommandsTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/Feature/PoliciesTest.php (1)
tests/Support/Models/User.php (1)
User(10-64)
src/Controls/Control.php (2)
tests/Support/Access/Controls/ModelControl.php (1)
perimeters(17-75)src/Perimeters/Perimeter.php (1)
applyAllowedCallback(76-79)
🪛 PHPMD (2.15.0)
tests/Feature/ControlsShouldTest.php
19-19: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
97-97: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (9)
src/Console/stubs/control.stub (1)
16-16: Good improvement for namespace resolution.Adding the leading backslash ensures the fully qualified class name is resolved from the global namespace, preventing potential namespace conflicts in generated control classes.
src/Console/PerimeterMakeCommand.php (1)
107-107: Good refactor of the overlay option.Changing from
VALUE_OPTIONALtoVALUE_NONEmakes the--overlayoption a proper boolean flag, which is more intuitive and consistent with how it's used in the command logic.tests/Unit/Console/MakeCommandsTest.php (1)
112-112: Test assertion correctly updated to match stub changes.The updated assertion properly reflects the addition of the leading backslash in the generated model property, maintaining test consistency with the stub template changes.
config/access-control.php (1)
15-30: Well-designed method mapping configuration.The new
methodsconfiguration section provides a clean, centralized way to alias access control methods to their corresponding permissions. The mapping ofviewAnytoviewhelps unify similar permission checks, and the structure follows Laravel configuration conventions.tests/Feature/PoliciesTest.php (1)
27-27: Test correctly updated to reflect method mapping changes.The Gate ability name change from
'viewAny global models'to'view global models'properly reflects the new configuration mapping whereviewAnymethods are aliased toviewpermissions. The test logic remains sound while adapting to the unified permission naming.tests/Feature/ControlsShouldTest.php (4)
17-24: Excellent test coverage for the new configuration mapping feature.The new test method properly validates that the configuration mapping works as expected, testing that
'viewAny'method gets mapped to'view'permission when checking Gate abilities.
28-28: Test updated correctly to reflect the new naming convention.The Gate ability name has been appropriately changed from
'viewAny client models'to'view client models'to align with the new configuration mapping.
97-97: Test updated correctly to reflect the new naming convention.The Gate ability name has been appropriately changed from
'viewAny global models'to'view global models'to align with the new configuration mapping.
19-19: Static analysis false positives for unused parameters.The PHPMD warnings about unused
$userparameters in Gate::define callbacks are false positives. These parameters are required by Laravel's Gate callback signature even when not used in simple test scenarios.Also applies to: 28-28, 97-97
…/laravel-access-control into feature/small-adjustments
closes #32
closes #31
closes #30
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests