Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Searching in datatable for ?param results in error message #5949

Closed
mattab opened this issue Aug 7, 2014 · 8 comments
Closed

Searching in datatable for ?param results in error message #5949

mattab opened this issue Aug 7, 2014 · 8 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Aug 7, 2014

Reproduce:

  • go to Actions report,
  • search for ?param and the search issues a warning message:
Warning: preg_match() [function.preg-match]: Compilation failed: nothing to repeat at offset 0 in /home/piwik-demo/www/demo.piwik.org/core/DataTable/Filter/Pattern.php on line 72

Backtrace -->

#0 Piwik\Error::errorHandler(...) called at [:]
#1 preg_match(...) called at [/home/piwik-demo/www/demo.piwik.org/core/DataTable/Filter/Pattern.php:72]
#2 Piwik\DataTable\Filter\Pattern::match(...) called at [/home/piwik-demo/www/demo.piwik.org/core/DataTable/Filter/PatternRecursive.php:80]
#3 Piwik\DataTable\Filter\PatternRecursive->filter(...) called at [/home/piwik-demo/www/demo.piwik.org/core/DataTable.php:425]

and then

Warning: preg_match() [function.preg-match]: Compilation failed: nothing to repeat at offset 0 in /home/piwik-demo/www/demo.piwik.org/core/DataTable/Filter/Pattern.php on line 72
@mattab mattab added this to the Piwik 2.5.0 milestone Aug 7, 2014
@mattab mattab added the Bug label Aug 7, 2014
@mattab
Copy link
Member Author

mattab commented Aug 10, 2014

this was also reported in forum

tsteur added a commit that referenced this issue Aug 11, 2014
@tsteur
Copy link
Member

tsteur commented Aug 11, 2014

At first I wanted to fix this issue in the PHP pattern filter by escaping those quantifiers as I did in JavaScript but then no quantifiers would ever work again there as the filter might be also used in another context. Therefore did it in JavaScript. Also escaped + and * which caused the same warning.

The side effect is that you could no longer write a regex in the search box like (nn)+ since the + would be escaped to (nn)\+. I am not sure if this was actually a feature and meant to be like this? If so, we could probably only escape those characters if it is the first character. Since less users are aware of regex I would assume people would rather want + to be interpreted as \+ as it is the case now

@tsteur tsteur self-assigned this Aug 11, 2014
@mattab
Copy link
Member Author

mattab commented Aug 11, 2014

I am not sure if this was actually a feature and meant to be like this?

it is a feature to let users write regular expressions in the search bar (and in API request via filter_pattern), eg. you can search for (en|de)\.wikipedia (and any other regex including * and +)

do you know if it's possible to detect whether a given search string is a regular expression or not?

(if we could detect this, then we could enable regular expression only when input string is indeed regular expression...)

I wonder how Google Analytics handles this case. After doing some quick test it seems their datatable search input box works with both regular expressions and simple search. For example if I search for + then it returns all URLs containing + character. But if I input regular expression containing + it still work as regular expression.

Maybe the best solution is to:

  • run the search as "plain text search"
  • run the search as regular expression (if the search is a valid regex string)

and then we could return the "union" of both searches?

@tsteur
Copy link
Member

tsteur commented Aug 12, 2014

As mentioned above I would only escape the first character if it is a quantifier in this case. I think it is very tricky to detect it otherwise. Checking for brackets on the left or right or so wouldn't work since even jpe?g is valid.

@tsteur
Copy link
Member

tsteur commented Aug 12, 2014

I don't think executing a pattern twice is the best idea since a user might get results that he doesn't want. Autodetecting/magic can be very annoying in case the auto detect does not work correctly in a case and you will never know which result you get.

Documentation says nothing about regexp:

filter_pattern; defines the text you want to search for in the filter_column. Only the row with the given column matching the pattern will be returned.

Maybe we should mention it there and then it is ok. For API users which are usually advanced users this behavior is ok. Best case would be maybe to have two different attributes (filter_pattern and filter_text for instance). This would be clear.

For regular users that use the UI I would personally prefer not to apply a regex by default and treat it as text. IDEs and text editors solve this problem by having a checkbox "Regex". But this makes the UI more complicated again. A solution could be to force advanced users to write regex within a leading and ending slash /$myRegex/ and show this as a tooltip on hover (title)

tsteur added a commit to matomo-org/developer-documentation that referenced this issue Aug 15, 2014
@tsteur
Copy link
Member

tsteur commented Aug 15, 2014

I'll close it for now as it fixes the reported issue. Maybe we can otherwise open a new ticket if needed?

@tsteur tsteur closed this as completed Aug 15, 2014
@mattab
Copy link
Member Author

mattab commented Aug 15, 2014

maybe we could mention regex in the API documentation for filter_pattern?
otherwise looks good to me

@tsteur
Copy link
Member

tsteur commented Aug 15, 2014

I did it this morning see previous commit ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

2 participants