Bug #90924
closedMaximum route parameters always appended
Added by Matthias Krappitz over 4 years ago. Updated about 4 years ago.
100%
Description
Since TYPO3 9.5.14 behaviour changed with the following routenhancer example:
Example:
type: Extbase
limitToPages:
- 123
extension: Example
plugin: Example
routes:
-
routePath: '/page/{page}'
_controller: 'Example::list'
_arguments:
page: '@widget_0/currentPage'
-
routePath: '/company/{company_profile}'
_controller: 'Example::list'
_arguments:
company_profile: 'companyProfile'
-
routePath: '/company/{company_profile}/page/{page}'
_controller: 'Example::list'
_arguments:
company_profile: 'companyProfile'
page: '@widget_0/currentPage'
defaultController: 'Example::list'
defaults:
page: '0'
requirements:
page: \d+
aspects:
page:
type: StaticRangeMapper
start: '1'
end: '200'
company_profile:
type: PersistedAliasMapper
tableName: tx_example_domain_model_example
routeFieldName: slug
Until TYPO3 9.5.13 when linking as follows this resulted in a correct and working link like this: /somepage/company/the-company/
<f:link.page pageUid="123" additionalParams="{tx_example_example: {companyProfile: 123}}">Example Company</f:link.page>
In TYPO3 9.5.14 and 9.5.15 it results in a wrong link: /somepage/company/the-company/page/0/ and calling it results in 404.
It looks to me that the routing for some reason picks / matches incorrectly the route '/company/{company_profile}/page/{page}', even though it should pick up the route '/company/{company_profile}' instead, as only the companyProfile parameter is in the link.
So this breaks many of my 9 installations in terms of URLs and I can't update.
Updated by Matthias Krappitz over 4 years ago
Bug is still there in TYPO3 9.5.16. Here is another example of a routeEnhancer, where probably the same bug appears in another flavor - see following example configuration, where i did not configure limitToPages, because event list and detail views are on many different pages:
routeEnhancers:
EventDetailPlugin:
type: Plugin
routePath: '/event/{eventUid}'
namespace: tx_example_pi1
aspects:
eventUid:
type: PersistedAliasMapper
tableName: tx_example_event
routeFieldName: slug
EventListPlugin:
type: Plugin
routePath: '/weitere/{page}'
namespace: tx_example_pi1
defaults:
page: '0'
requirements:
page: \d+
aspects:
page:
type: StaticRangeMapper
start: '1'
end: '200'
Until 9.5.13 this config works fine creating links like on pages, where the get parameters just match:
https://www.example.com/path/detail/event/gymnastik-3.html
https://www.example.com/path/list/weitere/1.html
As of 9.5.14 and later it results in wrong EventDetailPlugin enhanced links, attaching also the EventListPlugin enhancer for no reason and this results in a 404:
https://www.example.com/path/detail/weitere/0.html?tx_example_pi1%5BeventUid%5D=3237&cHash=a615bd9f6a6a532af18d25c187e41659
Whereas the EventListPlugin enhancer still stays the same and works.
This still prevents me from updating all my 9 projects since 9.5.13. IMHO this is a disastrous bug, which can kill an enormous amount of urls and no one seems to care until now. So I'm kind of unsure if I'm really the only one affected, because I'm having a mistake in my route enhancer config or a general misunderstanding here?
Updated by Oliver Hader over 4 years ago
- Related to Bug #91098: Routing and RouteEnhancer problems added
Updated by Robert Wolle over 4 years ago
Same here:
https://forge.typo3.org/issues/91333
We have a multisite with 70 projects with archive pages, which use pagination and have filters. So I cannot update to > 9.5.13 using the routeEnhancers in current state.
Updated by Georg Ringer over 4 years ago
- Related to Bug #91333: routeEnhancer of type Extbase with multiple routePaths and various argument with same controller and action are broken since Typo3 9.5.14 added
Updated by Matthias Krappitz over 4 years ago
Yes, this now is now even worse as we can't use the latest security related updates because of this.
Updated by Till Wimmer over 4 years ago
This is somehow related, but not sure if it's the exactly same issue: https://forge.typo3.org/issues/91537
In that case, 9.5.15 was the breaking version, not 9.5.14.
Updated by Oliver Hader over 4 years ago
The reason for all this is, that defaults have not been applied correctly in earlier version, but do since issue #86895 and are in line with Symfony's defaults (see https://symfony.com/blog/new-in-symfony-4-3-always-include-route-default-values).
I'd like to shed some light what's going on internally:
1) 1st example from above - &tx_example_example[companyProfile]=123
¶
Action-Controller is implicitly set by Extbase to Example::list
, thus the complete list of parameters would be&tx_example_example[companyProfile]=123&tx_example_example[action]=list&tx_example_example[controller]=Example
... routePath: '/company/{company_profile}' routePath: '/company/{company_profile}/page/{page}' ... defaults: page: '0'
This configuration defines ambiguous routes - even if page
is not given, the default value of '0'
is used - thus, both routes are complete. TYPO3's routing implementation assumes that the longest route (by string length) is the most specific one.
This could have been solved easily by removing the default behavior for page
.
The 404 error when calling /somepage/company/the-company/page/0/
(mind the slash at the end) is most probably related to issue #89091
2) 2nd example from above - tx_example_pi1[eventUid]=3237
¶
... routePath: '/event/{eventUid}' routePath: '/weitere/{page}' ... defaults: page: '0'
This configuration is very similar to the previous one, albeit the default value for page
is encapsulated in it's own section in YAML.
Thus, the route for /weitere/{page}
is always complete whenever some parameter contains the tx_example_pi1
parameter prefix.
The scenario of having a combination of the two enhancer declarations feels like a real bug which should be addressed.
Conclusion¶
- ambiguous routes should result in those that "match best" and consider default values as a fallback only
&tx_example_example[companyProfile]=123
→/company/my-company-name
&tx_example_example[companyProfile]=123&tx_example_example[@widget_0][currentPage]=2
→/company/my-company-name/page/2
- there should be a way to define the processing order of
routePath
definitions- e.g. like they occur in the configuration
- this would introduce more breaking behavior and would required some additional setting/feature-flag
Updated by Matthias Krappitz over 4 years ago
@Oliver Hader: Thanks for clarifying on the "new" behaviour of defaults for route parameters.
As for 1) removing the defaults for the page parameter solves the problem of the '/company/{company_profile}/page/{page}' route matching when there is just a company_profile parameter. But without this default the fluid paginate widget when linking to the first page with no page (and no company_profile) parameter results in an URL with action, controller and cHash parameter. So the defaultController parameters get appended as probably no route is matching any more:
/somepage/?tx_example_example[action]=list&tx_example_example[controller]=Example&cHash=142c62c96dd1c21eb853b1ac44daf900
So i needed to add an empty route to solve this, which may be a bit pointless, but is at least a solution:
-
routePath: '/'
_controller: 'Example::list'
As for 2) it does not make any sense for me that two totally separate routing rules get mixed up / combined, no matter how their configuration looks like, just because they share the same parameter prefix. But also this case could be solved with removing the defaults for page parameter in the page route.
So in total at least I'm now good with this issue, but the main problem is the incomplete documentation of these routing details and lack of communication of such an unexpected severe breaking change within a minor patch level update of the TYPO3 core.
As for the documentation I mean this passage for example:
https://docs.typo3.org/m/typo3/reference-coreapi/master/en-us/ApiOverview/Routing/AdvancedRoutingConfiguration.html#simple-enhancer
"The defaults section defines which URL parameters are optional. If the parameters are omitted on generation, they can receive a default value, and do not need a placeholder - it is possible to add them at the very end of the routePath."
Here we should add infos about the special behaviour of default parameters since 9.5.13, so that not more users fall into this trap.
Also in 9.5.14 changelog there should have been a bit more clear information about this behaviour change of routing defaults than just "TYPO3's Route ... evaluate "default" and "requirements" options similar to Symfony's configuration options.":
https://get.typo3.org/release-notes/9.5.14
Updated by Oliver Hader over 4 years ago
Matthias Krappitz wrote:
As for 2) it does not make any sense for me that two totally separate routing rules get mixed up / combined, no matter how their configuration looks like, just because they share the same parameter prefix. But also this case could be solved with removing the defaults for page parameter in the page route.
As mentioned above:
"The scenario of having a combination of the two enhancer declarations feels like a real bug which should be addressed."
So in total at least I'm now good with this issue, but the main problem is the incomplete documentation of these routing details and lack of communication of such an unexpected severe breaking change within a minor patch level update of the TYPO3 core.
As for the documentation I mean this passage for example:
https://docs.typo3.org/m/typo3/reference-coreapi/master/en-us/ApiOverview/Routing/AdvancedRoutingConfiguration.html#simple-enhancer
"The defaults section defines which URL parameters are optional. If the parameters are omitted on generation, they can receive a default value, and do not need a placeholder - it is possible to add them at the very end of the routePath."
That's wrong and should be changed as well... besides that "placeholder" actually is a "route variable"...
Here we should add infos about the special behaviour of default parameters since 9.5.13, so that not more users fall into this trap.
Which one? Or are you referring to https://github.com/TYPO3/TYPO3.CMS/commit/fd4ddf1329 which was included in 9.5.14?
Updated by Gerrit Code Review over 4 years ago
- Status changed from New to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64877
Updated by Gerrit Code Review over 4 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64877
Updated by Matthias Krappitz over 4 years ago
Which one? Or are you referring to https://github.com/TYPO3/TYPO3.CMS/commit/fd4ddf1329 which was included in 9.5.14?
Yes it could be this change. But as the majority of people will not read every git comment of patch level updates, this IMHO should have been mentioned more drastically here: https://get.typo3.org/release-notes/9.5.14.
"TYPO3's Route ... evaluate "default" and "requirements" options similar to Symfony's configuration options." is not really expressing what this change really can and will break.
Updated by Gerrit Code Review over 4 years ago
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64877
Updated by Gerrit Code Review over 4 years ago
Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64877
Updated by Oliver Hader over 4 years ago
@Matthias Krappitz: Please have a look at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64877 trying to fix the problem you described. Any feedback, review, tests are appreciated. Thx in advance!
Updated by Gerrit Code Review over 4 years ago
Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64877
Updated by Arne Bracht over 4 years ago
Don't know this is same problem as here: https://forge.typo3.org/issues/91633, but for this there is noch change and it did not work.
Updated by Oliver Hader over 4 years ago
- Category changed from Reports to Site Handling, Site Sets & Routing
Updated by Oliver Hader over 4 years ago
Arne Bracht Bracht wrote:
Don't know this is same problem as here: https://forge.typo3.org/issues/91633, but for this there is noch change and it did not work.
There's no direct relation between those issues.
Updated by Gerrit Code Review over 4 years ago
Patch set 1 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65040
Updated by Gerrit Code Review over 4 years ago
Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65041
Updated by Oliver Hader over 4 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 079cc536973332a4784c0f199bd8936311ab4aa5.
Updated by Gerrit Code Review over 4 years ago
- Status changed from Resolved to Under Review
Patch set 2 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/65041
Updated by Oliver Hader over 4 years ago
- Status changed from Under Review to Resolved
Applied in changeset e25bc8a867aa9995fd1e9f298c3d5caec2cad77d.
Updated by Robert Vock over 4 years ago
- Related to Bug #91880: Routing for actions without parameters broken added
Updated by Oliver Hader almost 3 years ago
- Related to Bug #91323: Route enhancer kills unregistered get parameters added
Updated by Oliver Hader about 2 years ago
- Related to Bug #90959: Wrong URL applied for Route added