Problem/Motivation
Views handler NumericFilter 'regular_expression' operator method is broken:
- it is wrongly set to op_regex() while the class method is actually opRegex().
- it passes the whole value
property which is an array, so the resulting SQL query is broken.
Using the regular_expression operator makes the system fatal (see #10).
Proposed resolution
Change op_regex() to opRegex()(done)Replace(done)$this->query->addWhere($this->options['group'], $field, $this->value, 'REGEXP');
with$this->query->addWhere($this->options['group'], $field, $this->value['value'], 'REGEXP');
Add tests(done)Check the patch for all supported databases(Test passes for MySQL and SQLite, fails on Postgresql #2845543: PostgreSQL regular expression match operators works only for text).Create change record(done: [#2845300])
Remaining tasks
Postponed due #2845543: PostgreSQL regular expression match operators works only for text.
User interface changes
API changes
Module developers extending or using or relying on NumericFilter::op_regex() must update their code as the method has been renamed to opRegex() (CR: [#2845300]).
Data model changes
Original post
Problem/Motivation
In Views at the moment when using a filter the class NumericFilter the REGEX filtering method is broken, first it can only used only after applying the patch from #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex', even then if used with a Date will not work because it will pass the whole value property which is an array, so the resulting SQL query is broken.
Very easy fix attached, we need test, but some are being added in #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex' so we can follow up on tests based on that.
Comments
Comment #2
esolitosPatch!
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedWhen i'm created tests for #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex', I also have two different syntax:
After your patch we have one syntax and it looks nice. Thanks! But why did you create a separate issue, instead of solving the problem in one issue?
In any case, i'm attached patch with tests for both correction (opRegexp and value['value']). But imho, one of these issues is duplicate.
Comment #4
jazzfiction CreditAttribution: jazzfiction commentedI can also verify the the issue and that the patch works. I independently did the same fixes on my own test install, and it fixed the issue(s).
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedNumericFilter have complex value (it is array with "min, max, value, type" keys) unlike value of StringFilter and Combine. And we must use
$this->value['value']
as elsewhere in this class. I am glad that @esolitos found it.Now everything is all right. And this issue is independent from #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex' because fixed "op_regexp" too. Also we have tests and reviews. Therefore, i'm changed status to RTBC.
But when it will commit, hopefully not forget the main hero - @SylvainM, who first found and prepare fix this problem by #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex'. Thanks!
Comment #7
alexpott@vaplas thanks for all your work on this - rtbcs are meant to be done by someone who has not worked on the patch - you've worked on both of the issues.
Missing function comments describing what is being tested
Unnecessary spaces.
Adding credit to @SylvainM due to #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex' which I've marked as duplicate of this one.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commented@alexpott, thanks for your review and very sorry for my defective patch and misuse rtbc. I tried to add comments, but not sure in my literacy. If someone will make them here, I will change without problems.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #10
gambryThis issue is a [potential] blocker for #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields.
Also raising this to major as using 'regex' as operator (on any plugin using/extending NumericFilter without implementing the method properly) makes the system fatal:
Fatal error: Call to undefined method Drupal\datetime\Plugin\views\filter\Date::op_regex() in core/modules/views/src/Plugin/views/filter/NumericFilter.php on line 242
To reproduce:
Finally as we are changing the operator method from
op_regex
toopRegex
and potentially breaking any code relying on old method definition, do we need a change record?Comment #11
gambryIt's not a 'field' filter, I would rather use the class description: "numeric filter handler" for consistency.
Same as above.
Really good work!
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for review and proposed improvements!
Comment #14
jonathanshawTest fail looks random
Comment #15
gambryEven though I feel nervous without a green. :)
Besides issue still needs the change record to alert Module developers about the operator method name change.
Also this random test doesn't look to be listed on #2829040: [meta] Known intermittent, random, and environment-specific test failures ?
Could that be a new one?
Comment #16
gambryComment #17
gambryTest is green and change record has been created: NumericFilter views handler 'regular_expression' operator method renamed. Back to RTBC.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedNeat work! I also added the post about random fail for conscience sake. Perhaps this is the case, when the CR is not needed, because
op_regex
just did not work. But anyway, this addition certainly not harm.Comment #19
gambry@vaplas
op_regex
did not work on NumericFilter and extending classes in core, but if any contrib or custom module extends NumericFilter and overrides the method than we would break those.Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commented@gambry, thanks for clarifying! It makes sense for contrib and custom modules. Among all the contrib modules, there is one that do this (sandbox ip), and one that believes that "the world is easy" :) (fraction)
Comment #21
LendudeReally nice work. Just some minor nitpicks with this, and then I agree with the RTBC:
missing period at the end, and not a really helpful comment as is. Maybe add why/what we are changing and expecting from this change.
Why are we rebuilding the router when we don't actually use the router? Test passes fine without this. I see the other tests in that file are doing this too, but we shouldn't add this unless it's really needed. (And we should open a follow up to clean the rest of the tests that don't need this).
I also queued the patch for all supported databases, never hurts when messing with queries that use regexes.
Comment #22
LendudeYup, new test fails on Postgres, so that needs work too.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commented@Lendude, great point, thanks!
I'm not sure in my new comment, so do not mind if someone can help with this.
Comment #24
gambryThis is probably the best place where to cast the db column, however the comment refers to a completely different issue.
I suggest to update the comment OR separate the 2 replace calls with something like:
UPDATE:
Coding standards for in-code comments require a final full stop at the end of period.
// Filtering by regular expression pattern.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedOk. Let's extend all Regular Expression Match Operators + SIMILAR.
Comment #26
gambryDrupal pgsql driver doesn't have 'SIMILAR TO' operator, nor regexp operators other than '~*' so I wouldn't process them.
Also I think we need the intervention of a pgsql driver maintainer to sign off this change.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedLet's solve the problem with PostgreSQL separately #2845543: PostgreSQL regular expression match operators works only for text, and this issue be postponed.
Comment #28
Lendude@vaplas++ (unless we can think of a way to somehow work around this in Views, but nothing really comes to mind).
Comment #29
gambrySo let's remove the tag 'Needs subsystem maintainer review' and make some progress in #2845543: PostgreSQL regular expression match operators works only for text.
Then the patch for this issue is now reverted to #12 + changes request from #21.
Comment #30
gambryIn addition to:
I'm tagging Needs issue summary update due #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex' being merged from #7-#8 but this issue summary still doesn't mention it.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedHere is another similar issue #2650964: Fix the execution of regular expression. I suggest it is also closed as a duplicate (although it appeared earlier and a little different in the tests). With adding credit to @yongt9412, like #6.
Absolutely agree with needs issue summary update (and title too). Because this issue appeared as an addition to #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex', and ironically, took the lead. So it would be good to adjust IS. But I'm not good at it :(
Done in this patch. Temp change status on NR for testbot trigger.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #34
gambryIssue summary should be a mix of original summary (focusing on '[...] it will pass the whole value property which is an array, so the resulting SQL query is broken.') + the summary of #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex'.
Comment #35
gambryComment #36
nicholas.alipaz CreditAttribution: nicholas.alipaz commentedHere is a version of the patch against 8.2.x branch. This is just for testing some other dependent issues and possibly for others whom might need it in 8.2.
Comment #37
jibran#2800727: Views NumericFilter regular_expression operator wrong method 'op_regex' is closed as a duplicate. The patch looks good to me. RTBC #27. Re-uploading the patch.
Comment #38
gambry@jibran this is postponed due #2845543: PostgreSQL regular expression match operators works only for text. Applying this patch without that one cause tests failure for PostgreSQL.
Comment #39
jibranSorry, I missed that one. Thanks for the reply
Comment #40
gambryBlocker has been committed. Let's move to Needs Review to trigger test bot.
Comment #41
gambryAll green, back to RTBC.
Comment #42
gambryComment #43
mparker17Removing the [PP-1] in the title since the blocker has been committed.
Comment #44
alexpottCommitted and pushed 3da27e5 to 8.4.x and 32eea0e to 8.3.x. Thanks!