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

Add RedirectMiddleware #14384

Closed
wants to merge 2 commits into from
Closed

Add RedirectMiddleware #14384

wants to merge 2 commits into from

Conversation

dereuromark
Copy link
Member

In 3.x we could use a hack with $reponse->send() and exit(); to make redirects from within the components and alike.
In 4.x this is not possible anymore, a cleaner approach is using the RedirectException available already.

But there is a core Middleware missing for handling this.

I added one on project level as demo here:
dereuromark/cakephp-sandbox@b4fb863

You can see how it can be used
https://sandbox.dereuromark.de/sandbox/search-examples/table?page=12 (200 OK)
But when you hit the next page usually, you get a hard exception
This is not very user-friendly, as often, just an item got deleted at the end and thus the page count is one less. So if userland code wants to easily implement a paginator that does this, such middleware is vital here.
https://sandbox.dereuromark.de/sandbox/search-examples/table?page=13 (302 redirect)
This one now redirects to the last page on its own.

With this change, it is easier inside apps to leverage the given exception now.

* @param \Psr\Http\Server\RequestHandlerInterface $handler The request handler.
* @return \Psr\Http\Message\ResponseInterface A response.
*/
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a Middleware for this? Handling RedirectException seems like behavior we could build into ControllerFactory::invoke() or into Http/Server. If we handled it in Http/Server we would need to update the ErrorHandlerMiddleware to not swallow those errors which is a bit clunky and not idea in my opinion.

Copy link
Member Author

@dereuromark dereuromark Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also fine with that :)
This was just my first idea. And this way it is also opt-in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing the exception from a component or controller class is another opt-in mechanism 😄

One drawback to handing RedirectException in the ControllerFactory is that RedirectExceptions raised in middleware won't work the same. Perhaps the ErrorHandlerMiddleware is the best place for these exceptions to be handled. It already handles lots of other kinds of exceptions, and this one has a pretty straightforward action to take.

Should we also expand the RedirectException to allow headers to be added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine if someone draws up a better alternative PR.

@markstory markstory self-assigned this Mar 24, 2020
markstory added a commit that referenced this pull request Mar 28, 2020
An alternative to #14384. I've added a new RedirectException class that
isn't coupled to Routing package and is integrated with the
ErrorHandlerMiddleware which enables any
middleware/controller/component to raise a RedirectException and get the
intended effect.
@dereuromark
Copy link
Member Author

With #14397 continuing this I will close this one.

@dereuromark dereuromark deleted the redirect-middleware branch March 29, 2020 01:18
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.

2 participants