- Download Diff

Summary:

[Corona] Catch exceptions in the FrontController

Review Request #554 — Created May 7, 2018 and submitted July 6, 2018, 2:41 p.m.

Information
Owner:	pprkut
Repository:	Lunr
Branch:	pprkut:exceptions
Bugs:
Depends On:
Blocks:	576
Commit:	df96749...
Reviewers
Groups:	lunr
People:

Description

Catch exceptions in a RequestResultHandler class, which can be used by either a FrontController or a Controller class.
The RequestResultHandler sets the proper response codes, which should remove boiler plate code from Controllers.

Testing Done

Unit tests

Issues

0
0
13
3
All Issues: 16

Description	From	Last Updated
why?	smillernl	May 28, 2018, 5:36 p.m.
Style	smillernl	May 28, 2018, 5:36 p.m.
style	smillernl	May 28, 2018, 5:36 p.m.
style	smillernl	May 28, 2018, 5:36 p.m.
to support application specific error, we may create a specific constructor here which would allow an application_error_code parameter	tardypad	June 15, 2018, 10:52 a.m.
style	smillernl	May 28, 2018, 5:36 p.m.
style	smillernl	May 28, 2018, 5:36 p.m.
style	smillernl	May 28, 2018, 5:36 p.m.
style	smillernl	May 28, 2018, 5:36 p.m.
I didn't understand the review comment about the need to make this a Trait 1st: just to confirm the motivation ...	tardypad	May 28, 2018, 5:36 p.m.
This looks weird now: a public function just calling an private function, which is itself only meant for this one. ...	tardypad	May 28, 2018, 5:36 p.m.
I'd imagine we'd want to know the exception class here. Maybe make it like$e::class . ': ' . $e->getMessage();	smillernl	June 15, 2018, 9:42 a.m.
$code should be renamed to $app_code as in the HTTPException class Otherwise this is quite confusing Same in other exception ...	tardypad	June 15, 2018, 5:37 p.m.
This is not matching the code above in the set_result function where no application code is expected to be represented ...	tardypad	June 15, 2018, 5:44 p.m.
maybe a better specific naming would be better here request_result_handler	tardypad	June 15, 2018, 5:45 p.m.
not sure I understand why you put it as "final". What problems can happen if it would be extended?	tardypad	June 15, 2018, 5:37 p.m.

Change Summary:

Fix summary

Summary:

-	[Corona] Add RequestResultHandlerTrait
+	[Corona] Catch exceptions in the FrontController

src/Lunr/Corona/Exceptions/BadRequestException.php (Diff revision 1)
Show all issues
```
why?
```
src/Lunr/Corona/Exceptions/BadRequestException.php (Diff revision 1)
Show all issues
```
Style
```
src/Lunr/Corona/Exceptions/ForbiddenException.php (Diff revision 1)
Show all issues
```
style
```
src/Lunr/Corona/Exceptions/HttpException.php (Diff revision 1)
Show all issues
```
style
```
src/Lunr/Corona/Exceptions/MethodNotAllowedException.php (Diff revision 1)
Show all issues
```
style
```
src/Lunr/Corona/Exceptions/NotFoundException.php (Diff revision 1)
Show all issues
```
style
```
src/Lunr/Corona/Exceptions/NotImplementedException.php (Diff revision 1)
Show all issues
```
style
```
src/Lunr/Corona/Exceptions/PreconditionFailedException.php (Diff revision 1)
Show all issues
```
style
```

src/Lunr/Corona/RequestResultHandlerTrait.php (Diff revision 1)

Show all issues

I'd imagine we'd want to know the exception class here. Maybe make it like$e::class . ': ' . $e->getMessage();

pprkut June 15, 2018, 9:42 a.m.

I'm fine seeing exception names in log messages, but don't want to see them in the JSON output of webservices, so not here at least.

src/Lunr/Corona/Exceptions/HttpException.php (Diff revision 1)

Show all issues

to support application specific error, we may create a specific constructor here which would allow an application_error_code parameter

pprkut May 17, 2018, 4:54 p.m.

Yes, indeed. We don't even need a special constructor, we just jave to not use a special constructor for the child classes.
We do need a special getter method here though.

src/Lunr/Corona/FrontController.php (Diff revision 1)

Show all issues

I didn't understand the review comment about the need to make this a Trait
1st: just to confirm the motivation behind not making that exception handling and result setting directly in the FrontController class

Is it to be flexible and leave the choice of the result handling to the "project" using it?
2nd: I am not a big fan of using traits.

They caused too many implementations and testing issues in the past.

I don't see much of their benefits over using a normal RequestResultHandler class there

pprkut May 17, 2018, 4:54 p.m.

1st
No, it's to allow request handling also outside of the FrontController. Basically, everywhere where you mock request values you want to catch exceptions and set response values.
2nd
Me neither, but in this case is seemed like the better option. It's not really meant to be injectable or overrideable. If you need additional handling of something you can still do that

in the controller. We could probably get the same behavior by making the class final. That would leave the need for a locator recipe, but that wouldn't be too bad. I'll think that over.

src/Lunr/Corona/FrontController.php (Diff revision 1)

Show all issues

This looks weird now: a public function just calling an private function, which is itself only meant for this one.

We may actually now better move that whole function into the Handler itself, no?
Not sure why you don't pass the request params as a parameter directly for example too.

pprkut May 17, 2018, 4:54 p.m.

