Remove doctrine/annotations dependency, use native PHP 8 attributes#9
Merged
koriym merged 5 commits intoray-di:0.xfrom Apr 9, 2026
Merged
Conversation
Replace koriym/attributes (DualReader) with ReflectionAttribute API to eliminate the transitive dependency on the deprecated doctrine/annotations library. Raise minimum PHP to 8.0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaced Doctrine/docblock annotation handling with native PHP 8 attributes across the module; tightened composer PHP requirement to ^8.0 and removed Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors role-based access control to rely solely on native PHP 8 attributes, removes the annotation-reader stack and its dependencies, and raises the minimum PHP version to 8.0. Sequence diagram for attribute-based role check in RequiredRolesInterceptorsequenceDiagram
actor Client
participant AopInvoker
participant RequiredRolesInterceptor
participant MethodInvocation
participant ReflectionMethod
participant ReflectionClass
participant RequiresRolesAttr as RequiresRoles
participant Acl as AclInterface
participant RoleProvider as RoleProviderInterface
Client->>AopInvoker: call securedMethod()
AopInvoker->>RequiredRolesInterceptor: invoke(MethodInvocation)
RequiredRolesInterceptor->>MethodInvocation: getMethod()
MethodInvocation-->>RequiredRolesInterceptor: ReflectionMethod
RequiredRolesInterceptor->>ReflectionMethod: getAttributes(RequiresRoles)
ReflectionMethod-->>RequiredRolesInterceptor: attrs[]
alt no_method_attribute
RequiredRolesInterceptor->>MethodInvocation: getThis()
MethodInvocation-->>RequiredRolesInterceptor: targetObject
RequiredRolesInterceptor->>ReflectionClass: __construct(targetObject)
RequiredRolesInterceptor->>ReflectionClass: getAttributes(RequiresRoles)
ReflectionClass-->>RequiredRolesInterceptor: attrs[]
end
RequiredRolesInterceptor->>RequiresRolesAttr: newInstance() on attrs[0]
RequiresRolesAttr-->>RequiredRolesInterceptor: RequiresRoles instance
RequiredRolesInterceptor->>Acl: addResource(GenericResource(targetClass))
loop roles in annotation.value
RequiredRolesInterceptor->>Acl: allow(role, targetClass)
end
RequiredRolesInterceptor->>RoleProvider: getRoles()
RoleProvider-->>RequiredRolesInterceptor: roles[]
RequiredRolesInterceptor->>Acl: isAllowed(roles[], targetClass)
Acl-->>RequiredRolesInterceptor: decision
alt allowed
RequiredRolesInterceptor->>MethodInvocation: proceed()
MethodInvocation-->>AopInvoker: result
AopInvoker-->>Client: result
else denied
RequiredRolesInterceptor-->>Client: access denied error
end
Class diagram for updated role-based access control componentsclassDiagram
class RequiredRolesInterceptor {
-AclInterface acl
-RoleProviderInterface roleProvider
+__construct(AclInterface acl, RoleProviderInterface roleProvider)
+invoke(MethodInvocation invocation)
}
class RequiresRoles {
<<attribute>>
+array value
+__construct(array value)
}
class ZendAclModule {
-AclInterface acl
-string roleProvider
+__construct(AclInterface acl, string roleProviderClass)
+configure() void
}
class AclInterface {
}
class RoleProviderInterface {
}
class MethodInvocation {
+getMethod() ReflectionMethod
+getThis() object
+proceed() mixed
}
class ReflectionMethod {
+getAttributes(string name) array
}
class ReflectionClass {
+__construct(object object)
+getAttributes(string name) array
}
class GenericResource {
+__construct(string resourceId)
}
RequiredRolesInterceptor --> AclInterface : uses
RequiredRolesInterceptor --> RoleProviderInterface : uses
RequiredRolesInterceptor ..> RequiresRoles : reads
RequiredRolesInterceptor --> MethodInvocation : intercepts
RequiredRolesInterceptor ..> ReflectionMethod : reflects
RequiredRolesInterceptor ..> ReflectionClass : reflects
RequiredRolesInterceptor ..> GenericResource : creates
ZendAclModule --> AclInterface : binds
ZendAclModule --> RoleProviderInterface : binds
RequiresRoles <.. ReflectionMethod : attribute
RequiresRoles <.. ReflectionClass : attribute
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In RequiredRolesInterceptor::invoke(), if no RequiresRoles attribute exists on either the method or class, $attrs will be empty and $attrs[0] will trigger an undefined offset—add a guard to return $invocation->proceed() when no attribute is found to match the previous behavior safely.
- Now that PHP 8 is the minimum, you can simplify RequiresRoles by using constructor property promotion (e.g.,
public function __construct(public array $value) {}) and drop the separate property declaration and assignment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In RequiredRolesInterceptor::invoke(), if no RequiresRoles attribute exists on either the method or class, $attrs will be empty and $attrs[0] will trigger an undefined offset—add a guard to return $invocation->proceed() when no attribute is found to match the previous behavior safely.
- Now that PHP 8 is the minimum, you can simplify RequiresRoles by using constructor property promotion (e.g., `public function __construct(public array $value) {}`) and drop the separate property declaration and assignment.
## Individual Comments
### Comment 1
<location path="src/RequiredRolesInterceptor.php" line_range="33-38" />
<code_context>
- /** @var $annotation RequiresRoles */
- $annotation = $this->reader->getMethodAnnotation($invocation->getMethod(), RequiresRoles::class);
- if (! $annotation) {
+ $attrs = $invocation->getMethod()->getAttributes(RequiresRoles::class);
+ if (! $attrs) {
$class = new \ReflectionClass($invocation->getThis());
- $annotation = $this->reader->getClassAnnotation($class, RequiresRoles::class);
+ $attrs = $class->getAttributes(RequiresRoles::class);
}
+ $annotation = $attrs[0]->newInstance();
$target = get_class($invocation->getThis());
$this->acl->addResource(new GenericResource($target));
</code_context>
<issue_to_address>
**issue:** Handle the case where no RequiresRoles attribute is present before indexing into $attrs.
If neither the method nor the class has a RequiresRoles attribute, `$attrs` will be empty and `$attrs[0]` will throw. Consider early-returning when no attributes are found to preserve the existing "no annotation → no ACL enforcement" behavior:
```php
$attrs = $invocation->getMethod()->getAttributes(RequiresRoles::class);
if (! $attrs) {
$class = new \ReflectionClass($invocation->getThis());
$attrs = $class->getAttributes(RequiresRoles::class);
}
if (! $attrs) {
return $invocation->proceed();
}
$annotation = $attrs[0]->newInstance();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Return $invocation->proceed() when RequiresRoles attribute is not found on method or class, preventing undefined offset error - Use PHP 8 constructor property promotion in RequiresRoles Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/RequiredRolesInterceptor.php (1)
33-38: Use Ray.Aop'sgetAnnotation()API instead of rawgetAttributes().
MethodInvocation::getMethod()returnsRay\Aop\ReflectionMethod, which providesgetAnnotation()andgetAnnotations()methods as the idiomatic way to retrieve attributes. While the current code using rawgetAttributes()works correctly (ReflectiveMethodInvocation::getMethod() already unwraps weaved objects to inspect the original class), using the framework's intended API is cleaner and more maintainable.💡 Suggested refactoring
- $attrs = $invocation->getMethod()->getAttributes(RequiresRoles::class); - if (! $attrs) { - $class = new \ReflectionClass($invocation->getThis()); - $attrs = $class->getAttributes(RequiresRoles::class); - } - if (! $attrs) { - return $invocation->proceed(); - } - - $annotation = $attrs[0]->newInstance(); + $method = $invocation->getMethod(); + $annotation = $method->getAnnotation(RequiresRoles::class) + ?? $method->getDeclaringClass()->getAnnotation(RequiresRoles::class); + if ($annotation === null) { + return $invocation->proceed(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/RequiredRolesInterceptor.php` around lines 33 - 38, Replace the raw ReflectionAttribute usage with Ray.Aop's annotation API: call $invocation->getMethod()->getAnnotation(RequiresRoles::class) to fetch the method-level RequiresRoles annotation, and if that returns null call $invocation->getMethod()->getDeclaringClass()->getAnnotation(RequiresRoles::class) (or use the class-level reflection from the invocation) and assign the result to $annotation; update references to $attrs and newInstance() accordingly so you no longer use getAttributes() or \ReflectionClass directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/RequiredRolesInterceptor.php`:
- Around line 33-38: Replace the raw ReflectionAttribute usage with Ray.Aop's
annotation API: call
$invocation->getMethod()->getAnnotation(RequiresRoles::class) to fetch the
method-level RequiresRoles annotation, and if that returns null call
$invocation->getMethod()->getDeclaringClass()->getAnnotation(RequiresRoles::class)
(or use the class-level reflection from the invocation) and assign the result to
$annotation; update references to $attrs and newInstance() accordingly so you no
longer use getAttributes() or \ReflectionClass directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6f9e5dc-6e9a-42d2-9209-64a840b2100d
📒 Files selected for processing (8)
composer.jsonsrc/Annotation/RequiresRoles.phpsrc/RequiredRolesInterceptor.phpsrc/ZendAclModule.phptests/Fake/FakeClassResource.phptests/Fake/FakeResource.phptests/RequiredRoleInterceptorTest.phptests/bootstrap.php
💤 Files with no reviewable changes (4)
- tests/Fake/FakeClassResource.php
- tests/bootstrap.php
- tests/Fake/FakeResource.php
- src/ZendAclModule.php
Member
|
@keitamotegi CIの設定が古くなっていて実行できなくなっているようです。 |
Member
|
@coderabbitai approve |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/continuous-integration.yml:
- Around line 18-21: Update the CI PHP matrix in
.github/workflows/continuous-integration.yml to include the package's declared
minimum supported versions (add '8.0' and '8.1' alongside the existing
'8.2','8.3','8.4','8.5') so the workflow tests PHP versions matching
composer.json (php: ^8.0); modify the matrix values under the PHP-version list
to include '8.0' and '8.1' and ensure any conditional logic that assumes newer
versions still works with these additions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0ff59d8-de05-4c98-bed6-146447e2d9ed
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
✅ Actions performedComments resolved. Approval is disabled; enable |
Member
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
|
Tip For best results, initiate chat on the files or code changes.
[resolve] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
koriym/attributes(DualReader) を削除し、PHP 8 ネイティブのReflectionAttributeAPI に置き換えdoctrine/annotationsへの推移的依存を解消^8.0に引き上げChanges
RequiredRolesInterceptor:Doctrine\Common\Annotations\Readerを削除、ReflectionMethod::getAttributes()/ReflectionClass::getAttributes()で Attribute を取得ZendAclModule:DualReader/AnnotationReader/AttributeReaderの DI バインドを削除RequiresRoles:@Annotation/@Targetdocblock を削除Breaking Changes
@RequiresRoles({"admin"})) は使用不可。#[RequiresRoles(value: ['admin'])]に移行が必要Summary by Sourcery
Remove third-party annotation readers in favor of PHP 8 native attributes for role-based access control and drop support for PHP 7.
Enhancements:
Build:
Tests:
Summary by CodeRabbit
Breaking Changes
Improvements