🐛 auto control discovery #20
Conversation
WalkthroughThis change updates several internal mechanisms for handling "control" classes and properties. It standardizes namespace usage in generated code, switches from method to property checks for control resolution, adds property existence checks before instantiating controls, and changes how control instances are retrieved within query scopes. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder
participant Model
participant Control
Builder->>Model: getModel()
Model-->>Builder: Model class
Builder->>Model: ::control()
Model->>Control: instantiate Control
Control->>Builder: queried(builder, user)
Possibly related PRs
Poem
✨ 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 (
|
![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/Console/ControlMakeCommand.php (1)
132-132: Consider removing trailing whitespaceThe added blank line after the query closure may be unintentional formatting. Consider removing trailing whitespace unless it serves a specific purpose in the generated code structure.
}), - +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Console/ControlMakeCommand.php(2 hunks)src/Controls/Control.php(1 hunks)src/Controls/HasControl.php(1 hunks)src/Controls/HasControlScope.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Controls/HasControlScope.php (1)
src/Controls/HasControl.php (1)
control(22-27)
src/Controls/HasControl.php (1)
src/Controls/Control.php (1)
new(221-224)
🔇 Additional comments (4)
src/Controls/HasControl.php (1)
36-36: Excellent defensive programming improvement!The addition of
property_exists()check prevents fatal errors when thecontrolproperty is not defined on a model class. This aligns well with the broader refactoring from method-based to property-based control resolution.src/Console/ControlMakeCommand.php (1)
119-119: Good practice: Absolute namespace referenceAdding the leading backslash ensures the perimeter class name is treated as an absolute namespace path, preventing potential conflicts with relative namespace resolution.
src/Controls/Control.php (1)
240-242:✅ Verification successful
Verify consistency of property-based control resolution
The change from
method_existstoproperty_existsaligns with the broader refactoring to standardize control discovery. However, ensure all control usages throughout the codebase are consistent with this property-based approach.Let me verify that all models using controls have the static
controlproperty defined correctly:
🏁 Script executed:
#!/bin/bash # Description: Find all model classes that might be using controls and verify they have the control property defined properly # Search for models that use HasControl trait echo "Models using HasControl trait:" rg -A 5 -B 5 "use.*HasControl" --type php echo -e "\n=== Static control property definitions ===" # Search for static control property definitions rg "static.*\\\$control" --type php echo -e "\n=== Control property access patterns ===" # Search for control property access patterns rg "::control\(\)" --type phpLength of output: 2113
Property-based control resolution verified
- The only model using the
HasControltrait (tests/Support/Models/Model.php) definesprotected static $control.- No
method_exists(..., 'control')calls remain in the codebase.- All
control()invocations inControl.php,HasControl.php, andHasControlScope.phphonor theproperty_exists(..., 'control')check.src/Controls/HasControlScope.php (1)
57-57:❓ Verification inconclusive
Consistent with the control discovery refactoring
The change from instance method
newControl()to static methodcontrol()aligns with the standardization across the codebase. This leverages the staticcontrol()method defined in theHasControltrait (lines 22-27 insrc/Controls/HasControl.php).Let me verify that all models using this scope properly implement the required static control method:
🏁 Script executed:
#!/bin/bash # Description: Verify that models using HasControlScope have the necessary control() method available echo "Searching for HasControlScope usage:" rg -A 10 -B 5 "HasControlScope" --type php echo -e "\n=== Models with HasControl trait (should have control() method) ===" rg -A 3 "use.*HasControl" --type php echo -e "\n=== Potential models without HasControl trait but using controlled() queries ===" rg -B 5 -A 5 "->controlled\(\)" --type phpLength of output: 3270
Ensure all Eloquent models using this scope implement
control()The switch from the instance method
newControl()to the staticcontrol()call is correct and aligns with theHasControltrait’s refactoring. However, I wasn’t able to find any application models (outside of the test suite) importing theHasControltrait—and thus exposing the staticcontrol()method—that would make use of this scope. Please verify that:
- Every model meant to be governed by this scope imports
Lomkit\Access\Controls\HasControl;- Each of those models either defines the static
$controlproperty or otherwise provides apublic static function control()Without that, calls to
$builder->getModel()::control()will fail at runtime.
Summary by CodeRabbit