Handle redirections before creating HTML response #8

edigu · 2016-05-23T23:00:16Z

I'm trying-out the phpdebugbar for a several days, it does the job nearly perfect. Thank you for your effort.

This PR aims to fix a silly behaviour which occurs when $outReponse is a Zend\Diactoros\Response\RedirectResponse instance.

I have a zend-expressive application which redirects users to another locations after login/logout like specific actions by returning a RedirectResponse instance to outer middleware. The problem is debugbar checks html content-type header to decide the response has HTML or not:

if ($this->isHtmlResponse($outResponse)) { ...

When the response doesn't have that header (like RedirectResponse case) debugbar creates a new HtmlResponse() and returns it which completely breaks my app's redirect flow.

P.s. My IDE applied some common coding standards via php-cs-fixer after save the file automatically. I can rollback them if you don't like.

snapshotpl · 2016-05-24T05:21:07Z

src/PhpDebugBarMiddleware.php

+     */
+    private function isRedirectResponse(ResponseInterface $response)
+    {
+        return $response instanceof RedirectResponse;


I will be working with Zend Diactoros, however we need to remember that's psr-7 implementation and someone can use other library. I prefere to check location header in response object

Sure, my first implementation was based on httpStatusCode 301 and 302 values. Since this repository depends on diactoros via composer.json and there are less-common status codes exists like 303, 304 etc.. i decided to check instance type. What you think about diactoros dependency?

We will add diactoros dependency and then? What if someone use eg https://github.com/guzzle/psr7 ?

I mean that middleware already depends on diactoros anyway I got the point, diactoros based redirection check would break other psr implementations. I'll send a patch shortly.

snapshotpl · 2016-05-24T05:21:48Z

@edigu thanks for pr, can u also provide unit test?

edigu · 2016-05-24T21:00:05Z

I just add another commit which doesn't depends RedirectResponse. I also add a unit test.

snapshotpl · 2016-05-24T21:09:33Z

src/PhpDebugBarMiddleware.php

+     */
+    private function isRedirect(ResponseInterface $response)
+    {
+        return in_array($response->getStatusCode(), [301, 302, 303, 307, 308]);


What if I send 301 without Location header?

I'm not sure about that. Debugbar may just return it. RFC says:

The new permanent URI SHOULD be given by the Location field in the response. Unless the request method was HEAD, the entity of the response SHOULD contain a short hypertext note with a hyperlink to the new URI(s).

IMHO producing a 301 response without a location header would a bad response for other layers too like nginx/apache and browsers. I think this is developer's responsibility.

snapshotpl · 2016-05-25T17:22:57Z

I think little about it and what about stay with return html as is now, and add link from Location header? It will not break your flow, and give you possibility to debug your page which generate redirect response.

edigu · 2016-05-26T12:36:17Z

I'm trying to understand why phpdebugbar (a toolbar for development purposes) generates a new template for developer/app?

I mean:

$template = '<html><head>%s</head><body><h1>DebugBar</h1><p>Response:</p><pre>%s</pre>%s</body></html>';

I have also 404/50X handlers and RedirectResponse instances for redirects. Every time in my app's workflow, phpdebugbar interrupts my app by dumping that template because it can't inject the debugbar head body etc..

IMHO if a middleware doesn't know how to handle a response, it should directly return it AS IS instead of trying to generate a new one.

snapshotpl · 2016-05-26T13:50:30Z

This template is generate when you return something else than html (json or xml)

edigu · 2016-05-26T18:39:09Z

Since json and xml are just data formats (I mean no HTML markup exists in the response) debugbar may stop there (i think creating a new HtmlResponse instance is the main problem here) or try to inject already collected metrics (params, queries, timinigs etc) into the response in an acceptable manner something like:

{
"status" : 200,
"data" : { },
"debugbar" : {
    "timing" : {}
    "pdo" : {},
    "doctrine" : {},
   "environment": {}
 }
}

or

<?xml version="1.0" encoding="UTF-8"?>
    <data>
      <foo />
   </data>
   <debugbar>
       <timing />
   </debugbar>
</xml>

maybe. I'm still not sure which one is better way to go. Returning the response AS IS sounds like simpler.

snapshotpl · 2016-05-30T17:35:28Z

I think we need to make this toolbar more flexible, because we cannot fits all need using one implementation. So attaching should be abstraction, I will do it.

In my opinion current implementation, if it's breaks redirect flow, it should still do it, but we can add some location information to generated html. In v2 I will make it more flexible to create own attach implementation, ok?

glen-84 · 2016-07-11T15:47:00Z

@snapshotpl When will this be fixed? I can't use redirects in my application while this middleware is active. 😞

Erythros · 2016-10-04T08:06:16Z

Any news on this?

snapshotpl · 2018-04-17T22:00:34Z

@edigu @glen-84 @Erythros can you take a look at #21 ?

edigu · 2018-04-17T22:46:03Z

IMHO debugbar should definitely check the response to make sure it is suitable to injecting more code into it. For e.g.:

function isResponseApplicable(ResponseInterface $response): bool
{
   // Does $response has a json/xml header?
   // Is it a kind of redirect response? 301/302?
   // Is it a 401/403/404 page? (with/without body)
   // Does the body contains non ascii (binary) content?
   // Does $response has a text/html text/plain header?
}

Then I would inject the debugbar only after make sure that the response is applicable.

snapshotpl · 2018-04-18T20:50:53Z

@edigu I know that's many ways (or cases) when you want to disable or enable debugbar. That's why I pushed #21 . Now you will be able to force enable or disable debugbar three ways.

Does $response has a json/xml header?

Then response will be wrapped json/xml response as html response.

Is it a kind of redirect response? 301/302?

Not every 3xx you want to redirect - sometimes you want to debug application before redirect.

Is it a 401/403/404 page? (with/without body)

Don't see problem here. Same like 2xx or 5xx so debug bar can be attached

Does the body contains non ascii (binary) content?

Can be fixed by new "force" feature

Does $response has a text/html text/plain header?

To text/html debugbar will be attached. With text/plain will work like json/xml

glen-84 · 2018-04-21T08:50:51Z

I ended up uninstalling this package, because the redirect behaviour was really annoying (and I had an issue with jQuery as well).

I agree with @edigu that the default behaviour should be to inject into HTML responses only.

snapshotpl · 2018-04-26T11:30:06Z

Fixed in #21

Handle redirect responses before serializing the response body

2410029

edigu changed the title ~~Handle redirect responses before serializing the response body~~ Handle redirect responses before creating HTML response May 23, 2016

snapshotpl reviewed May 24, 2016
View reviewed changes

Check HTTP status code for redirects instead of instance, add unit tests

4317e1c

snapshotpl reviewed May 24, 2016
View reviewed changes

edigu changed the title ~~Handle redirect responses before creating HTML response~~ Handle redirections before creating HTML response May 26, 2016

snapshotpl mentioned this pull request Apr 17, 2018

Disable and enable debug bar #21

Merged

snapshotpl closed this Apr 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle redirections before creating HTML response #8

Handle redirections before creating HTML response #8

edigu commented May 23, 2016 •

edited

Loading

snapshotpl May 24, 2016 •

edited

Loading

edigu May 24, 2016

snapshotpl May 24, 2016

edigu May 24, 2016 •

edited

Loading

snapshotpl commented May 24, 2016

edigu commented May 24, 2016

snapshotpl May 24, 2016

edigu May 24, 2016

snapshotpl commented May 25, 2016

edigu commented May 26, 2016 •

edited

Loading

snapshotpl commented May 26, 2016

edigu commented May 26, 2016

snapshotpl commented May 30, 2016

glen-84 commented Jul 11, 2016

Erythros commented Oct 4, 2016

snapshotpl commented Apr 17, 2018

edigu commented Apr 17, 2018

snapshotpl commented Apr 18, 2018 •

edited

Loading

glen-84 commented Apr 21, 2018

snapshotpl commented Apr 26, 2018

Handle redirections before creating HTML response #8

Handle redirections before creating HTML response #8

Conversation

edigu commented May 23, 2016 • edited Loading

snapshotpl May 24, 2016 • edited Loading

Choose a reason for hiding this comment

edigu May 24, 2016

Choose a reason for hiding this comment

snapshotpl May 24, 2016

Choose a reason for hiding this comment

edigu May 24, 2016 • edited Loading

Choose a reason for hiding this comment

snapshotpl commented May 24, 2016

edigu commented May 24, 2016

snapshotpl May 24, 2016

Choose a reason for hiding this comment

edigu May 24, 2016

Choose a reason for hiding this comment

snapshotpl commented May 25, 2016

edigu commented May 26, 2016 • edited Loading

snapshotpl commented May 26, 2016

edigu commented May 26, 2016

snapshotpl commented May 30, 2016

glen-84 commented Jul 11, 2016

Erythros commented Oct 4, 2016

snapshotpl commented Apr 17, 2018

edigu commented Apr 17, 2018

snapshotpl commented Apr 18, 2018 • edited Loading

glen-84 commented Apr 21, 2018

snapshotpl commented Apr 26, 2018

edigu commented May 23, 2016 •

edited

Loading

snapshotpl May 24, 2016 •

edited

Loading

edigu May 24, 2016 •

edited

Loading

edigu commented May 26, 2016 •

edited

Loading

snapshotpl commented Apr 18, 2018 •

edited

Loading