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

Bug: Route redirection not working #3041

Closed
MichalPB1 opened this issue May 27, 2020 · 16 comments
Closed

Bug: Route redirection not working #3041

MichalPB1 opened this issue May 27, 2020 · 16 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@MichalPB1
Copy link

Describe the bug
After using

$routes->addRedirect('example', 'profile/dashboard');

I get an error

key() expects parameter 1 to be array, string given

but if the second param I set as named route then everything is working properly. The problem exists only when the second param is a URI

CodeIgniter 4 version
4.0.2

Affected module(s)

Router/Router.php at line 430

I not sure but here I found some changes for this line https://github.com/codeigniter4/CodeIgniter4/pull/2481/files#diff-d2374d65fd714c18dbee9f5adaa8dd6cL488

Expected behavior, and steps to reproduce if appropriate
Redirection working properly for the second param as a URI .

Context

  • OS: Ubuntu
  • PHP version 7.3
@MichalPB1 MichalPB1 added the bug Verified issues on the current code behavior or pull requests that will fix them label May 27, 2020
@nyufeng
Copy link
Contributor

nyufeng commented May 28, 2020

This problem is because profile/dashboard not set in $routes. I will fix.

@MichalPB1
Copy link
Author

@Instrye thx for your answer :) but it not true. That route is already defined and if I change it to route name everything works.

I think that redirect should work even that URI is not defined - you don't think so? We could have a situation that redirection to not existed URI redirect it to the next URI.

@MichalPB1
Copy link
Author

It's more complicated :) It looks like redirection doesn't work for addresses with / eg checkout/shoppingCart, profile/orders when route name is defined

Now the redirection works

$routes->addRedirect('asdasd', 'checkout/shoppingCart', 301);

$routes->get('shoppingCart', 'CheckoutController::shoppingCart');

Now the redirection does not work:

$routes->addRedirect('asdasd', 'checkout/shoppingCart', 301);

$routes->get('shoppingCart', 'CheckoutController::shoppingCart', ['as' => 'checkout.shoppingCart']);

@nyufeng
Copy link
Contributor

nyufeng commented May 28, 2020

I just use $routes->add to test, This looks bad.

Regarding your second question, I think you are right, I have made a change

@nyufeng
Copy link
Contributor

nyufeng commented May 29, 2020

It's more complicated :) It looks like redirection doesn't work for addresses with / eg checkout/shoppingCart, profile/orders when route name is defined

Now the redirection works

$routes->addRedirect('asdasd', 'checkout/shoppingCart', 301);

$routes->get('shoppingCart', 'CheckoutController::shoppingCart');

Now the redirection does not work:

$routes->addRedirect('asdasd', 'checkout/shoppingCart', 301);

$routes->get('shoppingCart', 'CheckoutController::shoppingCart', ['as' => 'checkout.shoppingCart']);

This problem is because addRediret before setRoutes, you can run

$routes->get('shoppingCart', 'CheckoutController::shoppingCart');
$routes->addRedirect('asdasd', 'checkout/shoppingCart', 301);

I think this is the correct order

@MichalPB1
Copy link
Author

MichalPB1 commented May 29, 2020

You're right. After changing the routes order is a bit better but still, there are few bugs(?):

  • cannot redirect to checkout/shoppingCart if that route has set the name. It a forcing for us using checkout.shoppingCart route name - this is not practical if we manage redirection from eg database
  • cannot redirect to another redirect
  • setting redirection code is not working, always get 307 despite the fact that I set 301
  • I think that we should be able to redirect the existing route in the system to another. For example, I could change the login to register - it's an often practice if we have a login and register form on one view

or I am still doing something a wrong way?

@nyufeng
Copy link
Contributor

nyufeng commented May 29, 2020

You're right. After changing the routes order is a bit better but still, there are few bugs(?):

  • cannot redirect to checkout/shoppingCart if that route has set the name. It a forcing for us using checkout.shoppingCart route name - this is not practical if we manage redirection from eg database
  • cannot redirect to another redirect
  • setting redirection code is not working, always get 307 despite the fact that I set 301
  • I think that we should be able to redirect the existing route in the system to another. For example, I could change the login to register - it's an often practice if we have a login and register form on one view

or I am still doing something a wrong way?

I think, These problems already processed

@MichalPB1
Copy link
Author

