Problem/Motivation
Usecase:
There are three nodes with the following titles: foo, bar, drupal. Requirement: Create a view that lists nodes that does not have title foo or bar.
Using views string filter you can't create a SQL expression like "some_field NOT IN ('foo', 'bar')", as a workaround when searching a multiple value string in a string Regular Expression operation can be chosen but it's hard to implement a reverse search for regex because the regex is done on SQL level which is limited according to https://dev.mysql.com/doc/refman/5.7/en/regexp.html
POSIX regexes don't support using the question mark ? as a non-greedy (lazy) modifier to the star and plus quantifiers like PCRE (Perl Compatible Regular Expressions). This means you can't use +? and *?
http://stackoverflow.com/questions/18317183/1139-got-error-repetition-op...
Proposed resolution
Implement negation for REGEXP operation, it can be found in documentation as NOT REGEXP.
NOT REGEXP Negation of REGEXP
https://dev.mysql.com/doc/refman/5.7/en/regexp.html
Remaining tasks
Requeue tests for 8.8.x branch and more DBs.Final review / RTBCCommit
User interface changes
Adds a new "Negated Regular expression" option to the choices for the operation to use for Views string and integer filters.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#38 | 2853188-38.patch | 8.76 KB | enyug |
#37 | 2853188-37.patch | 8.76 KB | Anas_maw |
#30 | 2853188-14.patch | 8.7 KB | quietone |
#30 | 2853188-14-fail.patch | 6.24 KB | quietone |
Issue fork drupal-2853188
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
SurfinSpirit CreditAttribution: SurfinSpirit at FFW commentedComment #3
dawehnerDo you mind expanding the existing test coverage for it?
Comment #4
SurfinSpirit CreditAttribution: SurfinSpirit at FFW commentedNoticed there is one more filter that has regex option NumericFilter. Added the same functionality there as well.
Note. There is no test case for StringFilter that uses regular_expression operation.
@dawehner do you suggest creating a new test case or extending existing one to include a new filter?
Comment #5
dawehnerGood catch. Let's ensure though we don't forget the tests :)
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commented@SurfinSpirit, cool!
We need a new tests for NumericFilter and StringFilter, like #2821112: Views NumericFilter 'regular_expression' operator is broken. Because it new operator.
It should be
$this->value['value']
in NumericFilter.Also we need check the patch for all supported databases. Now it fails on PostgreSQL. We need fix it via separate issue (with tests), like #2845543: PostgreSQL regular expression match operators works only for text.
But we haven't
NOT REGEXP
converter. Hense we also needs add it here like'NOT REGEXP' => ['operator' => '!~*'],
and create test for it too.
Comment #7
SurfinSpirit CreditAttribution: SurfinSpirit at FFW commentedComment #8
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedUpdates to patch as per #6.
Comment #9
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedLine exceeding 80 characters.
Line exceeding 80 characters.
Comment #10
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedComment #11
Anonymous (not verified) CreditAttribution: Anonymous commented@Munavijayalakshmi, thanks for cleanup.
@aerozeppelin, nice work, it's really almost cover first half of #6. But we also need to add a GroupedExposed* tests for FilterNumericTest and FilterNumericTest. Like with other operators. E.g.:
See getGroupedExposedFilters() for do it.
Also we need open new issue (Feature request) about support
NOT REGEXP
operator in PostgreSQL. See PostgreSQL fail in #8. Because while we have problem with one of supported database, we can not add code to the core.Comment #12
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commented@vaplas, thank you for the review. I've tried to update the patch with comments in #11. Let's see what the test bot thinks.
Comment #14
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedFix for the failing test in #12.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commented@aerozeppelin thank you, last patch looks perfect. I opened #2860644: Add support of NOT REGEXP operator to PostgreSQL + fix testRegexCondition. And we need solve it, before change status of current issue to RTBC.
Comment #20
dwwStill applies to 8.7.x(!) and works great. Would love to get this in.
Looks like #2860644: Add support of NOT REGEXP operator to PostgreSQL + fix testRegexCondition got close, but no longer applies. I don't personally care about pgsql, but maybe we can get that in soonish...
Comment #22
dwwa) Yay, #2860644: Add support of NOT REGEXP operator to PostgreSQL + fix testRegexCondition is now in, so this is no longer blocked.
b) More accurate title, since this supports both string and integer filters.
c) I'll requeue tests for modern branches + environments, but I think it's all going to be green.
d) Fairly careful (but not utterly pedantic) review doesn't uncover any problems that need addressing. This is probably RTBC as-is. The actual code changes are very small and simple. There's basic test coverage for everything. The coverage could potentially be improved (along the lines of the tests in #2860644) to have a data provider and cover more possible cases, but we've at least got some coverage of the new option, which should be enough for commit.
Anyway, I'll come back to RTBC once the bot's happy.
Thanks,
-Derek
Comment #23
dwwFixing the summary to be more accurate.
Comment #28
muaz91Hi,
Testing out the patch from #14. it works for me. test with drupal 9.3
+1 for RTBC
Comment #29
muaz91Comment #30
quietone CreditAttribution: quietone at PreviousNext commentedThis needs to be on 10.0.x now. Changing version. The patch still applies so I downloaded it and made a fail patch as well.
There are tests so removing tag.
Comment #32
dwwThanks for finding this issue and getting it moving again, and for the test-only version of the patch! All looks great. Definitely RTBC. Will be very happy to see this land!
Saving credit for all the contributions here: patches + reviews. So far, everyone has meaningfully contributed, thanks! 🎉
Comment #33
dwwp.s. confirming that the fail patch is producing the right kinds of failures: Ringo keeps showing up! 😂
Comment #34
LendudeVery nitty nits
> 80 characters
This doesn't have doc block. I know a lot of the tests in this file don't have any, but I *think* we still need one for all new methods?
Queued on all databases again, cause want to be sure we still have support on all databases for this.
Comment #35
alexpottLet's address the nits in #34 and this one too...
More than a single line.
Plus as something new we should have a change record to announce the new thing we can do... see the "add change notice" link above.
Comment #36
BramDriesenDrafted a change record here: https://www.drupal.org/node/3314465
Still needs work for #34 & #35
Comment #37
Anas_maw CreditAttribution: Anas_maw at Coders Enterprise Web & Mobile Solutions commentedReroll patch in #30 to work on the latest 9.5 version
Comment #38
enyug CreditAttribution: enyug as a volunteer and at Service NSW commentedpatch for core 10.0.9
Comment #39
BramDriesenShouldn't this issue be targeting 10.1.x or 11.x?
Comment #40
BramDriesenComment #42
BramDriesenComment #43
smustgrave CreditAttribution: smustgrave at Mobomo commented2 nitpicky changes in the MR.
Going to go ahead and mark it if the committer could make that one word change.
Seems points in #34 and #35 have been addressed as well.
Comment #44
BramDriesenFixed one more nit. The dropdown items were showing
Negated Regular expression
, no need to have Regular with a capital so changed that to lowercase. Also fixed the nits with the suggestion of @smustgrave in #43.Comment #46
longwaveCommitted and pushed 4516d62542 to 11.x. Thanks!
Also published the change record.
Comment #47
BramDriesenThanks @longwave! Also updated the issue summary (remaining tasks).