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

  1. Change op_regex() to opRegex() (done)
  2. Replace $this->query->addWhere($this->options['group'], $field, $this->value, 'REGEXP'); with $this->query->addWhere($this->options['group'], $field, $this->value['value'], 'REGEXP'); (done)
  3. Add tests (done)
  4. 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).
  5. 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.

CommentFileSizeAuthor
#37 views_numericfilter-2821112-37.patch3.68 KBjibran
#37 2821112-core.patch3.79 KBjibran
#36 core_8.2-views_numericfilter_operator-2821112-36-do-not-test.patch984 bytesnicholas.alipaz
#32 interdiff-12-32.txt1009 bytesAnonymous (not verified)
#32 2821112-32.patch3.68 KBAnonymous (not verified)
#25 interdiff-23-25.txt1.97 KBAnonymous (not verified)
#25 2821112-25.patch4.98 KBAnonymous (not verified)
#23 interdiff-12-23.txt1.87 KBAnonymous (not verified)
#23 2821112-23.patch4.57 KBAnonymous (not verified)
#12 interdiff-7-12.txt1.01 KBAnonymous (not verified)
#12 2821112-12.patch3.72 KBAnonymous (not verified)
#8 interdiff-3-7.txt1.41 KBAnonymous (not verified)
#8 2821112-7.patch3.72 KBAnonymous (not verified)
#3 2821112-3.patch3.51 KBAnonymous (not verified)
#2 core-views-regex-filter-2821112-2.patch983 bytesesolitos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

esolitos created an issue. See original summary.

esolitos’s picture

Status: Active » Needs review
FileSize
983 bytes

Patch!

Anonymous’s picture

When i'm created tests for #2800727: Views NumericFilter regular_expression operator wrong method 'op_regex', I also have two different syntax:

'value' => '2[8]' # testFilterNumericRegularExpression

'value' => array('value' => '2[7-8]') #testFilterNumericExposedGroupedRegularExpression

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.

jazzfiction’s picture

I 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).

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

NumericFilter 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!

alexpott credited SylvainM.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

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

  1. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterNumericTest.php
    @@ -212,6 +212,60 @@ public function testFilterNumericExposedGroupedNotBetween() {
     
    +  public function testFilterNumericRegularExpression() {
    ...
    +
    +  public function testFilterNumericExposedGroupedRegularExpression() {
    

    Missing function comments describing what is being tested

  2. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterNumericTest.php
    @@ -212,6 +212,60 @@ public function testFilterNumericExposedGroupedNotBetween() {
    +  ¶
    

    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.

Anonymous’s picture

FileSize
3.72 KB
1.41 KB

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

Anonymous’s picture

Status: Needs work » Needs review
gambry’s picture

Priority: Normal » Major
Issue tags: +blocker, +Needs change record

This 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:

  • Create a view page and add any filter implementing NumericFilter (content 'ID' will work)
  • Set the filter operator to "Regular expression"
  • Save the view (the preview won't already work)
  • Visit the view page

Finally as we are changing the operator method from op_regex to opRegex and potentially breaking any code relying on old method definition, do we need a change record?

gambry’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterNumericTest.php
    @@ -212,6 +212,9 @@ public function testFilterNumericExposedGroupedNotBetween() {
    +   * Tests the Numeric field filter with the 'regular_expression' operator.
    

    It's not a 'field' filter, I would rather use the class description: "numeric filter handler" for consistency.

  2. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterNumericTest.php
    @@ -240,6 +243,10 @@ public function testFilterNumericRegularExpression() {
    +   * Tests the Numeric field filter with the 'regular_expression' operator
    

    Same as above.

Really good work!

Anonymous’s picture

Thanks for review and proposed improvements!

Status: Needs review » Needs work

The last submitted patch, 12: 2821112-12.patch, failed testing.

jonathanshaw’s picture

Status: Needs work » Reviewed & tested by the community

Test fail looks random

gambry’s picture

Status: Reviewed & tested by the community » Needs work

Even 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?

gambry’s picture

Assigned: Unassigned » gambry
gambry’s picture

Assigned: gambry » Unassigned
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Test is green and change record has been created: NumericFilter views handler 'regular_expression' operator method renamed. Back to RTBC.

Anonymous’s picture

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

gambry’s picture

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

Anonymous’s picture

@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)

Lendude’s picture

Status: Reviewed & tested by the community » Needs work

Really nice work. Just some minor nitpicks with this, and then I agree with the RTBC:

  1. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterNumericTest.php
    @@ -212,6 +212,67 @@ public function testFilterNumericExposedGroupedNotBetween() {
    +    // Change the filtering
    

    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.

  2. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterNumericTest.php
    @@ -212,6 +212,67 @@ public function testFilterNumericExposedGroupedNotBetween() {
    +    $this->container->get('router.builder')->rebuild();
    

    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.

Lendude’s picture

Yup, new test fails on Postgres, so that needs work too.

Anonymous’s picture

@Lendude, great point, thanks!

-    // Change the filtering
+    // Filtering by regular expression syntax

I'm not sure in my new comment, so do not mind if someone can help with this.

gambry’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -211,7 +211,7 @@ public function prepareQuery($query) {
+    return parent::prepareQuery(preg_replace('/ ([^ ]+) +(I*LIKE|NOT +I*LIKE|~\*|~) /i', ' ${1}::text ${2} ', $query));

This 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:

    //   [...]
    //   types involved in the query are unknown, so there is no way to
    //   conditionally execute this for affected queries only.
    $query = preg_replace('/ ([^ ]+) +(I*LIKE|NOT +I*LIKE) /i', ' ${1}::text ${2} ', $query));

    // Regular expression operator '~' works only for text, so make sure we cast the type.
    $query = preg_replace('/ ([^ ]+) +(~\*|~) /i', ' ${1}::text ${2} ', $query));
    return parent::prepareQuery($query);

UPDATE:

-    // Change the filtering
+    // Filtering by regular expression syntax

Coding standards for in-code comments require a final full stop at the end of period.
// Filtering by regular expression pattern.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
1.97 KB
gambry’s picture

Component: views.module » postgresql db driver
Issue tags: +Needs subsystem maintainer review
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -211,7 +211,16 @@ public function prepareQuery($query) {
+    // SIMILAR TO works only for text, so make sure we cast the type.
+    $query = preg_replace('/ ([^ ]+) +(SIMILAR TO|NOT SIMILAR TO) /i', ' ${1}::text ${2} ', $query);
+
+    // Regular expression match operators '~', '~*', '!~', '!~*' works only for
+    // text, so make sure we cast the type.
+    $query = preg_replace('/ ([^ ]+) +(~|~\*|!~|!~\*) /i', ' ${1}::text ${2} ', $query);

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

Anonymous’s picture

Let's solve the problem with PostgreSQL separately #2845543: PostgreSQL regular expression match operators works only for text, and this issue be postponed.

Lendude’s picture

Let's solve the problem with PostgreSQL separately and this issue be postponed

@vaplas++ (unless we can think of a way to somehow work around this in Views, but nothing really comes to mind).

gambry’s picture

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

gambry’s picture

In addition to:

Then the patch for this issue is now reverted to #12 + changes request from #21.

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

Here 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 :(

Then the patch for this issue is now reverted to #12 + changes request from #21.

Done in this patch. Temp change status on NR for testbot trigger.

Anonymous’s picture

Title: Views NumericFilter::opRegex() adds an array in the where instead of the value. » Views NumericFilter 'regular_expression' operator method op_regex() should be opRegex()
Status: Needs review » Postponed
gambry’s picture

Title: Views NumericFilter 'regular_expression' operator method op_regex() should be opRegex() » Views NumericFilter 'regular_expression' operator is broken
Issue summary: View changes

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

gambry’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
nicholas.alipaz’s picture

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

jibran’s picture

gambry’s picture

Status: Reviewed & tested by the community » Postponed

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

jibran’s picture

Title: Views NumericFilter 'regular_expression' operator is broken » [PP-1] Views NumericFilter 'regular_expression' operator is broken

Sorry, I missed that one. Thanks for the reply

gambry’s picture

Status: Postponed » Needs review

Blocker has been committed. Let's move to Needs Review to trigger test bot.

gambry’s picture

All green, back to RTBC.

gambry’s picture

Status: Needs review » Reviewed & tested by the community
mparker17’s picture

Title: [PP-1] Views NumericFilter 'regular_expression' operator is broken » Views NumericFilter 'regular_expression' operator is broken

Removing the [PP-1] in the title since the blocker has been committed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 3da27e5 to 8.4.x and 32eea0e to 8.3.x. Thanks!

  • alexpott committed 3da27e5 on 8.4.x
    Issue #2821112 by vaplas, jibran, esolitos, nicholas.alipaz, gambry,...

  • alexpott committed 32eea0e on 8.3.x
    Issue #2821112 by vaplas, jibran, esolitos, nicholas.alipaz, gambry,...

Status: Fixed » Closed (fixed)

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