slimphp / PHP-View

A Simple PHP Renderer for Slim 3 & 4 (or any other PSR-7 project)
MIT License
263 stars 60 forks source link

Handle exceptions and throwables in templates #29

Closed jeremyVignelles closed 8 years ago

jeremyVignelles commented 8 years ago

When an exception is thrown in the template, the output buffer should be cleaned up, so that a clean error message is shown to the user.

This PR handles PHP7 Throwables as well as being PHP 5.6 compatible.

geggleto commented 8 years ago

@jeremyVignelles This is going to be tricky. Slim 3 currently supports all the way down to 5.5 ... //ping @codeguy @akrabat

jeremyVignelles commented 8 years ago

I can't figure out any reason why this wouldn't be compatible with 5.5, though I haven't tested. I just couldn't install PHPUnit on PHP 5.5, and I decided to test through a docker container on PHP 5.6 and PHP 7.

geggleto commented 8 years ago

I have an open PR with Travis hooked up... I might just merge it if someone else doesn't

geggleto commented 8 years ago

My only question with this is if its BC with 5.5 because I did not think that Throwable existed in 5.5, 5.6.

jeremyVignelles commented 8 years ago

Just updated my branch to follow current master state.

I found the logic in this article : https://trowski.com/2015/06/24/throwable-exceptions-and-errors-in-php7/ at title "Writing Code to Support PHP 5.x and 7 Exceptions" The catch(Throwable $e) does not match in PHP 5.x, but does not crash either.

jeremyVignelles commented 8 years ago

Any updates on this?

akrabat commented 8 years ago

Does this mean the any var_dumps before the exception are now longer visible?

jeremyVignelles commented 8 years ago

Yes. In fact, my current issue is that my PHP template starts rendering the page until the exception. Then, the exception is thrown, and the exception handler is executed, which renders another page, but does not "clean" the current output buffer, and thus appends the content, which renders a very ugly page...

Is that a problem to hide the var_dump? These are only for debugging purpose right?

geggleto commented 8 years ago

@jeremyVignelles are you using the Slim' provided Error handler? I believe it wipes the contents of the Response object or creates a brand new one to display the error.

If you are using your own Exception handling, It would be best to wipe the contents of the response, render whatever you need to and set status code 500.

This PR wont stop the ugly templates if someone renders multiple templates in sequence.

jeremyVignelles commented 8 years ago

@geggleto I am using my own error handler, inspired from the example provided in the official user guide : http://www.slimframework.com/docs/handlers/error.html I also read the official ErrorHandler code, but I didn't see any ob_ function in it, maybe I am missing something.

The question is more a matter of conception : Is it better to have an isolated PHPRenderer, that always cleans after itself, even in case of exception, or is it better to have the application handle these cases? It's up to you :smiley:

This PR wont stop the ugly templates if someone renders multiple templates in sequence.

You are right, but it's up to the developer to know what he is doing. But there's a scenario this can be useful: If you want to render templates in strings (which seems to be supported by this module) If the first template crashes, the second template may still run if the exception is handled properly. By default, if the developper doesn't clean the output buffer, the second template will have the first template result included, which is not very handy.

geggleto commented 8 years ago

Yeah I leaning towards merging but I need some more time to think about all the possibilities of it :)

Maybe some more test cases are needed as well.

jeremyVignelles commented 8 years ago

OK, tell me if you want me to try something or implement a new test case. By the way, did you manage to test on PHP 5.5?

jeremyVignelles commented 8 years ago

Hi, Did you have time to take a look at this?

geggleto commented 8 years ago

Sorry @jeremyVignelles Yes, it looks okay. will look at merging this week.