There're 2 problems of the original function:
1. Do not handle params array
2. Do not remove #hash

Original:

Object {a: "test1", b: "test2", topics[]: "cba#hash"}

Patched:

Object {a: "test1", b: "test2", topics[]: Array[2]}
a: "test1"
b: "test2"
topics[]: Array[2]
0: "abc"
1: "cba"

Patched code will be same as PHP:

$url = 'www.example.com/path?&a=test1&b=test2&topics[]=abc&topics[]=cba#hash';
$parts = parse_url($url);
parse_str($parts['query'], $query);
var_dump($query);

array(3) {
  ["a"]=>
  string(5) "test1"
  ["b"]=>
  string(5) "test2"
  ["topics"]=>
  array(2) {
    [0]=>
    string(3) "abc"
    [1]=>
    string(3) "cba"
  }
}
Files: 
CommentFileSizeAuthor
#12 2175513-view-basejs.patch1.25 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 62,982 pass(es). View
#9 2175513-view-basejs.patch1.24 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 62,974 pass(es). View

Comments

droplet’s picture

Issue summary: View changes
dawehner’s picture

  1. +++ b/core/modules/views/js/base.js
    @@ -12,22 +12,30 @@
    +    while ( pair = re.exec(query) ) {
    ...
    +        } else {
    

    this does not look like proper drupal codestyle.

  2. +++ b/core/modules/views/js/base.js
    @@ -12,22 +12,30 @@
    +      // Ignore the 'q' path argument, if present.
    +      if (pair[1] !== 'q' && pair[2]) {
    +        var k = decodeURIComponent( pair[1].replace(/\+/g, " "));
    +        var v = decodeURIComponent( pair[2].replace(/\+/g, " "));
    

    We should also explain WHY we ignore it.

InternetDevels’s picture

FileSize
1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 59,754 pass(es). View
1002 bytes

Fixed codestyle and added description.

nod_’s picture

Status: Needs review » Needs work

I don't think the q paramter exists anymore, even for non-clean urls. We should be able to get rid of it in the code.

- the pair variable needs to be on it's own var line as well.
- we check for undefined with typeof XXX === 'undefined'
- if we're already using Array.isArray(), might as well use [].forEach() for the chars. ES5 FTW.

And at some point we end up doing:

else {
  params[undefined] = v;
}

Looks like the else could be dropped?

dawehner’s picture

Assigned: Unassigned » nod_
Status: Needs work » Needs review

Thank you!

Let's assign nod_ for a proper JS review.

droplet’s picture

@nod_,

- the IF condition is checking key existence (params[k], not the k). I can't think any case that will end up with "params[undefined]" because the regex will not allow it happen.

- any benefits to use [].forEach in this case?? or just make it looks cool only ? :)

nod_’s picture

- oh right! my mistake sorry.

- yep, only for the cool-factor.

nod_’s picture

Assigned: nod_ » Unassigned
Status: Needs review » Needs work
droplet’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
PASSED: [[SimpleTest]]: [MySQL] 62,974 pass(es). View

- Remove the 'q' path argument in D8 patch
- Fix code standard
- Leave for-while-loop as it. Don't see any reason to refactor it. :)
- Dropped last change #2099599: Minor for-loop optimizations for base.js

nod_’s picture

core/modules/views/js/base.js: line 23, col 33, Expected a conditional expression and instead saw an assignment.

jshint complains. There was the same issue in the add_css ajax command. Went around it by using a do/while.

nod_’s picture

Status: Needs review » Needs work
droplet’s picture

FileSize
1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 62,982 pass(es). View
droplet’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Reviewed & tested by the community

Lol that's a creative way to go around the jshint issue.

If you don't want to use do/while that's fine with me, it's cute though :p

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Let's add an example to the function docblock to show how this works. I think there could be more code commentary to explain what is going on.

droplet’s picture

Issue tags: +Needs Documentation

tagging for good writers.

droplet’s picture

Just checkout Node.js 5.5 stable version and noticed one issue about parse querystring performance. That reminded me about this issue.

something we can learn from nodejs:

code:
https://github.com/nodejs/node/blob/4bc1a4776164f813db8d22813fd06c7f5bdc...
test:
https://github.com/nodejs/node/blob/d1aabd626428cec65e5f54c04d9e3446d1e4...

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.