Bug #90924

Maximum route parameters always appended

Added by Matthias Krappitz over 1 year ago. Updated about 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Link Handling, Site Handling & Routing
Start date:
2020-04-01
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
9
PHP Version:
7.3
Tags:
Complexity:
Is Regression:
Yes
Sprint Focus:

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.


Related issues

Related to TYPO3 Core - Bug #91098: Routing and RouteEnhancer problemsNeeds Feedback2020-06-23

Actions
Related to TYPO3 Core - Bug #91333: routeEnhancer of type Extbase with multiple routePaths and various argument with same controller and action are broken since Typo3 9.5.14Closed2020-05-07

Actions
Related to TYPO3 Core - Bug #91880: Routing for actions without parameters brokenClosed2020-07-28

Actions
#1

Updated by Matthias Krappitz over 1 year 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?

#2

Updated by Oliver Hader over 1 year ago

  • Related to Bug #91098: Routing and RouteEnhancer problems added
#3

Updated by Robert Wolle over 1 year 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.

#4

Updated by Georg Ringer over 1 year 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
#5

Updated by Matthias Krappitz over 1 year ago

Yes, this now is now even worse as we can't use the latest security related updates because of this.

#6

Updated by Till Wimmer over 1 year 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.

#7

Updated by Oliver Hader over 1 year 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
#8

Updated by Matthias Krappitz over 1 year 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

#9

Updated by Oliver Hader over 1 year 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?

#10

Updated by Gerrit Code Review over 1 year 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

#11

Updated by Gerrit Code Review over 1 year 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

#12

Updated by Matthias Krappitz over 1 year 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.

#13

Updated by Gerrit Code Review over 1 year 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

#14

Updated by Gerrit Code Review over 1 year 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

#15

Updated by Oliver Hader over 1 year 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!

#16

Updated by Gerrit Code Review over 1 year 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

#17

Updated by Arne Bracht Bracht over 1 year 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.

#18

Updated by Oliver Hader over 1 year ago

  • Category changed from Reports to Link Handling, Site Handling & Routing
#19

Updated by Oliver Hader over 1 year 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.

#20

Updated by Gerrit Code Review over 1 year 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

#21

Updated by Gerrit Code Review over 1 year 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

#22

Updated by Oliver Hader over 1 year ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
#23

Updated by Gerrit Code Review over 1 year 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

#24

Updated by Oliver Hader over 1 year ago

  • Status changed from Under Review to Resolved
#25

Updated by Robert Vock over 1 year ago

  • Related to Bug #91880: Routing for actions without parameters broken added
#26

Updated by Benni Mack about 1 year ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF