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

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

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

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

This could use some test coverage if possible.

For the test coverage

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

Added tests.

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?

believe feedback for additional test coverage has been addressed.

Trying for an easier to read title

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.

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 to "Needs Review."

Rebase seems fine.

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

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.

Feedback appears to be addressed

Made a suggestion to change the code on the MR.

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.

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!

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

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?

Nope think that should be good.

Some nits about comments.

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.

Believe feedback has been addressed

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.

1. HandlerBase::breakString('word:word1+word:word2+word:word3');
2. HandlerBase::breakString('word:word1 word:word2 word:word3');
3. HandlerBase::breakString('word:word1,word:word2,word:word3');
4. HandlerBase::breakString('word:word1');

How will this change of behavior affects sites?