Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

belgattitude
Copy link

see #18

@belgattitude
Copy link
Author

@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

* @param DebugBarRenderer $debugbarRenderer
* @param bool $excludeNonHtmlContent Whether to disable debugbar on content-types != 'test/html'
*/
public function __construct(DebugBarRenderer $debugbarRenderer, $excludeNonHtmlContent=true)
Copy link
Member

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?

Copy link
Author

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test case

protected $excludeNonHtmlContent;

/**
* PhpDebugBarMiddleware constructor.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless information - to remove

Copy link
Author

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
Copy link
Member

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.

Copy link
Author

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.

@snapshotpl
Copy link
Member

And as you see tests are broken ;(

@belgattitude
Copy link
Author

@snapshotpl I've fixed the tests by setting default excludeNonHtmlContent to false... But then Json is broken again then.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants