-
Notifications
You must be signed in to change notification settings - Fork 20
Exclude by default content-types !== 'text/html' #19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@snapshotpl, here's a little P/R for JsonResponse. Actually I think I should be the default for all content-types !== 'text/html' Let me know what you think. This P/R is more for discussion. See also: #18 |
src/PhpDebugBarMiddleware.php
Outdated
* @param DebugBarRenderer $debugbarRenderer | ||
* @param bool $excludeNonHtmlContent Whether to disable debugbar on content-types != 'test/html' | ||
*/ | ||
public function __construct(DebugBarRenderer $debugbarRenderer, $excludeNonHtmlContent=true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value change current behaviour. Can we stay as is by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my part, yes
@@ -49,7 +63,10 @@ public function process(ServerRequestInterface $request, DelegateInterface $dele | |||
|
|||
if ($this->isHtmlResponse($response)) { | |||
return $this->attachDebugBarToResponse($response); | |||
} elseif ($this->excludeNonHtmlContent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test case
src/PhpDebugBarMiddleware.php
Outdated
protected $excludeNonHtmlContent; | ||
|
||
/** | ||
* PhpDebugBarMiddleware constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless information - to remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phpstorm ;)
protected $debugBarRenderer; | ||
|
||
public function __construct(DebugBarRenderer $debugbarRenderer) | ||
/** | ||
* @var bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe will be better to have some policy for that? Sometimes you want to debug json, but not generated pdf. With bool
it's not possible. We can provide some read implmentation and simple interface to provide own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree... not sure if I can get some time to think about it. We could add a special key '__debugbar' if it does not already exist . But I believe, going content-type-aware will need some more refactoring.
And as you see tests are broken ;( |
@snapshotpl I've fixed the tests by setting default I'm not sure that I can fix the current behaviour, would need probably some breaking changes, but for sure refactoring of the middleware. I let the P/R living here, but good if you can find a better fix |
see #18