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

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Frédéric Buclin)

Tracking

Bugzilla 4.2
Dependency tree / graph
Bug Flags:
approval +
approval4.2 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

19.77 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 434757 [details] [diff] [review]
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.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #434757 - Flags: review?(LpSolit)
(Reporter)

Comment 2

8 years ago
  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).
(Assignee)

Comment 3

8 years ago
Comment on attachment 434757 [details] [diff] [review]
v1

r=LpSolit
Attachment #434757 - Flags: review?(LpSolit) → review+
(Assignee)

Updated

8 years ago
Flags: approval+
(Reporter)

Comment 4

8 years ago
Thanks for the review, LpSolit! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7093.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Blocks: 556579
(Assignee)

Comment 5

8 years ago
Reopening as the patch has been backed out in bug 556579.
Status: RESOLVED → REOPENED
Flags: approval+
Resolution: FIXED → ---
Target Milestone: Bugzilla 4.0 → ---
(Assignee)

Comment 6

6 years ago
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.
Assignee: mkanat → LpSolit
Blocks: 730207
Status: REOPENED → ASSIGNED
Target Milestone: --- → Bugzilla 4.4
(Assignee)

Comment 7

6 years ago
Created attachment 609144 [details] [diff] [review]
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().
Attachment #434757 - Attachment is obsolete: true
Attachment #609144 - Flags: review?(wurblzap)
Attachment #609144 - Flags: review?(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
Attachment #609144 - Flags: review?(dkl) → review+
(Assignee)

Updated

6 years ago
Attachment #609144 - Flags: review?(wurblzap)
(Assignee)

Comment 9

6 years ago
(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.
Flags: approval4.2+
Flags: approval+
Target Milestone: Bugzilla 4.4 → Bugzilla 4.2
(Assignee)

Comment 10

6 years ago
Created attachment 610595 [details] [diff] [review]
patch, v2.1

quotewords() replaced by parse_line() + other comments addressed. Carrying forward dkl's r+.
Attachment #609144 - Attachment is obsolete: true
Attachment #610595 - Flags: review+
(Assignee)

Comment 11

6 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/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://lpsolit%40gmail.com@bzr.mozilla.org/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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago6 years ago
Resolution: --- → FIXED
Thank you fixing and reviewing this.
(Assignee)

Updated

5 years ago
Blocks: 856158
You need to log in before you can comment on or make changes to this bug.