route_to doesn't work for post routes

[daylightstudio]

I have an issue where I have a form in which I want to set the action to the post route using the route_to function. However, because the context of calling that route_to function is using the HTTPVerb that's not post, it doesn't find the route because they are excluded from the route collection. This seems like a common use case for using route_to.

<form action="<?=route_to('user/insert')?>" method="post">
...
</form>

Route Example:

$routes->post('admin/user/insert', 'UserController::insert', ['as' => 'user/insert']);

[lonnieezell]

Well, damn, you're absolutely correct. Routing was one of the first items that was tackled, and the route_to function wasn't a twinkle in my eye just yet. :) I was ignoring any that weren't in the current method so there were fewer routes to have to parse, but it looks like we'll have to change that....

Any chance you want to make a PR for that?

[daylightstudio]

Sure. The only thing I can think of as a fix is to simply remove HTTPVerb check which as you alluded to, may impact performance slightly.

[lonnieezell]

Yup, that's exactly what I'd do. If we find it impacting performance we can refactor the matching process later.

[daylightstudio]

Kind of a silly question, but how do you normally get the tests to run locally? I tried running phpunit in the root directory and nothing happens. Need to write a few for this update.

[lonnieezell]

No worries. :) I install phpunit through Composer for this, since the Laravel projects I work on require different versions of phpunit. If it's installed that way, I always just do ./vendor/bin/phpunit from the root, because I'm too lazy to setup an alias. :)

[daylightstudio]

I figured out my issues with running the test which had to do some modification I made to the Constants.php file which included other package Constants.

In thinking about this further, wont removing the HTTPVerb check potentially cause issues with GET/POST/DELETE routes with the same URI causing you to possibly get say the "DELETE" route when doing a "GET" request? Should we create a separate array that includes all created routes which the reverseRoute() method uses instead. Or should we do something with the $routes array by first indexing the route with the HTTPverb like so:

$this->routes[$this->HTTPVerb][$name] = [
	'route' => [$from => $to]
];

Or by prefixing the route with the verb:

$this->routes[$this->HTTPVerb . ':' . $name] = [
	'route' => [$from => $to]
];

The latter 2 would probably be a bit more work to fix stuff.

[lonnieezell]

I think the way you did it works great. My initial concern was the resources taken up by the potentially empty arrays for the methods, since arrays used to be fairly expensive. I've seen some blog posts by Blackfire.io that show that these packed arrays are much cheaper in PHP 7.

[daylightstudio]

Actually, after thinking about this implementation more, we could potentially have an issue if a route with the same URI uses add (which uses "*" array key) and another route uses say post and is declared before the add route. The add route would always be overwritten regardless of when it was added in the order because of the array_merge order. This is the line:

$collection = array_merge($this->routes['*'], $this->routes[$verb]);

I'm now thinking the string prefix approach may be better here. I'll work on it and send in a pull request for you to review.

[daylightstudio]

I got pretty far on the string prefix implementation but am having second thoughts now. The implementation I'm working on is giving me issues with named route lookups which are putting me down a smelly code alley that may not be worth it. The pull request array implementation just puts more importance on routes with a verb which may actually be better anyway.

[lonnieezell]

It seems to me that one with a specific method should normally override the wildcard version. Though, if someone waned to take the easy way and use add to handle a couple of different verbs to the same controller/method, and then wanted to specify only one of them as a different target for say, POST, that should probably still work. Hmm....

[daylightstudio]

I believe that scenario you describe would still work if you add the specific verb after the wildcard. It's when you add the wildcard after the verb that would be the issue if we are wanting the overall ordering of route declaration to determine which route is used. However, I'm feeling that if you specify a specific route over a wildcard, no matter what the order, that should be used (which should be what's happening now).

[lonnieezell]

Yeah, I agree. It should definitely check the specific verb collection first, and then check the wildcard grouping, and then fail. If that's what you've got, we should be good here, right?