thephpleague / route

Fast PSR-7 based routing and dispatch component including PSR-15 middleware, built on top of FastRoute.
http://route.thephpleague.com
MIT License
651 stars 126 forks source link

3.x, trying to use custom strategy not "getting it". #225

Closed alturic closed 5 years ago

alturic commented 5 years ago

TL;DR: How can I edit all of the exceptions $message property when they are thrown?


So, I'm trying to, honestly, simply override the message returned with each of the exceptions. I know I could do something nasty like edit the package directly but obviously that's never going to be proper.

What I did try doing was implement a CustomJsonStrategy, and copy the JsonStrategy, and all of the exceptions and Exception itself over verbatim (updating the namespaces of course) but in my custom strategy updated the namespaces for these copied files.

However, it's not that simple as my CustomJsonStrategy complains that Declaration must be compatible with StrategyInterface->getNotFoundDecorator(exception : \League\Route\Http\Exception\NotFoundException)

public function getNotFoundDecorator(NotFoundException $exception)
    {
        return function (ServerRequestInterface $request, ResponseInterface $response) use ($exception) {
            return $exception->buildJsonResponse($response);
        };
    }

Now, I know what I did (for what I was trying to do, simply change the $message for each of the exceptions), is probably nowhere near correct, but I am still trying to understand why exactly it doesn't work? I copied the contents of JsonResponse, Exception.php and all of the Exception/*.php files, (updating the namespacing correctly to point to these files) but it doesn't seem to work that way? Primarily confused as to why JsonStrategy works perfect (duh), but my copy/paste/update namespace's throws the errors? I imagine it's due to what the $route->setStrategy() method actually "does" but curious none-the-less.

philipobenito commented 5 years ago

To better answer your question it might be better if you explain why you want to change the messages or what the ultimate goal of changing them is?

Sent from my iPhone

On 26 Dec 2018, at 19:04, alturic notifications@github.com wrote:

TL;DR: How can I edit all of the exceptions $message property when they are thrown?

So, I'm trying to, honestly, simply override the message returned with each of the exceptions. I know I could do something nasty like edit the package directly but obviously that's never going to be proper.

What I did try doing was implement a CustomJsonStrategy, and copy the JsonStrategy, and all of the exceptions and Exception itself over verbatim (updating the namespaces of course) but in my custom strategy updated the namespaces for these copied files.

However, it's not that simple as my CustomJsonStrategy complains that Declaration must be compatible with StrategyInterface->getNotFoundDecorator(exception : \League\Route\Http\Exception\NotFoundException)

