Project

General

Profile

Actions

Bug #91633

open

9.5.19 breaks formerly working routeEnhancer

Added by Matthias Krappitz almost 4 years ago. Updated 9 months ago.

Status:
Under Review
Priority:
Must have
Assignee:
Category:
Link Handling, Site Handling & Routing
Start date:
2020-06-10
Due date:
% Done:

0%

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

Description

I have the following routeEnhancer, which worked until 9.5.18:

  BookingPlugin:
    type: Extbase
    limitToPages:
      - 123
    extension: Events
    plugin: Events
    routes:
      -
        routePath: '/{event}/{date}/{booking}'
        _controller: 'Booking::new'
        _arguments:
          event: event
          date: date
          booking: booking
    defaultController: 'Booking::new'
    defaults:
      booking: ''
    aspects:
      event:
        type: PersistedAliasMapper
        tableName: tx_events_domain_model_event
        routeFieldName: slug
      date:
        type: PersistedAliasMapper
        tableName: tx_events_domain_model_date
        routeFieldName: slug
      booking:
        type: StaticValueMapper
        map:
          new: 1

It creates an url like /some-page/some-event/some-date/new/ with following linking:

<f:link.page pageUid="123" additionalParams="{tx_events_events : {event : event, date : date, booking : 1}}">register</f:link.page>

From 9.5.19 this route results in a 404 page not found error, before it was resolving fine. I guess it has to do with https://github.com/TYPO3-CMS/core/commit/d94f2004eb2158cacb4c2e5acb21d73ffdf49858

IMHO we should really stop introducing severe breaking changes (especially sufficient communication about it) in patch level releases of an LTS version. This happened now quite some times recently, espcially in the routing area, always resulting in heavy URL problems and breaking projects.


Related issues 3 (0 open3 closed)

Related to TYPO3 Core - Bug #91246: routeEnhancer defaults not applied in TYPO3 v9.5.16Closed

Actions
Related to TYPO3 Core - Bug #90531: Requirements are not considered when an aspect is presentClosed2020-02-25

Actions
Related to TYPO3 Core - Bug #91537: 9.5.15+ breaks routing: Only respects the shortest matching ruleClosed2020-05-29

Actions
Actions #1

Updated by Matthias Krappitz almost 4 years ago

OK, solved it (temporarily) by removing the defaults section with the default for the booking parameter. Not sure if this is then related or a duplicate of #90924, but as the error just came with the update from 9.5.18 to 9.5.19 I think it can also be a new problem.

Actions #2

Updated by Arne Bracht almost 4 years ago

I have a similar problem with tx_news (8.3.0) after update 9.5.18 to 9.5.19

With or without defaults the Enhancer seems to work an makes wonderfull urls like 2020-05-13-wonderfullnews in list, but details/more link redirect to 404 (not found)

  News:
    type: Extbase
    limitToPages:
      - 7
    extension: News
    plugin: Pi1
    routes:
      - 
        routePath: '/{news-year}-{news-month}-{news-day}-{news-title}'
        _controller: 'News::detail'
        _arguments:
          news-title: news
          news-year: year
          news-month: month
          news-day: day
        requirements:
          news-year: '\d+'
          news-month: '\d+'
          news-day: '\d+'
    defaultController: 'News::detail'
    defaults:
      news-year: ''
      news-month: ''
      news-day: ''
    aspects:
      news-title:
        type: PersistedAliasMapper
        tableName: tx_news_domain_model_news
        routeFieldName: path_segment
      news-year:
        type: StaticRangeMapper
        start: '2010'
        end: '2025'
      news-month:
        type: StaticRangeMapper
        start: '1'
        end: '13'
      news-day:
        type: StaticRangeMapper
        start: '1'
        end: '32'
Actions #3

Updated by Oliver Hader almost 4 years ago

  • Related to Bug #91246: routeEnhancer defaults not applied in TYPO3 v9.5.16 added
Actions #4

Updated by Oliver Hader almost 4 years ago

  • Related to Bug #90531: Requirements are not considered when an aspect is present added
