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"
}
}| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 2175513-view-basejs.patch | 1.25 KB | droplet |
| #9 | 2175513-view-basejs.patch | 1.24 KB | droplet |
Comments
Comment #1
droplet commentedComment #2
dawehnerthis does not look like proper drupal codestyle.
We should also explain WHY we ignore it.
Comment #3
internetdevels commentedFixed codestyle and added description.
Comment #4
nod_I don't think the
qparamter exists anymore, even for non-clean urls. We should be able to get rid of it in the code.- the
pairvariable 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:
Looks like the else could be dropped?
Comment #5
dawehnerThank you!
Let's assign nod_ for a proper JS review.
Comment #6
droplet commented@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 ? :)
Comment #7
nod_- oh right! my mistake sorry.
- yep, only for the cool-factor.
Comment #8
nod_Comment #9
droplet commented- 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
Comment #10
nod_jshint complains. There was the same issue in the
add_cssajax command. Went around it by using a do/while.Comment #11
nod_Comment #12
droplet commentedComment #13
droplet commentedComment #14
nod_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
Comment #15
alexpottLet'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.
Comment #16
droplet commentedtagging for good writers.
Comment #17
droplet commentedJust 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...
Comment #30
larowlanComment #31
larowlanIs this still relevant?
Can we just use UrlSearchParams now?
Comment #32
nod_We can use it to simplify the code but URLSearchParam wouldn't give an array topics, at best it gives an array for
topics[]using the getAll method. Close enough though.Given the age of the issue, the lack of activity and the fact that URLSearchParam is a thing. It does't seem like the fix is necessary and modern alternative makes it easier to work with url search params than using this method.