Views HandlerBase::breakString works unexpectedly for a views with a huge amount of filters/parameters [#3409401]

Problem/Motivation

Views that generate requests with a huge amount of arguments (more than 3500) give WSOD.
The issue was reproduced on commerce order view with the amount of order items more than 3500.

Steps to reproduce

It was reproduced on commerce order view with a number of order items of more than 3500, so
1. Install commerce module
2. Create an order with the number of order items more than 3500.
3. Go to the order view page.

Expected result

Page (view) opened.

Actual result

The website encountered an unexpected error. Please try again later.
Error in the watchdog: TypeError: Cannot assign null to property Drupal\views\Plugin\views\argument\ArgumentPluginBase::$operator of type string in Drupal\views\Plugin\views\argument\NumericArgument->title() (line 63 of /usr/www/users/cbefce/web/core/modules/views/src/Plugin/views/argument/NumericArgument.php).

Root cause of issue

web/core/modules/views/src/Plugin/views/HandlerBase::breakString gives unexpected results in case of using a string with a huge amount of filter parameters, like "N1+N2+N3....+N3500" (it breaks somewhere at a number of filters ~ 3500).
The reason for that is preg_match can't handle a long string: For the longer string, the issue could be related to the length of the string causing a problem in PHP's regex engine. PHP has a limit on the size of the string it can process with regular expressions, and very long strings can lead to unexpected behavior, such as failing to match.

Proposed resolution

Fix web/core/modules/views/src/Plugin/views/HandlerBase::breakString to avoid using preg_match.

Comment	File	Size	Author
#36	views-handlerbase-breakstring-3409401-36.patch	5.63 KB	grapefruitjuice
#19	3409401-nr-bot.txt	90 bytes	needs-review-queue-bot
#3	drupal-3409401-2.patch	1.94 KB	Ihor Ukhan
#2	drupal-3409401-1.patch	1.91 KB	Ihor Ukhan

Issue fork drupal-3409401

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3409401-drupalviewspluginviewshandlerbasebreakstring-works-unexpectedly changes, plain diff MR !8417
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

18 December 2023 at 08:37

Igor Ukhan created an issue. See original summary.

Comment #2

Ihor Ukhan commented 18 December 2023 at 08:52

Status	File	Size
new	drupal-3409401-1.patch	1.91 KB

Here is the path to replace preg_match in the Drupal\views\Plugin\views/HandlerBase::breakString
All the tests Drupal\Tests\views\Functional\HandlerHandlerTest::testBreakString passed

Comment #3

Ihor Ukhan commented 18 December 2023 at 09:40

Status	File	Size
new	drupal-3409401-2.patch	1.94 KB

Here is the updated patch that passed all the tests

Comment #4

Ihor Ukhan commented 18 December 2023 at 09:41

1 file was hidden/shown/deleted

Status	File	Size
hidden	drupal-3409401-1.patch	1.91 KB

Comment #5

18 December 2023 at 09:41

Version:

10.1.x-dev

» 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #6

14 June 2024 at 17:17

naveenvalecha made their first commit to this issue’s fork.

Comment #7

14 June 2024 at 17:26

naveenvalecha opened merge request !8417

Comment #8

naveenvalecha

He/Him

commented 14 June 2024 at 17:26

Status:

Active

» Needs review

I have used #3 and it worked like a charm for me. It also fixed the issue for filtering a string containing '':" in it.
Example - topic:sci-fi+topic:fantasy

I have converted #3 patch to a PR https://git.drupalcode.org/project/drupal/-/merge_requests/8417

Comment #9

naveenvalecha

He/Him

commented 14 June 2024 at 17:39

Assigned:	Ihor Ukhan	» Unassigned
Status:	Needs review	» Reviewed & tested by the community

Setting it to RTBC as the work in #8 is not mine

Comment #10

catch

he/him

English

commented 5 July 2024 at 10:12

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

This could use some test coverage if possible.

Comment #11

smustgrave commented 11 July 2024 at 13:39

Status:

Needs review

» Needs work

For the test coverage

Comment #12

12 July 2024 at 12:43

Akhil Babu made their first commit to this issue’s fork.

Comment #13

akhil babu

he/him

commented 12 July 2024 at 13:00

Status:

Needs work

» Needs review

Added tests.

Comment #14

lendude

Dutch

Amsterdam

commented 8 August 2024 at 06:50

Status:

Needs review

» Needs work

Looks good, one more test case would be nice.

Can we come up with other cases that the regex might have missed like in #8 and make sure we are ok with that?

Comment #15

8 August 2024 at 10:46

bharath-kondeti made their first commit to this issue’s fork.

Comment #16

bharath-kondeti commented 8 August 2024 at 11:42

Status:

Needs work

» Needs review

Comment #17

smustgrave commented 13 August 2024 at 15:21

Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-Needs tests	+Needs Review Queue Initiative

believe feedback for additional test coverage has been addressed.

Comment #18

quietone commented 15 August 2024 at 02:32

Title:

Drupal\views\Plugin\views\HandlerBase::breakString works unexpectedly for a views with a huge amount of filters/parameters in a view

» Views HandlerBase::breakString works unexpectedly for a views with a huge amount of filters/parameters

Trying for an easier to read title

Comment #19

needs-review-queue-bot commented 9 October 2024 at 16:00

Status:

Reviewed & tested by the community

» Needs work

Status	File	Size
new	3409401-nr-bot.txt	90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Comment #20

11 November 2024 at 07:54

shalini_jha made their first commit to this issue’s fork.

Comment #21

shalini_jha commented 11 November 2024 at 08:37

Status:

Needs work

» Needs review

I reviewed the merge request and found some conflicts, so I rebased and resolved them. The pipeline has also passed successfully, so I’m moving it back to "Needs Review."

Comment #22

smustgrave commented 11 November 2024 at 16:49

Status:

Needs review

» Reviewed & tested by the community

Rebase seems fine.

Comment #23

quietone commented 6 December 2024 at 08:35

Status:

Reviewed & tested by the community

» Needs work

2 files were hidden/shown/deleted

Status	File	Size
hidden	drupal-3409401-2.patch	1.94 KB
hidden	3409401-nr-bot.txt	90 bytes

I left some comments in the MR. I also updated credit.

Comment #24

shalini_jha commented 9 December 2024 at 06:50

Status:

Needs work

» Needs review

I have reviewed all the feedback and implemented the necessary changes accordingly. After applying the updates, I re-ran the pipeline, and it is working as expected. I have also verified the empty string case, and it is functioning correctly without any issues. Moving this forward for your review. Kindly review.

Comment #25

smustgrave commented 26 December 2024 at 19:06

Status:

Needs review

» Reviewed & tested by the community

Feedback appears to be addressed

Comment #26

alexpott

he/they

English

🇪🇺🌍

commented 12 January 2025 at 12:10

Status:

Reviewed & tested by the community

» Needs work

Made a suggestion to change the code on the MR.

Comment #27

shalini_jha commented 13 January 2025 at 07:50

Status:

Needs work

» Needs review

I have updated the if condition to handle the scenario, and after testing, the test is now passing successfully. I am moving this for review. Please take a look and let me know if it makes sense to you.

Comment #28

smustgrave commented 28 January 2025 at 18:47

Status:

Needs review

» Needs work

For the scenario you saw a failure in can we add test coverage for that one too please.

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Comment #29

shalini_jha commented 29 January 2025 at 13:28

Thank you @smustgrave for the review & feedback. the scenario i have mentioned is the existing test that is failing after the code change

Comment #30

shalini_jha commented 29 January 2025 at 13:34

Status:

Needs work

» Needs review

I am moving this for review. Please take a look and let me know if it makes sense, or if any additional tests need to be added?

Comment #31

smustgrave commented 29 January 2025 at 14:34

Status:

Needs review

» Reviewed & tested by the community

Nope think that should be good.

Comment #32

longwave

he/him

English

commented 5 March 2025 at 12:53

Status:

Reviewed & tested by the community

» Needs work

Some nits about comments.

Comment #33

shalini_jha commented 5 March 2025 at 13:36

Status:

Needs work

» Needs review

I have addressed all the feedback, including updating the comment for the single-word condition to improve clarity. Please let me know if we need to assert the single word in the test for further clarification.

Moving this to NR , Kindly review.

Comment #34

smustgrave commented 6 March 2025 at 15:13

Status:

Needs review

» Reviewed & tested by the community

Believe feedback has been addressed

Comment #35

quietone commented 4 April 2025 at 06:29

Status:

Reviewed & tested by the community

» Needs work

https://git.drupalcode.org/project/drupal/-/merge_requests/8417/diffs#note_442553 @alexpott pointed out cases where the new code and the old produce different results. I don't see those test cases. If they are the, add a comment in the code.

To test that the results have not change I removed the changes from breakString and all the long string test cases from testBreakString. I expected the test to pass, it did not. These are the cases that failed.

HandlerBase::breakString('word:word1+word:word2+word:word3');
HandlerBase::breakString('word:word1 word:word2 word:word3');
HandlerBase::breakString('word:word1,word:word2,word:word3');
HandlerBase::breakString('word:word1');

How will this change of behavior affects sites?

Comment #36

grapefruitjuice commented 13 May 2025 at 07:02

Status	File	Size
new	views-handlerbase-breakstring-3409401-36.patch	5.63 KB

Until we are waiting for upgrading to D11, on the 10.4.x branch(we tested it on the 10.4.7 version) we have an error when we're trying to apply patches from this topic:

patching file modules/views/src/Plugin/views/HandlerBase.php
Hunk #1 FAILED at 742.
1 out of 1 hunk FAILED -- saving rejects to file modules/views/src/Plugin/views/HandlerBase.php.rej

So, I've updated the patch to the particular 10.4.x branch, coz we really need it and could not wait until we move to 11 version (I took the code from the pull request).

If anybody has the same problem with patching 10.4.x core, feel free to use the updated patch.

Comment #37

13 May 2025 at 07:02

Version:

11.x-dev

» main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.