Opened 6 years ago
Closed 6 years ago
#7110 closed defect (fixed)
Do real parsing of boolean query string parameters
Reported by: | dcf | Owned by: | dcf |
---|---|---|---|
Priority: | Low | Milestone: | |
Component: | Archived/Flashproxy | Version: | |
Severity: | Keywords: | ||
Cc: | Actual Points: | ||
Parent ID: | Points: | ||
Reviewer: | Sponsor: |
Description
The debug
query string parameter accepts anything, even values like "0"
and false, as a way to turn on debug mode.
http://crypto.stanford.edu/flashproxy/embed.html?debug=0
Add a get_query_string_param_boolean
function and use it to read the debug
value. The function should return true when it receives any of these three strings (and only these strings; no case-folding or anything else):
"true"
"1"
""
The function should return false iff it receives either of these strings:
"false"
"0"
The function should return null
in all other cases.
There need to be a couple of simple test cases added to flashproxy-test.js
.
Child Tickets
Attachments (4)
Change History (7)
Changed 6 years ago by
Attachment: | flashproxy.js added |
---|
comment:1 Changed 6 years ago by
Thank you for making these changes. It's easier if you attach patches instead; use
git format-patch HEAD^
or something similar.
get_query_string_param_boolean
should work the same way and have the same signature as the other get_query_string_param_*
functions. That is, it shouldn't just take a single value; it should take a query string, a parameter name, and a default value.
In other words, the test for the debug parameter should look something like this:
var query = parse_query_string(window.location.search.substr(1)); var DEBUG = get_query_string_param_boolean(query, "debug", false); if (DEBUG) { .... }
The switch
you have written would be shorter (6 lines) and clearer as an if
-else
.
test_get_query_string_param_boolean
looks good, but the test cases should use query strings to match the expected behavior of get_query_string_param_boolean
as described above.
There are some places in the patch that add blank lines; avoid making these kinds of unrelated changes. Try git checkout -p
to selectively discard parts of your changes.
Changed 6 years ago by
Attachment: | 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch added |
---|
Resubmitting patch with issues fixed.
comment:2 Changed 6 years ago by
Thanks, this looks better. A few more changes before I accept it:
Use four-space indents, not hard tabs.
Get rid of trailing whitespace. git apply
complains:
$ git apply 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:28: space before tab in indent. expected: true}, 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:36: trailing whitespace. expected: null}, 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:39: trailing whitespace. ]; 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:92: trailing whitespace. val = query[param] 0001-Patch-for-ticket-7110-adding-get_query_param_boolean.patch:93: trailing whitespace. if (val === undefined) { warning: squelched 1 whitespace error warning: 6 lines add whitespace errors.
I think get_query_param_boolean
can be written more clearly:
if (val === undefined) return default_val; else if (val === "true" || val === "1" || val === "") return true; else if (val === "false" || val === "0") return false; else return null;
The comment above get_query_param_boolean
should state exactly which values are considered true, which are considered false, and that all other values return null
.
The documentation for the debug
parameter at the top of the file should say what values are true, because "(to any value)" is not the case anymore.
Changed 6 years ago by
Attachment: | ticket#7110.patch added |
---|
New patch fixed according to comments, git apply friendly.
comment:3 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Added function get_query_string_param_boolean()