Skip to content

Handle exceptions and throwables in templates#29

Merged
akrabat merged 2 commits into
slimphp:masterfrom
jeremyVignelles:template-exception
Oct 11, 2016
Merged

Handle exceptions and throwables in templates#29
akrabat merged 2 commits into
slimphp:masterfrom
jeremyVignelles:template-exception

Conversation

@jeremyVignelles

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

@jeremyVignelles

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

@geggleto

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

Any updates on this?

@akrabat

akrabat commented May 9, 2016

Copy link
Copy Markdown
Member

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

@jeremyVignelles

Copy link
Copy Markdown
Contributor Author

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

geggleto commented May 9, 2016

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Contributor Author

@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 😃

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

geggleto commented May 9, 2016

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

@geggleto

Copy link
Copy Markdown
Member

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

@geggleto geggleto self-assigned this Sep 12, 2016
@akrabat akrabat merged commit e6bf98a into slimphp:master Oct 11, 2016
@akrabat akrabat added this to the 2.2 milestone Oct 11, 2016
@jeremyVignelles jeremyVignelles deleted the template-exception branch October 11, 2016 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants