Steps to Reproduce

  1. Add a field with a very long title/name like field_01234567890123456789012345 to any content type.
  2. Create a View with a page and a menu item that uses this field as a Contextual Filter
  3. Click save
  4. 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

CommentFileSizeAuthor
#16 2161727-16-PASS.patch11.59 KBdamiankloip
#16 2161727-16-test-only-FAIL.patch2.03 KBdamiankloip
#11 vdc-2161727-views-with-long-argument-names-11.patch9.56 KBjrockowitz
#9 vdc-2161727-views-with-long-argument-names-09.patch6.11 KBjrockowitz
#6 vdc-2161727-views-with-long-argument-names-06.patch6.01 KBjrockowitz
#1 vdc-2161727.patch4.16 KBdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
Issue tags: +VDC
StatusFileSize
new vdc-2161727.patch4.16 KB

This is indeed an interesting bug.

This is just a rough ID how to fix it.

Status: Needs review » Needs work

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

jrockowitz’s picture

jrockowitz commented

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'.

jrockowitz’s picture

jrockowitz commented

DUPLICATE

dawehner’s picture

dawehner
Primary language German
commented

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.

jrockowitz’s picture

jrockowitz commented
Issue summary: View changes
StatusFileSize
new vdc-2161727-views-with-long-argument-names-06.patch6.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.

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review
StatusFileSize
new vdc-2161727-views-with-long-argument-names-09.patch6.11 KB

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

Status: Needs review » Needs work

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

jrockowitz’s picture

jrockowitz commented
Status: Needs work » Needs review
StatusFileSize
new vdc-2161727-views-with-long-argument-names-11.patch9.56 KB

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

dawehner’s picture

dawehner
Primary language German
commented
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.

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#1709960: declare a maximum length for entity and bundle machine names

Related issue: #1709960: declare a maximum length for entity and bundle machine names

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#2120003: [META] Create sensible limits for the maximum length of configuration object filename components

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

xjm’s picture

xjm
she/her
Primary language English
commented
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!

damiankloip’s picture

damiankloip commented
Issue tags: -Needs tests
StatusFileSize
new 2161727-16-test-only-FAIL.patch2.03 KB
new 2161727-16-PASS.patch11.59 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.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review

The last submitted patch, 16: 2161727-16-test-only-FAIL.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I really hope that this is high enough in the chain.

webchick’s picture

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

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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