dispatch() is really specific to the FrontController. You wouldn't call that in a controller. handle_request() encapsulates the common parts.
I'm fine with passing the params as arguments here though.

Change Summary:

Trait -> class
pass params to handle_request

Commit:

-	d75e0b4bc867b22bab4fa99a24e850199daf802a
+	efa488f84f89737de6bfad96bd2db318501ad697

Diff:

Revision 2 (+1480 -32)

Show changes

	src/Lunr/Corona/FrontController.php
	src/Lunr/Corona/RequestResultHandler.php
	src/Lunr/Corona/Exceptions/BadRequestException.php
	src/Lunr/Corona/Exceptions/ForbiddenException.php
	src/Lunr/Corona/Exceptions/HttpException.php
	src/Lunr/Corona/Exceptions/MethodNotAllowedException.php
	src/Lunr/Corona/Exceptions/NotFoundException.php
	src/Lunr/Corona/Exceptions/NotImplementedException.php
	20 more

Change Summary:

Add support for application error codes

Description:

~		== PROOF OF CONCEPT ==
~
	~	Catch exceptions in a RequestResultHandler class, which can be used by either a FrontController or a Controller class.
	~	The RequestResultHandler sets the proper response codes, which should remove boiler plate code from Controllers.
-		Still not entirely sure this is the exact way we want to do it. Application specific error codes are, for example, not yet considered in here.
-
-		It's implemented as a trait, so you can reuse the logic in controllers that make further requests.

Commit:

-	efa488f84f89737de6bfad96bd2db318501ad697
+	92563880891ba3f7ff24f0a7024b7e679c038f6b

Diff:

Revision 3 (+1859 -32)

Show changes

	src/Lunr/Corona/FrontController.php
	src/Lunr/Corona/RequestResultHandler.php
	src/Lunr/Corona/Exceptions/BadRequestException.php
	src/Lunr/Corona/Exceptions/ForbiddenException.php
	src/Lunr/Corona/Exceptions/HttpException.php
	src/Lunr/Corona/Exceptions/MethodNotAllowedException.php
	src/Lunr/Corona/Exceptions/NotFoundException.php
	src/Lunr/Corona/Exceptions/NotImplementedException.php
	23 more

src/Lunr/Corona/Exceptions/BadRequestException.php (Diff revision 3)

Show all issues

$code should be renamed to $app_code
as in the HTTPException class
Otherwise this is quite confusing

Same in other exception sub classes

src/Lunr/Corona/Exceptions/HttpException.php (Diff revision 3)

Show all issues

This is not matching the code above in the set_result function where no application code is expected to be represented by NULL

pprkut June 15, 2018, 5:06 p.m.

Every HttpException will return an application error code != 0, and any other Exception will not pass a value at all to set_result() (hence the NULL default)
Essentially, the application error code is an integer for exceptions and a nullable integer in set_result(). Not seeing the problem.

tardypad June 15, 2018, 5:25 p.m.

With the current code, if I throw a BadRequestException without any app code, it will lead to an error_info being set in the result to 400
while we don't want to have this value set

pprkut June 15, 2018, 5:37 p.m.

It's the current behavior as well, it just happens elsewhere (in the JSONView/MsgpackView). I just figured it's easier/better to handle here than there. But if you think there's a case for keeping the application code NULL I'm listening :)

src/Lunr/Corona/FrontController.php (Diff revision 3)

Show all issues

maybe a better specific naming would be better here
request_result_handler

pprkut June 15, 2018, 5:06 p.m.

I picked $handler since anything else seemed awefully repetitive. $this->request_result_handler->handle_request().... meh.

IMHO this is a non-issue until there's more than one possible option.

tardypad June 15, 2018, 5:25 p.m.

Whatever indeed

I'd say the problem is mainly the naming of the handle_request function itself

but I couldn't come up with something better than vague names such as "run" or "process" (which is what the function is also doing)

pprkut June 15, 2018, 5:37 p.m.

Not opposed to using process() if you think that's better

src/Lunr/Corona/RequestResultHandler.php (Diff revision 3)

Show all issues

not sure I understand why you put it as "final".
What problems can happen if it would be extended?

pprkut June 15, 2018, 5:37 p.m.

It's mostly an indicator that the class shouldn't need extending. It's really generic, and if you need different behavior it's likely you need completely different behavior, in which case you'd replace it entirely anyway. Anyway, I dropped the final classifier.

Change Summary:

Fix parameter name and drop final classifier.

Commit:

-	92563880891ba3f7ff24f0a7024b7e679c038f6b
+	df967494bf4715ae7ae9f88ec0d70ef69ac1c1b4

Diff:

Revision 4 (+1857 -32)

Show changes

	src/Lunr/Corona/FrontController.php
	src/Lunr/Corona/RequestResultHandler.php
	src/Lunr/Corona/Exceptions/BadRequestException.php
	src/Lunr/Corona/Exceptions/ForbiddenException.php
	src/Lunr/Corona/Exceptions/HttpException.php
	src/Lunr/Corona/Exceptions/MethodNotAllowedException.php
	src/Lunr/Corona/Exceptions/NotFoundException.php
	src/Lunr/Corona/Exceptions/NotImplementedException.php
	23 more

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Corona] Catch exceptions in the FrontController

Change Summary:

Summary:

1st

2nd

Change Summary:

Commit:

Diff:

Change Summary:

Description:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: