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

URI Routing bug #1354

Closed
demortx opened this issue Oct 25, 2018 · 15 comments
Closed

URI Routing bug #1354

demortx opened this issue Oct 25, 2018 · 15 comments

Comments

@demortx
Copy link

demortx commented Oct 25, 2018

After the upgrade, it stopped working, $routes->post

Example if declared
$ routes-> add ('auth', 'Main :: index');
$ routes-> post ('auth', 'Main :: auth_post');

That post stops working, always redirects to
$routes->add('auth', 'Main :: index');

ignoring
$routes->post('auth', 'Main :: auth_post');

@jim-parry
Copy link
Contributor

This sounds like correct behavior to me.
Routing is based on first match, so your "post" rule will be not be used.
Try reversing the order of your rules.
See https://bcit-ci.github.io/CodeIgniter4/incoming/routing.html#resource-routes

This is a support question/problem, and should be raised on the forum, thanks.

@demortx
Copy link
Author

demortx commented Oct 25, 2018

the order has changed, the result has not changed, until the update worked properly, this is clearly an error

@jim-parry jim-parry reopened this Oct 25, 2018
@jim-parry
Copy link
Contributor

Ok - i will leave this one for Lonnie.

@demortx
Copy link
Author

demortx commented Oct 25, 2018

note that the url is the same, only the methods differ

@jim-parry
Copy link
Contributor

You say "after the upgrade" ... what are you referring to?
The last change to Router/Router was in August, a month before alpha.1

@lonnieezell
Copy link
Member

There was previously an error with correctly recognizing methods when an add function was used in combination with any of the other verbs (get, put, etc). When that was fixed, this should be working, but I'd be curious what change you are referring to also.

@jim-parry
Copy link
Contributor

btw, a quick unit test case returns "index" as the method, regardless of the order, and with the route collections's verb set to 'post'.

@jim-parry
Copy link
Contributor

jim-parry commented Oct 25, 2018

I would guess PR1285 on Oct 2nd, changed RouteCollection

@jim-parry
Copy link
Contributor

/**
 * @see https://github.com/bcit-ci/CodeIgniter4/issues/1354
 */
public function testRouteOrder()
{
	$this->collection->setHTTPVerb('post');

	$this->collection->post('auth', 'Main::auth_post');
	$this->collection->add('auth', 'Main::index');

	$router = new Router($this->collection);

	$router->handle('auth');
	$this->assertEquals('\Main', $router->controllerName());
	$this->assertEquals('auth_post', $router->methodName());

}

Adding the above to RouterTest, it fails, with the actual method returned "index".

@demortx
Copy link
Author

demortx commented Oct 25, 2018

da43a39

Rolled back this fix and it all worked.
$collection = array_merge($this->routes['*'], $this->routes[$verb]);

@lonnieezell
Copy link
Member

I'm going to guess we need to swap the order of the values in the array_merge, but not sure when I'll have a chance to look at that currently. Work is keeping me going with long days. :(

@jim-parry
Copy link
Contributor

hmm - if referring to the merge in getRoutes(), swapping the order breaks testMatchesCorrectlyWithMixedVerbs() but the testCorrectOrder() I showed above works.
I'll see if I can come up with a compromise

@jim-parry
Copy link
Contributor

jim-parry commented Oct 25, 2018

Just merged a fix for this, fingers crossed. Passes both the tests from #1285 and the new one I added above.
It wasn't swapping the order, but it did replace some rules inappropriately. It now only merges in those generic rules that would not replace the verb-specific ones.

@jim-parry
Copy link
Contributor

@demortx Can you check if this fix works for you? Thanks.

@jim-parry
Copy link
Contributor

I am interpreting the lack of response as being a thumbs up for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants