Feat/rewrote control registering #29
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Artisan
participant ControlMakeCommand
participant FileSystem
Developer->>Artisan: Run make:control
Artisan->>ControlMakeCommand: Execute command
ControlMakeCommand->>FileSystem: List available models
ControlMakeCommand->>Developer: Prompt for control name and model
Developer->>ControlMakeCommand: Provide input
ControlMakeCommand->>FileSystem: Generate control file with $model property
sequenceDiagram
participant App
participant Access
participant Control
participant Model
App->>Access: controlForModel(Model)
Access->>Access: Iterate registered controls
Access->>Control: isModel(Model)
Control-->>Access: true/false
Access-->>App: Return matching Control instance or null
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code Graph Analysis (1)src/Controls/HasControlScope.php (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🔇 Additional comments (2)
✨ 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: 7
🧹 Nitpick comments (2)
src/Access.php (2)
86-86: Fix docblock annotationThe docblock uses
@varbut should use@paramfor method parameters.- * @var string[] + * @param string[] $paths
139-139: Consider making classFromFile privateThis method appears to be used only internally within the class.
- protected function classFromFile(SplFileInfo $file, string $basePath) + private function classFromFile(SplFileInfo $file, string $basePath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Access.php(1 hunks)src/AccessServiceProvider.php(2 hunks)src/Console/ControlMakeCommand.php(10 hunks)src/Console/stubs/control.stub(1 hunks)src/Controls/Control.php(1 hunks)src/Controls/HasControl.php(2 hunks)src/Policies/ControlledPolicy.php(3 hunks)tests/Support/Access/Controls/ModelControl.php(1 hunks)tests/TestCase.php(2 hunks)tests/Unit/Console/MakeCommandsTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/Support/Access/Controls/ModelControl.php (2)
src/Access.php (1)
Access(14-149)tests/Support/Models/Model.php (1)
Model(12-43)
src/Policies/ControlledPolicy.php (1)
src/Access.php (2)
Access(14-149)controlForModel(67-80)
tests/Unit/Console/MakeCommandsTest.php (2)
tests/TestCase.php (1)
setUp(18-26)tests/Unit/TestCase.php (1)
setUp(10-15)
src/AccessServiceProvider.php (1)
src/Access.php (3)
Access(14-149)discoverControls(87-117)discoverControlsWithin(124-129)
src/Access.php (3)
tests/Support/Models/Model.php (1)
Model(12-43)src/Controls/Control.php (2)
Control(12-222)isModel(28-31)src/Policies/ControlledPolicy.php (1)
create(72-75)
🔇 Additional comments (11)
tests/Support/Access/Controls/ModelControl.php (1)
15-15: LGTM! Model association follows the new pattern correctly.The explicit declaration of the associated model via the
$modelproperty aligns with the refactoring to centralize control-model associations through theAccessclass.src/Console/stubs/control.stub (1)
12-16: LGTM! Stub template correctly implements the new model association pattern.The property declaration includes proper documentation and type annotations. The
{{ namespacedModel }}placeholder will be appropriately replaced by the command that uses this stub.tests/TestCase.php (2)
9-9: LGTM! Proper imports for the new control system.The imports are necessary for the new
Access-based control registration system used in the test setup.Also applies to: 11-11
22-25: LGTM! Test setup correctly registers controls for the new system.The test setup properly creates an
Accessinstance and registers theModelControlto ensure the control discovery system works correctly during testing.src/AccessServiceProvider.php (2)
34-34: LGTM! Integration of control discovery into service provider registration.The call to
registerAccessControls()properly integrates the newAccess-based control discovery system into the service provider lifecycle.
97-107: LGTM! Control discovery implementation is correct.The method correctly instantiates the
Accessclass and triggers control discovery from the default paths. SinceAccessuses static arrays to store controls, the instance-based approach works correctly.src/Controls/HasControl.php (2)
5-5: LGTM! Necessary import for the new control system.The
Accessimport is required for the updatednewControl()method implementation.
26-26: LGTM! Control discovery now uses the centralized Access class.The change from static control resolution to
Access::controlForModel()aligns with the refactoring to centralize control management. The method correctly handles the nullable return type.src/Policies/ControlledPolicy.php (1)
37-37: Verify direct property access is intentionalThe change from
$this->getModel()to$this->modeluses direct property access instead of the getter method. While more efficient, this bypasses any potential logic in the getter.If the getter method
getModel()on line 25 performs no additional logic beyond returning the property, this change is fine. Otherwise, consider keeping the getter usage for consistency and future flexibility.src/Controls/Control.php (1)
28-31: LGTM!The
isModel()method provides a clean way to check model association.src/Console/ControlMakeCommand.php (1)
119-134: Well-implemented model replacementsThe
buildModelReplacements()method properly generates all necessary stub replacements for the model, including namespaced and variable forms.
| @unlink(app_path('Access/Perimeters/TestPerimeter.php')); | ||
| @unlink(app_path('Access/Controls/TestControl.php')); | ||
| @unlink(app_path('Access/Controls/Control.php')); | ||
| @unlink(app_path('Access/Perimeters/TestPerimeter.php')); |
There was a problem hiding this comment.
Remove duplicate file deletion
Line 16 duplicates the file deletion already performed on line 13.
@unlink(app_path('Access/Perimeters/TestPerimeter.php'));
@unlink(app_path('Access/Controls/TestControl.php'));
@unlink(app_path('Access/Controls/Control.php'));
- @unlink(app_path('Access/Perimeters/TestPerimeter.php'));
@unlink(app_path('Access/Perimeters/SecondTestPerimeter.php'));🤖 Prompt for AI Agents
In tests/Unit/Console/MakeCommandsTest.php between lines 13 and 16, there is a
duplicate call to unlink the file 'Access/Perimeters/TestPerimeter.php' on lines
13 and 16. Remove the duplicate unlink call on line 16 to avoid redundant file
deletion attempts.
| /** | ||
| * The control name resolver. | ||
| * The model the control refers to. | ||
| * | ||
| * @var callable | ||
| * @var class-string<Model> | ||
| */ | ||
| protected static $controlNameResolver; | ||
| protected string $model; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider initializing the model property
The $model property is declared but not initialized. This could cause issues if accessed before being set by subclasses.
Consider providing a default value or making it nullable:
/**
* The model the control refers to.
*
* @var class-string<Model>
*/
- protected string $model;
+ protected string $model = '';🤖 Prompt for AI Agents
In src/Controls/Control.php around lines 14 to 19, the protected string property
$model is declared but not initialized, which may lead to errors if accessed
before being set. To fix this, either initialize $model with a default
class-string value or change its type to nullable by adding a question mark and
set it to null by default, ensuring safe access before assignment.
Summary by CodeRabbit