Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods. #6246

lkcampbell · 2013-12-14T01:07:16Z

This is a fix for issue #6008. It also adds remove() and isEmpty() methods to the UrlParams module.

redmunds · 2013-12-16T22:40:10Z

src/utils/UrlParams.js

+            });
+        } else {
+            self._store = {};
+        }


Seems like it's possible for this code could provide unexpected results for self._store. For example, if url ends with a ?. Also, if all search parameters are integers, then self._store would be an array.

I think it would be safest to initialize it to an empty object in all cases:

self._store = {}; if (searchString) { urlParams = searchString.split("&"); urlParams.forEach(function (param) { p = param.split("="); self._store[decodeURIComponent(p[0])] = decodeURIComponent(p[1]); }); }

@redmunds, I believe this is already addressed in the URLParams constructor, which initializes self._store to an empty object.

If the constructor made the call to parse() and parse() was private method, then I would agree. But parse() is public and can be called multiple times.

Oops, I missed comment until after my last commit. Let me look into it.

redmunds · 2013-12-16T23:06:08Z

Done with review. This looks good. Just a few comments. Thanks for adding unit tests!

redmunds · 2013-12-20T16:34:10Z

@lkcampbell What's the status of this one? I was waiting for you to add a few more unit tests.

lkcampbell · 2013-12-21T02:12:11Z

@redmunds, I'm going to work on the update this weekend.

lkcampbell · 2013-12-21T17:21:12Z

@redmunds, changes for code review committed.

It occurred to me as I was updating the edge cases that this code doesn't have any exception handling for seriously malformed URLs. Do you want me to address those cases as well or is this sufficient for now?

redmunds · 2013-12-21T17:31:54Z

this code doesn't have any exception handling for seriously malformed URLs

Seems like this code should only handle "url params" (hence the name), so I think it's outside of the scope of this module, and don't need to worry about it.

lkcampbell · 2013-12-21T17:36:56Z

@redmunds, added _store initialization into parse method. Code review changes committed.

redmunds · 2013-12-23T16:58:39Z

Looks good. Merging.

Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods.

lkcampbell added 2 commits November 15, 2013 09:03

Fix parsing URLs with no parameters. Add isEmpty and remove methods.

7c9e7aa

Add UrlParams unit tests

6cd9418

ghost assigned redmunds Dec 14, 2013

redmunds reviewed Dec 16, 2013
View reviewed changes

redmunds mentioned this pull request Dec 19, 2013

Reload Without User Extensions command #5620

Closed

Fix parsing edge cases and added edge case tests

353ffe9

Fix _store initialization in public method parse()

44d8b9d

lkcampbell mentioned this pull request Dec 22, 2013

"Restart in Safe Mode" #5078

Closed

redmunds added a commit that referenced this pull request Dec 23, 2013

Merge pull request #6246 from lkcampbell/fix-6008

b8d6411

Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods.

redmunds merged commit b8d6411 into adobe:master Dec 23, 2013

lkcampbell deleted the fix-6008 branch December 23, 2013 23:19

This was referenced Aug 30, 2021

[CLOSED] Reload Without User Extensions command brackets-archive/bracketsIssues#5153

Open

[CLOSED] Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods. brackets-archive/bracketsIssues#5690

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods. #6246

Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods. #6246

Uh oh!

lkcampbell commented Dec 14, 2013

Uh oh!

redmunds Dec 16, 2013

Uh oh!

lkcampbell Dec 21, 2013

Uh oh!

redmunds Dec 21, 2013

Uh oh!

lkcampbell Dec 21, 2013

Uh oh!

redmunds commented Dec 16, 2013

Uh oh!

redmunds commented Dec 20, 2013

Uh oh!

lkcampbell commented Dec 21, 2013

Uh oh!

lkcampbell commented Dec 21, 2013

Uh oh!

redmunds commented Dec 21, 2013

Uh oh!

lkcampbell commented Dec 21, 2013

Uh oh!

redmunds commented Dec 23, 2013

Uh oh!

Uh oh!

Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods. #6246

Fix UrlParams parsing when URL has no parameters. Add remove() and isEmpty() methods. #6246

Uh oh!

Conversation

lkcampbell commented Dec 14, 2013

Uh oh!

redmunds Dec 16, 2013

Choose a reason for hiding this comment

Uh oh!

lkcampbell Dec 21, 2013

Choose a reason for hiding this comment

Uh oh!

redmunds Dec 21, 2013

Choose a reason for hiding this comment

Uh oh!

lkcampbell Dec 21, 2013

Choose a reason for hiding this comment

Uh oh!

redmunds commented Dec 16, 2013

Uh oh!

redmunds commented Dec 20, 2013

Uh oh!

lkcampbell commented Dec 21, 2013

Uh oh!

lkcampbell commented Dec 21, 2013

Uh oh!

redmunds commented Dec 21, 2013

Uh oh!

lkcampbell commented Dec 21, 2013

Uh oh!

redmunds commented Dec 23, 2013

Uh oh!

Uh oh!