Views that use fields with long names (ie 32 characters) will throw a show-stopping error [#2161727]

Steps to Reproduce

Add a field with a very long title/name like field_01234567890123456789012345 to any content type.
Create a View with a page and a menu item that uses this field as a Contextual Filter
Click save
Site will return the below error and stop working

Warning: preg_match(): Compilation failed: subpattern name is too long (maximum 32 characters) at offset 56 in Drupal\Core\Routing\RouteProvider->getRoutesByPath() (line 257 of core/lib/Drupal/Core/Routing/RouteProvider.php).

Notes

The regex pattern that is failing to compile is #^/test(?:/(?P<arg_field_01234567890123456789012345_value>[^/]++))?$#s
The regex pattern contains a subpattern greater than 32 characters because the 32 character field name is prefixed with 'arg_' and suffixed with '_value'
The subpattern suffix '_value' can change depending on the type field and requested data.
There is no way to allow 32 character subpatterns in regex.
This is a known Symfony issue, but it should not be bringing down a Drupal website.
I have searched Drupal.org and Google and this does not seem like a very common issue.
My guess is that Views' contextual filters is the only place in core that is going to run into this problem.
Marking this as major since it will crash a website.

Workarounds

Don't use long field names. Field names with less than 20 characters will avoid this issue.

Possible Solutions

Use subpatterns based on field names.
Pass all subpatterns through md5() which returns a 32 character string... this is an ugly solution.
Use an arguments index instead of its name

References

https://github.com/symfony/symfony/issues/8933

Comment	File	Size	Author
#16	2161727-16-PASS.patch	11.59 KB	damiankloip
#16
#16	2161727-16-test-only-FAIL.patch	2.03 KB	damiankloip
#16
#11	vdc-2161727-views-with-long-argument-names-11.patch	9.56 KB	jrockowitz
#11
#9	vdc-2161727-views-with-long-argument-names-09.patch	6.11 KB	jrockowitz
#9
#6	vdc-2161727-views-with-long-argument-names-06.patch	6.01 KB	jrockowitz
#6
#1	vdc-2161727.patch	4.16 KB	dawehner
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dawehner

German

CreditAttribution: dawehner commented 23 December 2013 at 22:31

Status:	Active	» Needs review
Issue tags:		+VDC

File	Size
vdc-2161727.patch	4.16 KB

This is indeed an interesting bug.

This is just a rough ID how to fix it.

Comment #2

24 December 2013 at 00:24

Status:

Needs review

» Needs work

The last submitted patch, 1: vdc-2161727.patch, failed testing.

Comment #3

jrockowitz CreditAttribution: jrockowitz commented 26 December 2013 at 12:54

Thanks for taking a crack at this one,

I tested the patch ad it is not working because the 4 character 'arg_' prepended to the 32 character md5() hash is 36 characters long. This still throws a regex compilation error.

When I remove the 'arg_' prefix from 'arg_' . md5($arg_id);, the site loaded with no errors but I could not get contextual arguments for the 32 character field to work.

I am not very familiar with D8 and Symfony but it seems that the $view_argument_map is being created but not used to map the 'md5' parameters to their names when a request is coming in.

Finally, this is a very minor tweak but since this issue may affect contrib modules as well, the '$view_argument_map' could be call just '$argument_map'.

Comment #4

jrockowitz CreditAttribution: jrockowitz commented 26 December 2013 at 12:55

~~DUPLICATE~~

Comment #5

dawehner

German

CreditAttribution: dawehner commented 27 December 2013 at 18:44

I tested the patch ad it is not working because the 4 character 'arg_' prepended to the 32 character md5() hash is 36 characters long. This still throws a regex compilation error.

should we just truncate the md5 generated hash? We use arg_ in other places.

I am not very familiar with D8 and Symfony but it seems that the $view_argument_map is being created but not used to map the 'md5' parameters to their names when a request is coming in.

I don't really care ...

Feel free to work on this patch, I just wanted to get some idea out, so other people don't have to research where the change would be needed.

Comment #6

jrockowitz CreditAttribution: jrockowitz commented 6 January 2014 at 16:21

Issue summary:

View changes

File	Size
vdc-2161727-views-with-long-argument-names-06.patch	6.01 KB

dawehner, you are right, I just need to bite the bullet and see what I can do to help fix this issue.

Your patch definitely pointed me in the right direction.

Basically the below objects and their related tests need to be tweaked to fix this issue.

\Drupal\views\Plugin\views\display\PathPluginBase
\Drupal\views\Tests\Plugin\display\PathPluginBaseTest.php
\Drupal\views\Routing\ViewPageController
\Drupal\views\Tests\Routing\ViewPageControllerTest.php

I think my suggestion to use an md5() hash to limit the parameter name's length is the wrong solution. Since a views arguments are 'indexed' and this order will never change, it is possible that the arg's index can be used as the route's parameter name instead of the view full argument name.

For example, in the current view tests the 'test_id' argument converts to `test_route/{arg_test_id}`, I think it can be simplified to just be `test_route/{arg_0}`

Attached is a patch implementing this solution with all the existing tests updated to look for arg_0 instead arg_{name}. I did remove your test since I am no longer using the '_view_argument_map' option. I am unsure if a new test is needed since this approach is not adding new functionality.

I just noticed after I uploaded my patch that PHPStorm added some extra data to the file. Since I can't delete the uploaded patch, I am going to let the 'test bot' test it and then I will make sure to upload a cleaner patch.

Comment #7

jrockowitz CreditAttribution: jrockowitz commented 7 January 2014 at 04:03

Status:

Needs work

» Needs review

Comment #8

7 January 2014 at 04:05

Status:

Needs review

» Needs work

The last submitted patch, 6: vdc-2161727-views-with-long-argument-names-06.patch, failed testing.

Comment #9

jrockowitz CreditAttribution: jrockowitz commented 7 January 2014 at 10:28

Status:

Needs work

» Needs review

File	Size
vdc-2161727-views-with-long-argument-names-09.patch	6.11 KB

Uploading same patch generated using git command instead of PHPStorm IDE.

Comment #10

7 January 2014 at 11:52

Status:

Needs review

» Needs work

The last submitted patch, 9: vdc-2161727-views-with-long-argument-names-09.patch, failed testing.

Comment #11

jrockowitz CreditAttribution: jrockowitz commented 8 January 2014 at 14:16

Status:

Needs work

» Needs review

File	Size
vdc-2161727-views-with-long-argument-names-11.patch	9.56 KB

Fixing broken tests in Drupal\views\Tests\Plugin\DisplayPageTest

Comment #12

dawehner

German

CreditAttribution: dawehner commented 8 January 2014 at 16:46

Status:

Needs review

» Reviewed & tested by the community

Yeah this seems to be the only really possible solution, don't use the argument ID but just some index in the pattern.

Comment #13

xjm

she/her

English

CreditAttribution: xjm commented 10 January 2014 at 17:37

Comment #14

xjm

she/her

English

CreditAttribution: xjm commented 10 January 2014 at 17:39

Also #2120003: [META] Create sensible limits for the maximum length of configuration object filename components.

Comment #15

xjm

she/her

English

CreditAttribution: xjm commented 10 January 2014 at 18:03

Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs tests

Reviewed with @Dries, @webchick, and @effulgentsia. The approach in this patch looks like a good solution; the other issues I've linked wouldn't solve this even if they were resolved. However, this issue is missing an explicit regression test to prove the functional bug with long fieldnames is now resolved. So let's get an automated test that fails before the patch and passes after. Thanks!

Comment #16

damiankloip CreditAttribution: damiankloip commented 11 January 2014 at 09:33

Issue tags:

-Needs tests

File	Size
2161727-16-test-only-FAIL.patch	2.03 KB

2161727-16-PASS.patch	11.59 KB

4 files were hidden/shown/deleted

File	Size
vdc-2161727.patch	4.16 KB

vdc-2161727-views-with-long-argument-names-06.patch	6.01 KB

vdc-2161727-views-with-long-argument-names-09.patch	6.11 KB

vdc-2161727-views-with-long-argument-names-11.patch	9.56 KB

I don't see too much point in adding this, as the internal implementation is completely changing, making the argument ID irrelevant. Anyway, here's a test for that.

No interdiff as the test only patch is the interdiff.