fix Pre Hook Exception in case _controller is not stringable #366
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
The committers listed above are authorized under a signed CLA. |
| if (is_string($controller)) { | ||
| $controllerName = $controller; | ||
| } elseif (is_array($controller) && is_object($controller[0]) && isset($controller[1])) { | ||
| // [$object, 'method'] | ||
| $controllerName = get_class($controller[0]) . '::' . $controller[1]; | ||
| } elseif (is_array($controller) && is_string($controller[0]) && isset($controller[1])) { | ||
| // ['ClassName', 'method'] | ||
| $controllerName = $controller[0] . '::' . $controller[1]; | ||
| } elseif (is_object($controller)) { | ||
| $controllerName = get_class($controller); | ||
| } else { | ||
| $controllerName = 'sub-request'; | ||
| } |
There was a problem hiding this comment.
| if (is_string($controller)) { | |
| $controllerName = $controller; | |
| } elseif (is_array($controller) && is_object($controller[0]) && isset($controller[1])) { | |
| // [$object, 'method'] | |
| $controllerName = get_class($controller[0]) . '::' . $controller[1]; | |
| } elseif (is_array($controller) && is_string($controller[0]) && isset($controller[1])) { | |
| // ['ClassName', 'method'] | |
| $controllerName = $controller[0] . '::' . $controller[1]; | |
| } elseif (is_object($controller)) { | |
| $controllerName = get_class($controller); | |
| } else { | |
| $controllerName = 'sub-request'; | |
| } | |
| if (!is_callable($controller, true, $controllerName)) { | |
| $controllerName = 'sub-request'; | |
| } |
There was a problem hiding this comment.
I like your suggestion, as it is cleaner. I am just not certain if we can assume that every controller out there passes the is_callable check.
But I don't want to create an academic discussion here and therefor slow down this fix.
Can you maybe just elaborate a bit on your suggestion?
There was a problem hiding this comment.
is_callable is only slightly stricter than the if-else statements: it additionally checks whether arrays have exactly two elements and whether objects implement ::__invoke(). (Anything that fails the is_callable() check should also cause an exception in the default ControllerResolver.)
FWIW it might make sense to listen to ControllerEvent/ControllerArgumentsEvent instead of relying on the _controller attribute to have access to the final controller callable that will be invoked.
There was a problem hiding this comment.
The problem with applying this suggestion is that there is no $controllerName when it is a callable.
But I think you are right in suggesting to listen to an event (or hooking to the end of the dispatch method - I'm not familiar with C-level PHP, so can't see what is possible and makes sense). The moment the actual controller is definitely resolved is when returning from the dispatching of the ControllerArgumentsEvent. At this point it is also guaranteed that the controller is callable (Symfony will throw an exception if it is not, but this should hopefully only ever happen in a dev environment, so should be ok to ignore here). This would avoid getting problems with any custom logic.
But looking at the resolved callable would mean that the original if-else construct in the PR is needed to get meaningful values.
There was a problem hiding this comment.
that there is no $controllerName when it is a callable
is_callable() sets $controllerName
There was a problem hiding this comment.
@Nevay that means you still think your suggestion is the best solution?
I want to move this forward. If you think that is the best solution I will apply it so that we can go on.
There was a problem hiding this comment.
I think you should try Nevay's suggestion, and add integration tests to confirm all the variances of $controller work as expected
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
============================================
- Coverage 82.17% 79.45% -2.73%
+ Complexity 1870 730 -1140
============================================
Files 137 54 -83
Lines 7822 2696 -5126
============================================
- Hits 6428 2142 -4286
+ Misses 1394 554 -840
Flags with carried forward coverage won't be shown. Click here to find out more. see 83 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
closes open-telemetry/opentelemetry-php#1569