Ok, I tested your code from PR and:

  1. Yes, that works :)
  2. Yes, that works :)
  3. I found in HTTP/Response.php at line 784 that response code is overridden for a method other than refresh, but that method is forced as auto in Codeigniter.php at line 315 - is it a correct?
  4. I have a problem with redirect login to register. It looks like the URL === route name is a problem and we are redirection to http://localhost:8082/\App\Controllers\AuthController::login
$routes->add('login', 'AuthController::login', ['as' => 'login']);

@nyufeng
Copy link
Contributor

nyufeng commented Jun 1, 2020

Ok, I tested your code from PR and:

  1. Yes, that works :)
  2. Yes, that works :)
  3. I found in HTTP/Response.php at line 784 that response code is overridden for a method other than refresh, but that method is forced as auto in Codeigniter.php at line 315 - is it a correct?
  4. I have a problem with redirect login to register. It looks like the URL === route name is a problem and we are redirection to http://localhost:8082/\App\Controllers\AuthController::login
$routes->add('login', 'AuthController::login', ['as' => 'login']);

The first parameter is the URI pattern for the old route. The second parameter is either the new URI to redirect to, or the name of a named route.

So, Second Parameter is URI OR named, Class::name not support

Another problem is not what I am good at, But code annotate seems to give a reason, You can see : https://en.wikipedia.org/wiki/Post/Redirect/Get

@MichalPB1
Copy link
Author

So, Second Parameter is URI OR named, Class::name not support

I know that Class::name is not supported. The example was not a redirection but route definition so

  1. Define a route URI the same as a the route name
$routes->add('login', 'AuthController::login', ['as' => 'login']);
  1. Add a redirection from eg login to another url eg register
$routes->addRedirect('login', 'register', 301);
  1. You are redirected to http://localhost:8082/\App\Controllers\AuthController::login wchich is a wrong URI

That problem is only when the URI is the same as the route name

Another problem is not what I am good at, But code annotate seems to give a reason, You can see : https://en.wikipedia.org/wiki/Post/Redirect/Get

Yes, I saw that. But I not able to create a 301 redirection via $routes->addRedirect? So how can I do it?

@nyufeng
Copy link
Contributor

nyufeng commented Jun 2, 2020

You first problem I can't recurrent.
In my test. If:

$routes->addRedirect('login', 'register');
$routes->add('login', 'AuthController::login', ['as' => 'login']);

Location: http://example.com/register
View: Controller or its method is not found: \App\Controllers\Register::index
If:

$routes->add('login', 'AuthController::login', ['as' => 'login']);
$routes->addRedirect('login', 'register', 302);

Location: http://localhost/login
View: Controller or its method is not found: \App\Controllers\AuthController::login

@MichalPB1
Copy link
Author

@Instrye forgive me, I gave you wrong code :/

There should be a get method not an add method

$routes->get('login', 'AuthController::login', ['as' => 'login']);
$routes->get('register', 'AuthController::register', ['as' => 'register']);
$routes->addRedirect('login', 'register');

or

$routes->addRedirect('login', 'register');
$routes->get('login', 'AuthController::login', ['as' => 'login']);
$routes->get('register', 'AuthController::register', ['as' => 'register']);

We are redirected to location \App\Controllers\AuthController::login

@nyufeng
Copy link
Contributor

nyufeng commented Jun 8, 2020

@Instrye forgive me, I gave you wrong code :/

There should be a get method not an add method

$routes->get('login', 'AuthController::login', ['as' => 'login']);
$routes->get('register', 'AuthController::register', ['as' => 'register']);
$routes->addRedirect('login', 'register');

or

$routes->addRedirect('login', 'register');
$routes->get('login', 'AuthController::login', ['as' => 'login']);
$routes->get('register', 'AuthController::register', ['as' => 'register']);

We are redirected to location \App\Controllers\AuthController::login

The problem because custom router replace. Router must be only.

@MichalPB1
Copy link
Author

MichalPB1 commented Jun 8, 2020

The problem because custom router replace. Router must be only.

Sorry, but I don't understand. Can you explain what you mean?

@nyufeng
Copy link
Contributor

nyufeng commented Jun 11, 2020

First, you set login is AuthController::login. Then, you set login is redirect to register. It was set twice, which caused a conflict.
I know what I said is difficult to understand because it is not my mother tongue, please forgive me.

@michalsn
Copy link
Member

@Instrye is right.

It seems to me like everything is sorted out already. If not, please feel free to open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

3 participants