Skip to content
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

Error handlers should respect outputBuffering setting #2208

Closed
akrabat opened this issue Apr 18, 2017 · 5 comments
Closed

Error handlers should respect outputBuffering setting #2208

akrabat opened this issue Apr 18, 2017 · 5 comments

Comments

@akrabat
Copy link
Member

akrabat commented Apr 18, 2017

Given this code,

$app->get("/", function ($request, $response, $args) {
    var_dump($request);
    throw new \Exception("oops");
});

I should see the var_dump'd information displayed if outputBufferring is set to append or prepend.

This requires changes to:

  • Route::__invoke() to remove the ob_end_clean() calls.
  • Pass the outputBuffering setting into Slim\Handlers\AbstracError's constructor and store as a protected property
  • Update Slim\Hanlders\Error to use ob_get_clean() and add it to the response in the correct place as per the outputBuffering setting.
@akrabat akrabat added the Slim 3 label Apr 18, 2017
@akrabat
Copy link
Member Author

akrabat commented Apr 22, 2017

Thinning about it, this is probably solvable with middleware.

@RyanNerd
Copy link
Contributor

@akrabat Is this going to be "solved with middleware"?

I'm looking at issues tagged with 'help wanted' so I can possibly pitch in. I obviously don't want to work on something that doesn't need fixing...

@geggleto
Copy link
Member

@akrabat this seems like a bug; I wouldn't want it fixed as a middleware.

@RyanNerd Let me know if you want to make a PR for this :)

@RyanNerd
Copy link
Contributor

@geggleto I'll see if I can come up with a working PR in the next few days.

@pengu-fr
Copy link

pengu-fr commented Nov 6, 2017

I have a problem with this implementation...
Removing the ob_get_clean from the "catch(s)" of Route::__invoke() breaks the ob_levels, as Route::__invoke() start the OB, but never close it in case of error. If we don't use Slim's default errorHandler, we have to know/implement Slim outputBuffering setting again, and close the OB correctly if needed... and I don't think it should be implemented in the errorHandler

IMO it's not the error handler responsability to handle the output buffer started by the Router. It should not even know about the outputBuffering setting.

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

No branches or pull requests

4 participants