Actions #5

Updated by Gerrit Code Review almost 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/+/64903

Actions #6

Updated by Oliver Hader almost 4 years ago

Yes, handling route requirements is very unfortunate.

Analysis of current issue

Given that URL /page/event-name/2020-01-02/new shall be resolved with configuration shown in original report.
Based on requirements Symfony's routing is applying different regular expressions.

The following variants show that the current behavior (1st in the following list) is plain wrong since the first variable is resolved to event-name/2020-01-02 (since slugs were allowed to contain slashes...).

a) original configuration (having defaults)

see https://regex101.com/r/ixxdac/1

#^/page/(?P<tx_events_events__event>.+)/(?P<tx_events_events__date>.+)(?:/(?P<tx_events_events__booking>.+))?$#sDu

resolved to

tx_events_events__event: event-name/2020-01-02
tx_events_events__date: new

b) omitting defaults for booking

see https://regex101.com/r/V8DNQ2/1

#^/page/(?P<tx_events_events__event>.+)/(?P<tx_events_events__date>.+)/(?P<tx_events_events__booking>.+)$#sDu

resolved to

tx_events_events__event: event-name
tx_events_events__date: 2020-01-02
tx_events_events__booking: new

c) avoid overriding requirements in AbstractEnhancer, keep original defaults (sticking to Symfony's default, not allowing slashes)

see https://regex101.com/r/voY91N/1

#^/page/(?P<tx_events_events__event>[^/]++)/(?P<tx_events_events__date>[^/]++)(?:/(?P<tx_events_events__booking>[^/]++))?$#sDu

resolved to

tx_events_events__event: event-name
tx_events_events__date: 2020-01-02
tx_events_events__booking: new

History of the whole story

  • v9.5.14
    • https://review.typo3.org/c/Packages/TYPO3.CMS/+/60361
    • defaults and requirements were not correctly a applied - a definitive bug
    • potentially breaking since suddenly TYPO3 behaves like it was supposed to be - like Symfony's routing
    • defaults and requirements used until then, actually were not considered (probably copy&paste scenarios)
    • requirements for variables having aspects were overridden, since aspects knew best, what is allowed and what not
  • v9.5.15
    • https://review.typo3.org/c/Packages/TYPO3.CMS/+/63655
    • relaxed the strict Symfony default setting for requirements, allowing slashes in slugs
    • routePath: '/{a}' was then valid for a URL like /slug/having/slashes
    • this behavior was applied per default - which is basically the main problem in general
  • v9.5.19

Conclusion - my personal POV

  • "allowing slashes in slugs" is suboptimal ("stupid")
  • in case this is really required, it has to be given explicitly in route enhancer configuration
  • changing this behavior will be breaking (again) for those relying on this feature

Some examples in order to demonstrate why allowing slashes in slugs is ambiguous and problematic:

routePath: '/{first}/{second}/{third}'
defaults:
  second: ''
  third: ''

and the following URLs

  • /a: simple, that's
    • &first=a
  • /a/b: could be
    • &first=a&second=b or
    • &first=a/b
  • /a/b/c: could be
    • &first=a&second=b&third=c or
    • &first=a/b&second=c or
    • &first=a&second=b/c or
    • &first=a/b/c
Actions #7

Updated by Oliver Hader almost 4 years ago

on https://forge.typo3.org/issues/91633?issue_count=53&issue_position=2&next_issue_id=91519&prev_issue_id=91636#note-2

Arne Bracht Bracht wrote:

I have a similar problem with tx_news (8.3.0) after update 9.5.18 to 9.5.19

With or without defaults the Enhancer seems to work an makes wonderfull urls like 2020-05-13-wonderfullnews in list, but details/more link redirect to 404 (not found)

requirements in provided configuration are not considered, they need to be on the same level as defaults (currently they are nested in the first routes item). Besides that defaults before mandatory variables are not considered - otherwise URL ---wonderfullnews would be valid as well.

The problem for this use case is not related to any change in the core, but basically to the ambiguity of variable values in the routePath:

routePath: '/{news-year}-{news-month}-{news-day}-{news-title}'
  • /2020-1-2-wonderfullnews, is resolved correctly
tx_news_pi1__year = 2020
tx_news_pi1__month = 1
tx_news_pi1__day = 2
tx_news_pi1__news = wonderfullnews
  • /2020-1-2-wonderfull-news, starts getting tricky (due to the hyphen in the news title)
tx_news_pi1__year = 2020-1
tx_news_pi1__month = 2
tx_news_pi1__day = wonderfull
tx_news_pi1__news = news
  • /2020-1-2-wonder-full-news, same problem
tx_news_pi1__year = 2020-1-2
tx_news_pi1__month = wonder
tx_news_pi1__day = full
tx_news_pi1__news = news

Thus, using - at the same time as route delimiter and variable payload does not work.
If requirements were on the same level as defaults it would have worked - since \d+ denies using the hyphen -.

Conclusion: This behavior was not caused by the core, but was a result of the ambiguous configuration.

Actions #8

Updated by Oliver Hader almost 4 years ago

  • Category changed from Miscellaneous to Link Handling, Site Handling & Routing
Actions #9

Updated by Gerrit Code Review almost 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/+/64903

Actions #10

Updated by Gerrit Code Review almost 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/+/64903

Actions #11

Updated by Gerrit Code Review almost 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/+/64903

Actions #12

Updated by Gerrit Code Review almost 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/+/64903

Actions #13

Updated by Gerrit Code Review almost 4 years ago

Patch set 6 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/+/64903

Actions #14

Updated by Gerrit Code Review almost 4 years ago

Patch set 7 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/+/64903

Actions #15

Updated by Gerrit Code Review almost 4 years ago

Patch set 8 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/+/64903

Actions #16

Updated by Arne Bracht almost 4 years ago

Oliver Hader wrote:

....
Thus, using - at the same time as route delimiter and variable payload does not work.
If requirements were on the same level as defaults it would have worked - since \d+ denies using the hyphen -.

Conclusion: This behavior was not caused by the core, but was a result of the ambiguous configuration.

@Oliver:

thank you for your explanation. Now I understand it a little bit more :-). With _ instead of -,

