Move default into class so simpler ::class syntax can be used. #9806
Conversation
| * @param array $config Configuration options to use. If empty, `Configure::read('Error')` | ||
| * will be used. | ||
| */ | ||
| public function __construct($renderer = null, array $config = []) | ||
| { | ||
| if ($renderer === null) { | ||
| $renderer = Configure::read('Error.exceptionRenderer'); |
There was a problem hiding this comment.
Couple of lines below there's Configure::read('Error') which would return array containing exceptionRenderer so a separate call seems unnecessary.
Personally I would avoid setting $this->renderer unless explicitly passed and then get the renderer directly from $this->_config in getRenderer() method if $this->renderer is unset.
There was a problem hiding this comment.
That makes indeed more sense alltogether. Wanna help me adjust the PR here?
Current coverage is 94.86% (diff: 100%)@@ 3.next #9806 diff @@
==========================================
Files 417 417
Lines 29367 29535 +168
Methods 3616 3647 +31
Messages 0 0
Branches 0 0
==========================================
+ Hits 27863 28017 +154
- Misses 1504 1518 +14
Partials 0 0
|
|
Looks good to me. |
| @@ -121,6 +133,10 @@ public function handleException($exception, $request, $response) | |||
| */ | |||
| protected function getRenderer($exception) | |||
| { | |||
| if (!$this->renderer) { | |||
| $this->renderer = $this->config('exceptionRender') ?: ExceptionRenderer::class; | |||
| } | |||
There was a problem hiding this comment.
Any reason this couldn't be done in the constructor? Are you trying to avoid the class load?
There was a problem hiding this comment.
I don't see the need to check for default / fallback class so early until it needs to be actually instantiated.
There was a problem hiding this comment.
Isn't this a case for using $_defaultConfig?
There was a problem hiding this comment.
Good point, but that would introduce strings again, which is what I think @dereuromark is trying to remove.
There was a problem hiding this comment.
No, I only wanted to not have it outside to be a requirement to be passed in :) See the linked app PR
| @@ -121,6 +133,10 @@ public function handleException($exception, $request, $response) | |||
| */ | |||
| protected function getRenderer($exception) | |||
| { | |||
| if (!$this->renderer) { | |||
| $this->renderer = $this->config('exceptionRender') ?: ExceptionRenderer::class; | |||
There was a problem hiding this comment.
Shouldn't this be exceptionRenderer?
dereuromark
force-pushed
the
3.next-error-handler-middleware
branch
from
63a5ab5
to
f4f8100
Compare
|
I rebased and fixed the name. |
| if (is_string($this->renderer)) { | ||
| $class = App::className($this->renderer, 'Error'); | ||
| if (!$this->exceptionRenderer) { | ||
| $this->exceptionRenderer = $this->config('exceptionRender') ?: ExceptionRenderer::class; |
There was a problem hiding this comment.
Still needs to be exceptionRenderer.
There was a problem hiding this comment.
Yeah the config value doesn't!atch the one in the app skeleton
The default should be moved inside the class to allow simpler Application setup:
etc
No need to force instantiation here.