Updated: Comment #0

Problem/Motivation

MatcherDumper::MAX_PARTS denotes the maximum number of parts a path can have in the new routing system.

This limitation is carried over from the old menu system. It is one, that we should try to avoid, though, if possible. In config_translation.module we have already hit the limit and need to work around it by providing multiple levels of pages all on one route. (Instead of /.../translate/add/fr we have to do /.../translate?action=add&langcode=fr and then dispatch to the appropriate forms ourselves by checking the request query.)

Currently MatcherDumper::MAX_PARTS is only used in one place in core and it can very easily be avoided.

Proposed resolution

Remove MatcherDumper::MAX_PARTS

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#14 route_provider-2120505-14.patch3.14 KBdawehner
#15 route_provider-2120505-14-tests-only.patch1.46 KBtstoeckler
#14 interdiff-14.txt1.46 KBdawehner
#10 2120505-10.patch1.69 KBtstoeckler
#10 2120505-6-10-interdiff.txt1.11 KBtstoeckler
#6 2120505-6.patch1.29 KBtstoeckler
#6 2120505-1-6-interdiff.txt743 byteststoeckler
#1 2120505-1.patch1.1 KBtstoeckler

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review
StatusFileSize
new 2120505-1.patch1.1 KB

Let's see if PhpStorm is actually right and there is only one usage in core.

amateescu’s picture

amateescu commented 
+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -242,7 +242,7 @@ protected function getRoutesByPath($path) {
     $parts = array_slice(array_filter(explode('/', $path), function($value) {
       return $value !== NULL && $value !== '';
-    }), 0, MatcherDumper::MAX_PARTS);
+    }), 0);

Shouldn't we remove array_slice() entirely?

dawehner’s picture

dawehner
Primary language German
commented

It is easy to remove MatcherDumper::MAX_PARTS but how do we want to deal with MENU_MAX_DEPTH and MENU_MAX_PARTS as these are actually used in several places.

amateescu’s picture

amateescu commented

Yeah, menu links are not ready for that..

dawehner’s picture

dawehner
Primary language German
commented

On the other hand though these functions are indeed just used for menu links.
The other systems like local tasks/local actions/breadcrumbs now work independent from that and should not have any limitations.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Category: task » bug
StatusFileSize
new 2120505-1-6-interdiff.txt743 bytes
new 2120505-6.patch1.29 KB

Right, the fact that you can actually have routes with more parts, but that getRoutesByPath() will not find them is actually a bug report.

The fact that menu links will still have this limitation in D8 is unfortunate, but yeah...

Also super facepalm for #2, here's a fix for that.

Status: Needs review » Needs work

The last submitted patch, 2120505-6.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

#6: 2120505-6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2120505-6.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2120505-6-10-interdiff.txt1.11 KB
new 2120505-10.patch1.69 KB

So, the array keys get retained by array_filter() but not by array_slice(). Interesting.

While looking into RouteProvider::getCandidateOutlines() I realized that because we do bitwise comparison in there, we only support paths with 32/64 parts (depending on the system). This is already the case, however, so I think should not be changed in any way here. I.e. if you currently extend RouteProvider and simply do RouteProvider::MAX_PARTS = 100, you will notice the same thing.

Status: Needs review » Needs work

The last submitted patch, 2120505-10.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
Issue tags: +Random fail

BreakpointGroupCRUDTest.php, Line 29

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: -Random fail

#10: 2120505-10.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
StatusFileSize
new interdiff-14.txt1.46 KB
new route_provider-2120505-14.patch3.14 KB

Let's also extend the test coverage for it.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new route_provider-2120505-14-tests-only.patch1.46 KB

Awesome, thanks!
Uploading a tests-only patch to prove the validity of the tests.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Status: Needs review » Needs work

The last submitted patch, 15: route_provider-2120505-14-tests-only.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

Nicely fails the correct assertions.
Can't RTBC myself, though :-/

dawehner’s picture

dawehner
Primary language German
commented

Well ... your last 3 patches dropped the test coverage again.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

@dawehner: the last patch is in #14, the others are notifications of files *hidden*, not new files posted.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Also looks good to me.

dawehner’s picture

dawehner
Primary language German
commented

Oh, you are right.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Btw: #2132795: Remove \Drupal\Core\Routing\RouteCompiler::MAX_PARTS

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This indeed seems to be a holdover from the old routing system. No tests had to be changed to allow this to work, but we nicely get extra test coverage for crazy paths. :)

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.