themosis / framework

The Themosis framework core.
https://framework.themosis.com/
GNU General Public License v2.0
672 stars 121 forks source link

Allow redirects on admin pages - Allow responses instances as a returned value #716

Closed Jaspervv closed 2 years ago

Jaspervv commented 5 years ago

Problem

I was interested in creating an admin page that redirects to another page, however returning a redirect throws the following error:

// themosis/framework/src/Page/Page.php:896
The controller method must return a view instance.

Code

use Themosis\Support\Facades\Page;

$page = Page::make('test-redirect', 'Home')
    ->set();

$page->route('/', function()
{
    return redirect('/');
});
pvdptje commented 5 years ago

For admin pages, use wp_safe_redirect instead: wp_safe_redirect(add_query_arg(['page' => $page], admin_url('admin.php')));

Jaspervv commented 5 years ago

use wp_safe_redirect instead

returning nothing, or returning wp_safe_redirect still throws an error because the callback is required to return a view.

It would be possible to call wp_safe_redirect, and then return a view.

$page->route('/', function()
{
    wp_safe_redirect('/');
    return view('some.existing.view');
});

(the redirect happens first, so the view will never be shown)

But I think these routes shouldn't be enforced to return a renderable instance.

pvdptje commented 5 years ago

I would prefer an exit(); instead of a view.

Jaspervv commented 5 years ago

I would prefer an exit(); instead of a view.

Exit is probably the best temp solution, yes.

jlambe commented 5 years ago

@Jaspervv definitely an issue or at least we can improve admin pages. The only object we should return is a Response object or at least a class that extends it.

Because on the administration, WordPress is throwing all the headers way before hitting the Page API, we should take care to only use the content part of a returned response object. In the case of a RedirectResponse, I'm also not sure it will work as headers are already sent at that time.

So doing a wp_safe_redirect() followed by an exit() call should be better.

My curiosity question is why are you trying to redirect on a GET request ? :)

Jaspervv commented 5 years ago

My curiosity question is why are you trying to redirect on a GET request ? :)

We created a header&footer customisation page, and we wanted a Menus submenu that redirects to WordPress's /wp-admin/nav-menus.php.
Is this bad practice or smth?

jlambe commented 5 years ago

I don't know if it is bad practice but as in the end there is redirect, the page is clearly not showing anything. In your case a user end-up doing 2 requests to access the WordPress menus. Wondering if it's not best to filter the global $menus (<- not sure about variable name but it exists) to insert the link directly under your main page.

Anyway there is clearly an enhancement to do on the admin pages.

Jaspervv commented 5 years ago

@jlambe Thanks, I'll check it out. :)
This issue can be closed but if you want to keep it open as a reminder then feel free to.

jlambe commented 5 years ago

On GET requests, we can only echo a Response content. On POST requests, we can echo a Response headers and content.

-> So this means that when defining a post route for an admin page, the controller could return a RedirectResponse instance...

jlambe commented 2 years ago

@Jaspervv I'm late but redirect responses are coming on upcoming 3.0 release. It will be possible to return redirect responses from both GET and POST admin requests.