Bug #43589
Route defaults can be omitted when creating URIs
Status: | Resolved | Start date: | 2012-12-04 | |
---|---|---|---|---|
Priority: | Should have | Due date: | ||
Assigned To: | Bastian Waidelich | % Done: | 100% |
|
Category: | MVC - Routing | |||
Target version: | TYPO3 Flow Base Distribution - 2.0 beta 1 | |||
PHP Version: | 5.4 | Complexity: | ||
Has patch: | No | Affected Flow version: | (any) |
Description
The intended behavior is following:
When generating an URI using the UriBuilder, a route only matches the specified routeValues if they are all equal to the defaults of the route. So given the following route:
1- 2 uriPattern: 'foo' 3 defaults: 4 bar: 'baz'
(@package, @controller etc left out intentionally to keep it simple)
following should be the case:
Router::resolve(array('bar' => 'baz')); // resolves to "foo" Router::resolve(array('bar' => 'xyz')); // does NOT resolve Router::resolve(array()); // does NOT resolve
But currently this is the behavior:
Router::resolve(array('bar' => 'baz')); // resolves to "foo" Router::resolve(array('bar' => 'xyz')); // does NOT resolve Router::resolve(array()); // resolves to "foo"
Meaning: you can omit default values (if they are not part of the uriPattern).
This works fine in most cases, but it makes it quite hard to get everything right, because too many routes match and it's hard to comprehend.
I already work on a fix, but it has two drawbacks I'd like to get your feedback for:
1. It's a breaking change and you'll have to check all your routes after applying the fix..
I did that with a quite large project and most routes still worked out of the box. Except for the pagination routes:
1- 2 name: 'Articles with optional pagination' 3 uriPattern: 'articles(/page/{--paginate.currentPage})' 4 defaults: 5 '@package': 'My.Package' 6 '@controller': 'Article' 7 '@action': 'index' 8 '--paginate': 9 currentPage: '0' 10 '@package': '' 11 '@subpackage': '' 12 '@controller': '' 13 '@action': 'index'
This route wouldn't resolve if $routeValues['--paginate']['@action‘] is not set to "index" from now on, so I had to create an additional route:
1- 2 name: 'Articles with pagination' 3 uriPattern: 'articles/page/{--paginate.currentPage}' 4 defaults: 5 '@package': 'My.Package' 6 '@controller': 'Article' 7 '@action': 'index' 8 '--paginate': 9 '@package': '' 10 '@subpackage': '' 11 '@controller': '' 12 '@action': 'index' 13 14- 15 name: 'Articles' 16 uriPattern: 'articles' 17 defaults: 18 '@package': 'My.Package' 19 '@controller': 'Article' 20 '@action': 'index'
2. @action can't be omitted
The other drawback is the fallback behavior of the UriBuilder:
Currently, if you don't specify @package the package of the current request is used. The same for @subpackage and @controller (that's why <f:link.action action="foo" /> links to the fooAction of the current controller.
For @action it's different currently: If you don't specify it, the @action is not set in the $routeValues.
Because of the current behavior a route specifying the @action will still resolve. With the fix that wouldn't be the case anymore..
I see two solutions:
A) Make the actionName argument of UriBuilder::uriFor() required (as well as the action arguments of uri.action, link.action and form.
B) Fallback to "index" (or detect the first action of the controller class) I'm adding this for completeness, but this would be quite error prone IMO..
Related issues
Associated revisions
[!!!][BUGFIX] Route defaults have to be specified when creating URIs
Currently when creating URIs one can omit route values that are defined
as defaults of the route.
This is not the intended behavior though, because with a lot of routes
in place this leads to the situation where it gets difficult to target
a certain route.
Thus for a route to match the given route values, one should have to
specify all the routing defaults exactly as they are specified by the
route (except for when a route part appears in the uriPattern).
This is a breaking change if you used Router or UriBuilder to create
URIs without specifying all defaults (see #43589 for details).
Besides, all Fluid ViewHelpers that use the UriBuilder now require the
action argument to be specified.
Change-Id: I7e4e0e20a397839c89a107b28fa176dd62459ec1
Fixes: #43589
Releases: 1.2
[TASK] Adjust routing documentation to recent changes
This fixes the routing examples provided with the definitive guide.
Change-Id: Idaff4388a489a435440078828c7267797e75674e
Related: #43589
Releases: master, 2.0
[TASK] Adjust routing documentation to recent changes
This fixes the routing examples provided with the definitive guide.
Change-Id: Idaff4388a489a435440078828c7267797e75674e
Related: #43589
Releases: master, 2.0
History
#1 Updated by Gerrit Code Review over 2 years ago
- Status changed from Accepted to Under Review
Patch set 1 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/16960
#2 Updated by Gerrit Code Review over 2 years ago
Patch set 2 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/16960
#3 Updated by Gerrit Code Review over 2 years ago
Patch set 3 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/16960
#4 Updated by Karsten Dambekalns over 2 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100