Bug #91633
closed9.5.19 breaks formerly working routeEnhancer
Added by Matthias Krappitz over 4 years ago. Updated 4 months ago.
0%
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.
Updated by Matthias Krappitz over 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.
Updated by Arne Bracht over 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'
Updated by Oliver Hader over 4 years ago
- Related to Bug #91246: routeEnhancer defaults not applied in TYPO3 v9.5.16 added
Updated by Oliver Hader over 4 years ago
- Related to Bug #90531: Requirements are not considered when an aspect is present added
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/+/64903
Updated by Oliver Hader over 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
andrequirements
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
andrequirements
used until then, actually were not considered (probably copy&paste scenarios)requirements
for variables havingaspects
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
- https://review.typo3.org/c/Packages/TYPO3.CMS/+/64843
- restricting the disposal of "allow slashes in slugs" only for undefined
requirements
ofaspects
- the regression was caused by applying the greedy setting for undefined
requirements
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
Updated by Oliver Hader over 4 years ago
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.
Updated by Oliver Hader over 4 years ago
- Category changed from Miscellaneous to Site Handling, Site Sets & Routing
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/+/64903
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/+/64903
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/+/64903
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/+/64903
Updated by Gerrit Code Review over 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
Updated by Gerrit Code Review over 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
Updated by Gerrit Code Review over 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
Updated by Arne Bracht over 4 years ago
Oliver Hader wrote:
....
Thus, using-
at the same time as route delimiter and variable payload does not work.
Ifrequirements
were on the same level asdefaults
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?
Updated by Till Wimmer over 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.
Updated by Arne Bracht over 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.
Updated by Arne Bracht over 4 years ago
- Related to Bug #91537: 9.5.15+ breaks routing: Only respects the shortest matching rule added
Updated by Oliver Hader over 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).
Updated by Till Wimmer over 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?
Updated by S P over 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)
Updated by Gerrit Code Review over 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
Updated by Arne Bracht over 4 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.
Updated by cosmoblonde GmbH about 4 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
Updated by Gerrit Code Review about 4 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
Updated by Johannes Seipelt about 4 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.
Updated by cosmoblonde GmbH about 4 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
Updated by Gerrit Code Review almost 3 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
Updated by Oliver Hader about 2 years ago
- Sprint Focus set to On Location Sprint
Updated by Benni Mack over 1 year ago
- Sprint Focus deleted (
On Location Sprint)
Updated by Oliver Hader 4 months ago
- Status changed from New to Closed
I don't think this is required anymore.
The interest in the provided patch was low during last four years.