Unexpected output: Affected paths: relative - resubmit patch.
⚙ D8 INP-1375 - Ensure "keywords" URL parameter is present on all search related pages
Page MenuHomeIn-Portal Phabricator

INP-1375 - Ensure "keywords" URL parameter is present on all search related pages
Needs ReviewPublic

Authored by erik on Sep 25 2014, 10:05 AM.

Details

Reviewers
alex
Test Plan
NOTE: Test with both enabled and disabled mod-rewrite.
  1. create 11 products with names, containing Test & fun text
  2. enter Test & fun text into search box
  3. after search form is submitted verify, that entered keywords are displayed in
    • search input box
    • url (in the urlencoded form)
    • page title
  4. click on More... link under search results
  5. confirm, that previously entered keywords appear in same places (as above)
  6. click on the 2 to go to 2nd page in search results (pagination control is located under the list)
  7. confirm, that previously entered keywords appear in same places (as above)

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.2.x
Lint
Lint ErrorsExcuse: Full file reformatting is not a part of this task.
Unit
No Unit Test Coverage
Build Status
Buildable 86
Build 86: arc lint + arc unit

Event Timeline

erik updated this revision to Diff 13.Sep 25 2014, 10:05 AM
erik retitled this revision from to Keywords absent in the url of the search results page.
erik updated this object.
erik edited the test plan for this revision. (Show Details)
erik added a reviewer: alex.
erik set the repository for this revision to rINP In-Portal.
erik added 1 JIRA issue(s): INP-1375.
alex requested changes to this revision.Sep 25 2014, 10:32 AM
alex edited edge metadata.
alex added inline comments.
core/kernel/db/db_tag_processor.php
762

Please do the following:

  • there is dedicated kApplication::unescapeRequestVariable method that needs to be used in cases, where on Front-End we need to revert force htmlspecialchars call (don't use kUtil::unescape for that)
  • use kUtil::escape with ...ESCAPE_URL argument instead of direct call to urlencode
themes/advanced/platform/elements/content_boxes.elm.tpl
202

There is no point in using " because it's not used in any other HTML tag attribute value. So please use ".

207–208

Don't we have any tag like SearchLink that builds link to template and passes keywords to it?

This revision now requires changes to proceed.Sep 25 2014, 10:32 AM
alex added a comment.Nov 23 2014, 3:11 AM

Adding comment to create corresponding "Issue Link" from JIRA Issue back to this Differential Revision.

alex retitled this revision from Keywords absent in the url of the search results page to INP-1375 - Keywords absent in the url of the search results page.Dec 7 2014, 3:30 PM
alex edited edge metadata.
alex set the repository for this revision to rINP In-Portal.Dec 7 2014, 3:42 PM
erik added inline comments.Mar 10 2015, 5:05 AM
themes/advanced/platform/elements/content_boxes.elm.tpl
207–208

No, we don't have such tag.

alex added inline comments.Mar 10 2015, 5:10 AM
themes/advanced/platform/elements/content_boxes.elm.tpl
207–208

Then we should add one I guess. Also there is no such url_escape parameter.

erik edited the test plan for this revision. (Show Details)Mar 12 2015, 5:20 AM
alex edited the test plan for this revision. (Show Details)Mar 12 2015, 5:31 AM
erik updated this revision to Diff 120.Mar 12 2015, 5:55 AM
erik edited edge metadata.

Fixes.
Used more proper methods for escape/unescape request values.
Fiх to search logic, to allow keywords in URL without full search results recalculation.

erik updated this revision to Diff 121.Mar 12 2015, 6:00 AM
erik edited edge metadata.
erik removed rINP In-Portal as the repository for this revision.
alex added a comment.Mar 12 2015, 6:51 AM

The http://qa.in-portal.org/D8?id=120 diff was created using wrong command arc diff --update D8 instead of arc diff --update D8 --cl and that's why it doesn't contain any relevant changes.

alex set the repository for this revision to rINP In-Portal.Mar 12 2015, 6:56 AM
alex requested changes to this revision.May 19 2015, 8:21 PM
alex edited edge metadata.
alex added inline comments.
core/kernel/processors/main_processor.php
310–313

Return back.

core/kernel/processors/tag_processor.php
187 ↗(On Diff #121)

Undo changes to this method.

205 ↗(On Diff #121)

Undo changes to this method.

themes/advanced/platform/elements/content_boxes.elm.tpl
207–208

Not done.

This revision now requires changes to proceed.May 19 2015, 8:21 PM
erik updated this revision to Diff 204.May 29 2015, 12:57 PM
erik edited edge metadata.

Code style fixes.

alex requested changes to this revision.May 30 2015, 7:31 AM
alex edited edge metadata.

Test plan is incomplete. You're testing single search, but not testing consequential search using same/different keywords where searches will return 0 or something. For example for me 2nd search never returns any results.

Also see more detailed comments below.

core/units/helpers/search_helper.php
817–821
  1. The $request_keywords contains " when " was present in search keywords (e.g. test " link), but $keywords properly has " and they never match resulting no search performed. Please use $this->Application->unescapeRequestVariable method to avoid this.
  2. Please do a trim of keywords before comparing because the $keywords is always trimmed and when we have trailing spaces then we'll have a non-match all the time.
825–831

I've performed one search and then I'm getting it's result every time I do a next search. I think that conditions used are not quite accurate:

!$request_keywords

This will match 2 things:

  • absent keyword
  • present, but empty keyword

If later is expected behavior then use === false instead of !

$request_keywords == $session_keywords

This sounds like correct one (new keywords match to session = reuse previous search).

!$this->Application->GetVar('do_not_drop_search_table')

The do_not_drop_search_table request variable is set by 1st OnSimpleSearch event, that is executed and therefore whole condition evaluates to true in that case and no search is performed. See me debugging this below:

$this->Conn->Query($sql)

This sounds like correct one (search table exists = don't drop it considering that keywords match to previously ones used).

This revision now requires changes to proceed.May 30 2015, 7:31 AM
erik updated this revision to Diff 256.Aug 17 2015, 4:38 AM
erik edited edge metadata.

Made working consequential search, made other required improvements.

alex retitled this revision from INP-1375 - Keywords absent in the url of the search results page to INP-1375 - Ensure "keywords" URL parameter is present on all search related pages.Mar 27 2016, 8:06 AM
alex edited edge metadata.