routePath: '/{news-year}_{news-month}_{news-day}_{news-title}'

it works without the patch. With Patch set 8 it works again with - as a separator. Thanks a lot for that.

So is that right, that \d+ alows using the hyphen - again?

Actions #17

Updated by Till Wimmer almost 4 years ago

I would really appreciate a lot when one of the developers could also have a look at issue 91537. I'm not sure if this is related because the breaking change was in 9.5.15.

Actions #18

Updated by Arne Bracht almost 4 years ago

Till Wimmer wrote:

I would really appreciate a lot when one of the developers could also have a look at issue 91537. I'm not sure if this is related because the breaking change was in 9.5.15.

take a look here in the Patchnotes: [[https://review.typo3.org/c/Packages/TYPO3.CMS/+/64903]]

This issue is for the slash between variables in the route and look at Olivers first explanation in this thread.

Actions #19

Updated by Arne Bracht almost 4 years ago

  • Related to Bug #91537: 9.5.15+ breaks routing: Only respects the shortest matching rule added
Actions #20

Updated by Oliver Hader almost 4 years ago

Arne Bracht Bracht wrote:

[...]

it works without the patch. With Patch set 8 it works again with - as a separator. Thanks a lot for that.

You're welcome. I really hope these are the last issues we had with routing flaws...

So is that right, that \d+ alows using the hyphen - again?

Yes, having the requirements for all three data related variables set to \d+ means that these variables can only contain numbers, but no hyphen. That also would have solved the problem (in the example given requirements configuration was indented too much - see my comments above).

Actions #21

Updated by Till Wimmer almost 4 years ago

I have tested the patch 64903 and it solves the issue reported as bug #91537.

Thanks everyone.

Will this be available in the next 9.5. release?

Actions #22

Updated by Stefan P almost 4 years ago

I really hope these are the last issues we had with routing flaws...

Sorry, to disappoint you: #91711 :) (PageType decorator behaves inconsistently)

Actions #23

Updated by Gerrit Code Review almost 4 years ago

Patch set 9 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/+/64903

Actions #24

Updated by Arne Bracht over 3 years ago

  • Priority changed from Should have to Must have

So, the patch is not in 9.5.20, will it be in 9.5.21? Thx.

Actions #25

Updated by cosmoblonde GmbH over 3 years ago

Hi,

I had experienced issues with routeEnhancer definitions with 9.5.19 and 9.5.20 too and had successfully added the patch to the 9.5.20 version.

Today version 9.5.21 was released and I updated my system and was surprised to discover that the patch doesn't seem to be included? My routes break again...

Can please someone tell me why this patch hasn't made it into the newest release?

Thank you!
Ulrike

Actions #26

Updated by Gerrit Code Review over 3 years ago

Patch set 10 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/+/64903

Actions #27

Updated by Johannes Seipelt over 3 years ago

Because it was rather unclear for me, something like:
- /{year}-{month}
or
- /{year}/{month}

still works, if you have correct requirements set, like:

    defaults:
      month: '1'
      year: '2020'
    requirements:
      month: '\d+'
      year: '\d+'

I copied mine from the news docs where the requirements are set on the same level as _arguments:
https://docs.typo3.org/p/georgringer/news/master/en-us/AdministratorManual/BestPractice/Routing/Index.html#human-readable-dates

that way those requirements were are not applied at all. It just worked because of the core defaults before 9.5.19 (that's at least how I understand why it didn't broke before 9.5.19).

With the requirements on the same indentation level as the "defaults" its working now for me on 9.5.22 with "/" or "-" like before, without any patch.

Actions #28

Updated by cosmoblonde GmbH over 3 years ago

Hi,

I still have the same problems as described in comment #25 and none of the latest TYPO3 releases has integrated this patch that is working for me.

I compared the patched files with the latest release version 9.5.23 and I found out that by only adding the following check in the file typo3/sysext/core/Classes/Routing/Enhancer/AbstractEnhancer.php line 60

if (empty($requirements)) {
  return;
}

everything works for me again.

Here is an extract from my sites yaml configuration:

  BibliographiePlugin:
    type: Extbase
    extension: cb_bibliographie
    plugin: bibliographie
    routes:
      -
        routePath: '/fbg/liste/{chapter}/{facet/type}/{category}/{page}'
        _controller: 'Bibliographie::list'
    defaults:
      facet/type: '0'
      category: '522'
      page: '1'
    aspects:
      chapter:
        type: PersistedAliasMapper
        tableName: tx_cbbibliographie_domain_model_bibliographie
        routeFieldName: path_segment
      facet/type:
        type: StaticValueMapper
        default: 0
        map:
          alle_formate: 0
          buch: 1
          zeitung: 2
          sammlung: 3
          audio: 4
          video: 5
          vertonung: 6
      category:
        type: PersistedAliasMapper
        tableName: sys_category
        routeFieldName: slug
      page:
        type: StaticRangeMapper
        start: '1'
        end: '1000'
    defaultController: 'Bibliographie::hierarchicalList'

Only the first parameter ever works as expected. Any additional parameter is mapped correctly but the URL is returning a 404. Even changing the order of the parameters or the type of mapper is always producing the same error.

Is this additional existence check for $requirements something you can implement into the AbstractEnhancer class?

Can anyone confirm the issue?

Thank you!
Ulrike

Actions #29

Updated by Gerrit Code Review over 2 years ago

Patch set 11 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64903

Actions #30

Updated by Oliver Hader over 1 year ago

  • Sprint Focus set to On Location Sprint
Actions #31

Updated by Oliver Hader over 1 year ago

  • Assignee set to Oliver Hader
Actions #32

Updated by Benni Mack 9 months ago

  • Sprint Focus deleted (On Location Sprint)
Actions

Also available in: Atom PDF