Add a getAttributes to IControllerMethodReflector #37279
Conversation
Signed-off-by: Joas Schilling <coding@schilljs.com>
|
I'm on the fence here. It's a small enhancement in all fairness. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -69,15 +68,11 @@ | |||
| $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action'); | |||
| $this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), $action); | |||
| } else { | |||
| $reflectionMethod = new ReflectionMethod($controller, $methodName); | |||
| $attributes = $reflectionMethod->getAttributes(BruteForceProtection::class); | |||
| $attributes = $this->reflector->getAttributes(BruteForceProtection::class); | |||
Check failure
Code scanning / Psalm
InvalidArgument
| @@ -96,16 +91,13 @@ | |||
| $this->throttler->sleepDelay($ip, $action); | |||
| $this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata()); | |||
| } else { | |||
| $reflectionMethod = new ReflectionMethod($controller, $methodName); | |||
| $attributes = $reflectionMethod->getAttributes(BruteForceProtection::class); | |||
| $attributes = $this->reflector->getAttributes(BruteForceProtection::class); | |||
Check failure
Code scanning / Psalm
InvalidArgument
| @@ -63,8 +62,7 @@ | |||
| return; | |||
| } | |||
|
|
|||
| $reflectionMethod = new ReflectionMethod($controller, $methodName); | |||
| $hasAttribute = !empty($reflectionMethod->getAttributes(UseSession::class)); | |||
| $hasAttribute = !empty($this->reflector->getAttributes(UseSession::class)); | |||
Check failure
Code scanning / Psalm
InvalidArgument
| @@ -86,8 +84,7 @@ | |||
| return $response; | |||
| } | |||
|
|
|||
| $reflectionMethod = new ReflectionMethod($controller, $methodName); | |||
| $hasAttribute = !empty($reflectionMethod->getAttributes(UseSession::class)); | |||
| $hasAttribute = !empty($this->reflector->getAttributes(UseSession::class)); | |||
Check failure
Code scanning / Psalm
InvalidArgument
| use OCP\AppFramework\Utility\IControllerMethodReflector; | ||
|
|
||
| /** | ||
| * Reads and parses annotations from doc comments | ||
| */ | ||
| class ControllerMethodReflector implements IControllerMethodReflector { | ||
|
|
||
| protected ?\ReflectionMethod $reflection; |
There was a problem hiding this comment.
To be more specific because exist other types of reflections:
| protected ?\ReflectionMethod $reflection; | |
| protected ?\ReflectionMethod $reflectionMethod; |
| @@ -138,4 +141,18 @@ public function getAnnotationParameter(string $name, string $key): string { | |||
|
|
|||
| return ''; | |||
| } | |||
|
|
|||
| /** | |||
| * @template T of Attribute | |||
There was a problem hiding this comment.
Attribute classes do not extend Attribute class, which is final. So you cannot do that.
|
Subjectively what I don't like about the controller method reflector is that is so stateful. There are two methods to work with it. One analyzes a controller method, another one allows you to test for specific attributes. It sounds a bit artificial but there is no guarantee that at the point of asking for an attribute, nobody else called the In that sense, and with the way PHP reflection works anyway, I would prefer to make the service stateless and pass the controller object and method name into This remark is not new with attributes. The problem was there with annotations as well. It is just that parsing phpdoc annotations is expensive and there the state for caching makes sense for better performance. |
|
Exactly my thoughts after seeing the resulting PR. |
As discussed in #36928 (comment)
Looking at the diff, I'm not sure it's worth it.
Checklist