554819 - Quicksearch should be using Text::ParseWords instead of custom code in splitString

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

Right now, quicksearch has its own custom code to parse the whole string, and that's not necessary, because Perl ships with a module called Text::ParseWords that will do exactly this.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

One of the advantages of using this is that it allows you to search for literal quotes, like "\"", or do to something like this\ string and get "this string" as a single word.

Comment 2

[Max Kanat-Alexander]

  Alternately, at some point we may decide that doing \" and \(space) isn't that important, and turn on "keep" (the second argument), which will let us know which strings were quoted (and thus help us fix bug 553884, possibly).

Comment 3

[Frédéric Buclin]

Comment on attachment 434757 [details] [diff] [review]
v1

r=LpSolit

Comment 4

[Max Kanat-Alexander]

Thanks for the review, LpSolit! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7093.

Comment 5

[Frédéric Buclin]

Reopening as the patch has been backed out in bug 556579.

Comment 6

[Frédéric Buclin]

I have a working patch, which also fixes bug 730207 and doesn't regress bug 553884 nor bug 556579. It also fixes several incorrect behaviors.

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

Here is a fully functional patch which fixes several things:
- non-ASCII characters are correctly taken into account, even when not quoted (this fixes bug 730207).
- @lpsolit OR reporter:lpsolit didn't work, because it was translated to @lpsolit|reporter:lpsolit which meant that the assignee was either "lpsolit" or "reporter:lpsolit", which is not the intended behavior. This is now fixed. Now only commas are accepted to enumerate values.
- the broken url_decode() is no longer needed and is now gone.
- I added many examples to the documentation to help users better understand how things work, and how to use the syntax correctly. I also explicitly mentioned that strings containing special characters such as quotes, colons, or commas must be quoted, with an example. I also explain the difference between field:value1,value2 and field:value1|value2 (related to the example above). This makes our syntax much less ambiguous.
- this patch doesn't regress bug 553884 nor bug 556579 and still let us kill splitString().

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 609144 [details] [diff] [review]
patch, v2

Review of attachment 609144 [details] [diff] [review]:
-----------------------------------------------------------------

Why not use parse_line() instead of quotewords() as it does the same thing and saves an extra function call (quotewords calls parse_line internally).
Otherwise works fine with the other changes on commit. r=dkl

dkl

::: template/en/default/pages/quicksearch.html.tmpl
@@ +28,5 @@
> +<ul>
> +  <li><a href="#basics">The Basics</a></li>
> +  <li><a href="#basic_examples">Examples of Simple Queries</a></li>
> +  <li><a href="#fields">Fields You Can Search On</a></li>
> +  <li><a href="#advanced">Advanced Features</a></li>

rename #features to be less ambiguous.

@@ +169,3 @@
>      characters that would otherwise be interpreted specially by quicksearch.
> +    For example, <kbd>"this|thing"</kbd> would search for the literal string
> +    <em>this|thing</em> and would not be parsed as <kbd>"this OR that"</kbd>.

Change this|thing to this|that.

@@ +300,5 @@
> +<h2 id="advanced_examples">Examples of Complex Queries</h2>
> +
> +<p>It is pretty easy to write rather complex queries without too much effort.
> +  For very complex queries, you have to use the
> +  <a href="query.cgi?format=advanced">Advanced Search form</a>.</p>

put "form" outside the link

Comment 9

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #8)
> Why not use parse_line() instead of quotewords() as it does the same thing
> and saves an extra function call (quotewords calls parse_line internally).

Sure, I can do that. I will attach an updated patch, also addressing other comments you made. Thanks for the review!

As discussed with dkl on IRC, we will take this patch for 4.2.1 as it also fixes bug 730207.

Comment 10

[Frédéric Buclin]

Attachment: patch, v2.1

quotewords() replaced by parse_line() + other comments addressed. Carrying forward dkl's r+.

Comment 11

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Util.pm
modified Bugzilla/Search/Quicksearch.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/pages/quicksearch.html.tmpl
Committed revision 8169.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Util.pm
modified Bugzilla/Search/Quicksearch.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/pages/quicksearch.html.tmpl
Committed revision 8060.

Comment 12

[Marc Schumann [:Wurblzap]]

Thank you fixing and reviewing this.