Bug Report #4630

Optional route params become required

Added by Maurice van der Star about 2 years ago. Updated over 1 year ago.

Status:ClosedStart date:10/24/2012
Priority:HighDue date:
Assignee:Lorenzo Pisani% Done:

100%

Category:Core
Target version:3.3.1
Resolution:fixed Points:1

Description

This report is related to the topic:
http://forum.kohanaframework.org/discussion/11196/bug-on-default-params-with-routes-3-3develop-or-3-3master#Item_3

I've found that the initial problem was my own fault (assigning string default to param that has a numeric regex). I found that my project is still broken though, even with very simple routes. It seems like when a param after a optional param is filled, it makes the optional required. This is not the case with older versions (3.3 RC or 3.2).

I've created a minion Task with different tests. They can be found @
https://github.com/mauserrifle/kohana/blob/3.3/routes/application/classes/Task/Routebug.php

Description from file:

 *  First problem
 * -----------
 * I thought it was a bug but found out my param 'version' was defined as
 * a [0-9]++ and the default was set as a string '1'. This causes multiple
 * weird behaviors I will be demonstrating within this Task. Not really a bug
 * as its fixed with the numeric default value. Still worth to look at.
 *
 * Maybe a idea to check if a string is numeric?
 *
 * Second problem
 * ----------
 * Ok this problem is really bugging me atm with lots of routes in my projects.
 * Optional params eventually will become required somehow. This is also demon
 * strated within this task
 *
 *
 * Everything is OK on commit 4d17878d08a6c139c615e19cb47eae7dd78820df.
 * Some things are NOTOK on commit fb062cfe4f0bae498ab09a383fb5901bd8563528

To run the tests:

git clone -b "3.3/routes" --recursive git://github.com/mauserrifle/kohana.git
cd kohana
rm install.php
php index.php routebug

To see behavior of old RC, (even 3.2 behaved like this):

cd system
git checkout 
php ../index.php routebug

As you can see everything is fine on 4d17878d08a6c139c615e19cb47eae7dd78820df and will break on fb062cfe4f0bae498ab09a383fb5901bd8563528.

I hope I made it little bit clear. First problem is my own fault by assigning a string default to a param that as a numeric regex. But might need fixing. It's different behavior than older versions. The second problem is clearly a problem (it also happens in the first problem).

Have not yet looked into the bugged code itself. Wanted to show you guys some tests first.


Related issues

Related to Kohana v3.x - Bug Report #4113: Route::url() works improperly Closed 07/14/2011
Duplicated by Kohana v3.x - Bug Report #4681: Route::url() says param is required when it's not Closed 12/20/2012

Associated revisions

Revision ff29ed42
Added by Chris Bandy about 2 years ago

Refactor uri(), refs #4630

Uses preg_replace_callback to process each group and key at one level
exactly once. Recursively processes groups depth-first to detect
specified parameters before deciding to include the group in the result.

Revision 40e71236
Added by Lorenzo Pisani over 1 year ago

Merge pull request #315 from kohana/3.3/feature/4630-uri-optional-params

Build URI using preg_replace_callback
refs #4630

Revision e19c5feb
Added by Chris Bandy over 1 year ago

Recursively match braces for route groups, fixes #4630

Revision 48635278
Added by Chris Bandy over 1 year ago

Remove superfluous regex assertion, refs #4630

Braces are matched by the group regex

History

#1 Updated by Maurice van der Star about 2 years ago

I made a typo.

To see behavior of old RC, (even 3.2 behaved like this):

cd system
git checkout fb062cfe4f0bae498ab09a383fb5901bd8563528
php ../index.php routebug

#2 Updated by Jeremy Bush about 2 years ago

It seems like when a param after a optional param is filled, it makes the optional required.

This sounds like expected behavior to me. If a later parameter is specified, it would make sense that all previous parameters would be required as well. Otherwise your route matching would not make sense.

#3 Updated by Maurice van der Star about 2 years ago

Jeremy Bush wrote:

It seems like when a param after a optional param is filled, it makes the optional required.

This sounds like expected behavior to me. If a later parameter is specified, it would make sense that all previous parameters would be required as well. Otherwise your route matching would not make sense.

I get your point of view but do not agree for flexibility. If you define your params with specific regexes, the system can skip the param. This way you can make restful like routes with one route defined.

I like the idea having a route like:

adverts(/<id>)/<action>

And match URLs like this on it:

adverts/edit
adverts/1/edit