public function getNotFoundDecorator(NotFoundException $exception) { return function (ServerRequestInterface $request, ResponseInterface $response) use ($exception) { return $exception->buildJsonResponse($response); }; } Now, I know what I did (for what I was trying to do, simply change the $message for each of the exceptions, is probably nowhere near correct, but I am still trying to understand why exactly it doesn't work? I copied the contents of JsonResponse, Exception.php and all of the Exception*.php files, (updating the namespacing correctly to point to these files) but it doesn't seem to work that way? Primarily confused as to why JsonStrategy works perfect (duh), but my copy/paste/update namespace's throws the errors? I imagine it's due to what the $route->setStrategy() method actually "does" but curious none-the-less.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

alturic commented 5 years ago

With a question of "why", I feel like it's not something that's easily achievable/doable... ha. As to the "Why"?

Well, the "Not found" message, to me, would make more sense to have it be "Not a valid endpoint". This is a simple matter of taste, I get that. Perhaps a better question would be, how can I add/replace any exception class? Say I didn't want to use the NotFoundException that's included? Say I wanted to also add some logging to a NotFoundException, etc. That's why I thought simply copying JsonStrategy and the League\Route\Http\Exception*.php would literally do that. I am green with OOP though. :(

I don't mean that response as facetious, or trying to be over simplistic or anything of that nature. Honestly though, changing the default messages is literally all I'm really looking to do though.

Don't get me wrong, it would be nice to be able to have said CustomJsonStrategy, and then be able to do something like throw new InvalidCredentialsException in a controller. See that's where I'm honestly confused. I'm fairly certain I can throw new ImATeapotException('I am not a teapot') and it would? overwrite the default $message associated with the ImATeapotException that the package has? If so, making new exceptions seems fairly simple, extend HttpException and you'd be good to go? However, editing, at the very least, the getNotFoundDecorator and getMethodNotAllowedDecorator seems to be "different".

Honestly, I thought that's what these decorators (in a proper implementation, not how I tried it, although I'm still very eager to know why copy/pasting these classes verbatim doesn't work the same way) allowed for? Changing the messages, or status codes associated with the packages included exceptions?

philipobenito commented 5 years ago

I’ll preface this with the fact that I do not recommend what you’re planning to do, it adds a layer of complexity that heavily outweighs any benefit, as any client shouldn’t care about the message.

That being said, it is doable with a custom strategy, I’m not at a computer for a few days though so will come back to you with an example when I am.

Sent from my iPhone

On 26 Dec 2018, at 20:28, alturic notifications@github.com wrote:

With a question of "why", I feel like it's not something that's easily achievable/doable... ha. As to the "Why"?

Well, the "Not found" message, to me, would make more sense to have it be "Not a valid endpoint". I don't mean that response as facetious, or trying to be over simplistic or anything of that nature. Honestly though, changing the default messages is literally all I'm really looking to do though.

Don't get me wrong, it would be nice to be able to have said CustomJsonStrategy, and then be able to do something like throw new InvalidCredentialsException in a controller. See that's where I'm honestly confused. I'm fairly certain I can throw new ImATeapotException('I am not a teapot') and it would? overwrite the default $message associated with the ImATeapotException that the package has? If so, making new exceptions seems fairly simple, extend HttpException and you'd be good to go? However, editing, at the very least, the getNotFoundDecorator and getMethodNotAllowedDecorator seems to be "different".

Honestly, I thought that's what these decorators (in a proper implementation, not how I tried it, although I'm still very eager to know why copy/pasting these classes verbatim doesn't work the same way) allowed for? Changing the messages, or status codes associated with the packages included exceptions?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

alturic commented 5 years ago

Let me ask this, and this is primarily due to reading other various questions (funnily enough, one revolving around 404's) where the answer was "Because this isn't a framework, it makes no assumptions about how the user should handle the 404.", how would you "intercept" the packages internal NotFoundException and use one of your own? I ask that with the logic being, using the packages included exceptions, they use 'status_code' for the status code. Almost, forcing you to use the same naming convention for your entire app then.

Again, not being facetious (I love the package), nor being overly obtuse or simplistic, but just trying to understand.

Edit: Things like that is what I'm looking to achieve. Changing the naming conventions used in responses, the messages returned, etc.

philipobenito commented 5 years ago

ApplicationStrategy exceptions just bubble out so you can catch those, JsonStrategy builds a response so not as simple as that but you’d extend the JsonStrategy and overload the decorator methods.

As for the body of a 404 response, it’s there for trivial reasons, you’d get the response code from the response status code itself, not the representation of that in the body.

Sent from my iPhone

On 26 Dec 2018, at 20:55, alturic notifications@github.com wrote:

Let me ask this, and this is primarily due to reading other various questions (funnily enough, one revolving around 404's) where the answer was "Because this isn't a framework, it makes no assumptions about how the user should handle the 404.", how would you "intercept" the packages internal NotFoundException and use one of your own? I ask that with the logic being, using the packages included exceptions, they use 'status_code' for the status code. Almost, forcing you to use the same naming convention for your entire app then.

Again, not being facetious (I love the package), nor being overly obtuse or simplistic, but just trying to understand.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

alturic commented 5 years ago

Philip,

Thanks for the replies. I get your responses, somewhat, and perhaps I'm making horrible examples (status_code for one heh), but I hope my main point is coming across of how to customize the responses, etc. Perhaps I will try ApplicationStrategy and wrap the ->dispatch in a long try/catch block trying to handle the exceptions.

I could go full silly and just remove the dependancy and put it directly in my app (as I'm fresh with OOP heh) but ugh I didn't want to do that.