Make PrettyPageHandler more easily extendable #517
Conversation
There was a problem hiding this comment.
This is a good PR, thank you!
I support adding more ability to extend, as long as it is explicitly designed for. Your identified functions getFrames and getCode are good places to extend Whoops.
Could you make their names and descriptions a bit more explicit? Instead of getCode could you make it getExceptionCode, instead of getFrames - getExceptionFrames. In their descriptions in the doc comment, describe “Get the code of the exception that is currently being handled”.
My point being if we are adding new functions that 3rd-party developers can use to extend, let’s make sure the functions are very explicitly documented. :-)
Overall, thanks for the PR!
| @@ -666,7 +691,7 @@ public function blacklist($superGlobalName, $key) { | |||
| * @param $superGlobalName string the name of the superglobal array, e.g. '_GET' | |||
| * @return array $values without sensitive data | |||
| */ | |||
| private function masked(array $superGlobal, $superGlobalName) { | |||
| private function getMasked(array $superGlobal, $superGlobalName) { | |||
There was a problem hiding this comment.
Why is this rename bundled with this commit? Note that the method is still private. Did you mean to make it protected as well?
There was a problem hiding this comment.
I toyed with the idea of making it protected but I think this is the kind of method that really is meant to be private
| $extraTables = array_map(function ($table) use ($inspector) { | ||
| return $table instanceof \Closure ? $table($inspector) : $table; | ||
| }, $this->getDataTables()); | ||
| $extraTables = array_map( |
There was a problem hiding this comment.
I tend to avoid reformatting in the same commit as meaningful changes, as to keep the diffs more meaningful.
There was a problem hiding this comment.
Sorry, I tend to reformat right when I see this kind of things
|
I'll update this evening for sure |
|
Cleaner PR! |
|
Thank you for your contribution! |
|
Quick question: next release has a planed date? |
|
2.1.10 released. |
Extending from PrettyPageHandler is a real pain: almost everything is private (nor everything should be protected neither) and most of the actual work is done in "handle" method so if I want to extend the class I've to completely copy/and/rewrite the whole method, even for the simplest of the modifications, with the problems this carries with future modifications of the original. This violates the O in SOLID
My use case: I need to filter certain frames out of the stack, and the corresponding exception code as well. To accomplish this I've just extracted "frames" and "code" collection into protected methods that can be overridden in children