That is not possible now.

I do not like the idea of switching it to:

adverts/<action>(/<id>)

as its not really logic URL's. Every previous param should have meaning. So if the user is changing the URL from adverts/repost/1 to adverts/repost, that would not make sense, as repost is not a page without a ID. So the uri structure is not proper. (I've read too much SEO guides by google)

The original route of the kohana-api module by managedit also has this route:

api/<controller>(/<id>)(/<custom>)

One route for all restful matches.

I am sure a lot of projects will break with this new behavior and I wonder if this is implemented by purpose? I cannot find any ticket about it. If this really is a change then I think you will have more people complaining about it and not being in the migration guide.

I hope you guys agree that the behavior of 3.2 and 3.3 RC1/2 is better than what happens now. This behavior is only changed like 15 commits earlier. If it stays, I think the routes are way too limited :(

#4 Updated by Robert-Jan Dreu -rjd22- about 2 years ago

IMHO you should never use 1 route for all. This is wrong in many ways.

#5 Updated by Maurice van der Star about 2 years ago

Robert-Jan Dreu rjd22 wrote:

IMHO you should never use 1 route for all. This is wrong in many ways.

I agree. I have lots of routes in my application (The API route is a exception). They just all follow the pattern as described above to be restful as possible. I don't think you need multiple routes for that. That would create crazy amounts, and for what? Because the route system is made less flexible?

It never been the case in previous versions (even RC2 and above) and now it breaks.

#6 Updated by Jason Reading about 2 years ago

As a workaround, if you provide a blank default for the optional parameter this error does not occur.

To take your example above:

Route::set(
    'api',
    'api/<controller>(/<id>)(/<custom>)',
    array(
        'id' => '\d+',
    )
)->defaults(
    array(
        'controller' => 'api',
        'id' => '',     // Parameter is optional
        'custom' => '', // Parameter is optional <-- this line has no affect on the bug as custom is the last optional part of the route
    )
);

#7 Updated by Jeremy Bush about 2 years ago

In my opinion, a route like this: `adverts(/<id>)/<action>` is a very unclear route. This should be split into two routes. This will work in the current version of course, but I would not make a route like this, ever.

However, a route like `api/<controller>(/<id>)(/<custom>)` is simply incorrect. I think in this specific example the regex is different for each parameter, but if they were the same, this would end up in unexpected reverse routing behavior. For example:

`Route->uri(array('controller' => 'test', 'custom' => 'foobar'))`

This would generate `api/test/foobar`, which is clearly wrong. You would never want this.

In fact, if they were different (like they are now), this reverse route would never match! I think a reverse route should always match the route it was generated from.

With that being said, I don't know the specific reason it changed. A git blame will tell us. Do you have the commit that exactly changed the behavior? (not the commit that it worked on).

#8 Updated by Maurice van der Star about 2 years ago

Jeremy Bush wrote:

In my opinion, a route like this: `adverts(/<id>)/<action>` is a very unclear route. This should be split into two routes. This will work in the current version of course, but I would not make a route like this, ever.

However, a route like `api/<controller>(/<id>)(/<custom>)` is simply incorrect. I think in this specific example the regex is different for each parameter, but if they were the same, this would end up in unexpected reverse routing behavior. For example:

`Route->uri(array('controller' => 'test', 'custom' => 'foobar'))`

This would generate `api/test/foobar`, which is clearly wrong. You would never want this.

In fact, if they were different (like they are now), this reverse route would never match! I think a reverse route should always match the route it was generated from.

With that being said, I don't know the specific reason it changed. A git blame will tell us. Do you have the commit that exactly changed the behavior? (not the commit that it worked on).

Well I kinda would want that if that's the expected behavior of the routing. Which was the case. I do get where you come from as its a little bit 'magic'. So yes on that case it's a good learn for now, but I do think it's a too big behavior change that was apparently not planned.

Commit 52fcb825871c77227b8270a0d7c72a2b2b343274 causes it: https://github.com/kohana/core/commit/52fcb825871c77227b8270a0d7c72a2b2b343274

#9 Updated by Jeremy Bush about 2 years ago

Maurice van der Star wrote:

Well I kinda would want that if that's the expected behavior of the routing. Which was the case. I do get where you come from as its a little bit 'magic'. So yes on that case it's a good learn for now, but I do think it's a too big behavior change that was apparently not planned.

Major versions are for behavioral changes. Nothing is "too big". This looks like it was a side effect of fixing a default value route bug. We'll discuss it and see what we want to do.

#10 Updated by Maurice van der Star about 2 years ago

True, but I do miss it bad. Thanks for your time all!

#11 Updated by Sadjow Leão almost 2 years ago

It looks like node.js frameworks routing system. I guess that it should be implemented. Use the same route for a group of action can be a software design decision.

#12 Updated by Matt Lyon almost 2 years ago

I recently ran into this issue when upgrading one of my projects to 3.3 as well. I prefer the old behaviour (so much so that I've actually just overridden the Route::uri() function in my own application to revert the change). If this isn't going to be resolved otherwise, then I definitely think it should be mentioned in the upgrade guide, at the very least.

#13 Updated by Lorenzo Pisani almost 2 years ago

  • Assignee set to Lorenzo Pisani

#14 Updated by dan caragea almost 2 years ago

Jeremy Bush wrote:

It seems like when a param after a optional param is filled, it makes the optional required.

This sounds like expected behavior to me. If a later parameter is specified, it would make sense that all previous parameters would be required as well. Otherwise your route matching would not make sense.

It's actually the other way around:
For a route like
`Route::set('cart/address', 'cart/address(/c<cart_id>)(/<proj_id>)', ['proj_id' => '\d*', 'cart_id' => '\d*'])`
`Route::url('cart/address', ['cart_id' => 3])` throws an exception but `Route::url('cart/address', ['proj_id' => 3])`
doesn't. I would've expected the cart to work and proj not, if anything. But I'd vote to allow ()() for flexibility, it does have its uses.

#15 Updated by Lorenzo Pisani over 1 year ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100
  • Resolution set to fixed

Chris Bandy refactored the uri method and added some more tests. I merged his changes to the develop branch so you can give it a try and see if there are any other edge cases that tests need to be written for.

#16 Updated by Maurice van der Star over 1 year ago

Lorenzo Pisani wrote:

Chris Bandy refactored the uri method and added some more tests. I merged his changes to the develop branch so you can give it a try and see if there are any other edge cases that tests need to be written for.

I am very happy to confirm it's fixed. I upgraded my system folder to the latest commit. Everything seems to be working, even my crazy complex API route from my initial forum post. Thank you so much Chris, Lorenzo and all!

#17 Updated by Matt Lyon over 1 year ago

  • Status changed from Closed to Feedback

I found one weird result with this latest fix. Running the following code:

Route::set('test', '(<controller>(/<action>(/<id>)(/<type>)))', array('id' => '[0-9]+', 'type' => '(html|json)'))
    ->defaults(array(
        'controller' => 'test',
        'action' => 'index',
        'type' => 'html'
    ));

var_dump(Route::get('test')->uri());
var_dump(Route::get('test')->uri(array('controller' => 'test')));
var_dump(Route::get('test')->uri(array('controller' => 'test', 'action' => 'index')));
var_dump(Route::get('test')->uri(array('controller' => 'test', 'action' => 'index', 'id' => 123)));
var_dump(Route::get('test')->uri(array('controller' => 'test', 'action' => 'index', 'type' => 'html')));
var_dump(Route::get('test')->uri(array('controller' => 'test', 'action' => 'index', 'id' => 123, 'type' => 'html')));

Kohana 3.2/master outputs:

string '' (length=0)
string 'test' (length=4)
string 'test/index' (length=10)
string 'test/index/123' (length=14)
string 'test/index/html' (length=15)
string 'test/index/123/html' (length=19)

Kohana 3.3/master outputs:

string '' (length=0)
string '' (length=0)
string '' (length=0)
string 'test/index/123/html' (length=19)
string '' (length=0)
string 'test/index/123/html' (length=19)

Kohana 3.3/develop (with the most recent commits) outputs:

string '' (length=0)
string '' (length=0)
string '' (length=0)
string '<controller>(/<action>(/123' (length=27)
string '' (length=0)
string '<controller>(/<action>(/123' (length=27)

Kohana 3.2's output seems correct/appropriate to me.

#18 Updated by Chris Bandy over 1 year ago

  • Status changed from Feedback to Review
  • % Done changed from 100 to 90
  • Resolution deleted (fixed)

Definitely a bug. Maybe because <type> has a capturing group? (?:html|json) should perform better in any case.

#19 Updated by Chris Bandy over 1 year ago

  • Status changed from Review to Closed
  • % Done changed from 90 to 100
  • Resolution set to fixed

Also available in: Atom PDF