Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

url: make url.format() encode all occurrences of # in search #8072

Closed
wants to merge 1 commit into from
Closed

url: make url.format() encode all occurrences of # in search #8072

wants to merge 1 commit into from

Conversation

imyller
Copy link
Member

@imyller imyller commented Aug 11, 2016 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

Description of change

This fixes an error where the first occurrence of # in search parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064

@nodejs-github-bot nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Aug 11, 2016
@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/3630/

@addaleax
Copy link
Member

I think the changes here look good, but you may want to shorten the subject line of the commit message to no more than 50 characters and use a full URL for the referenced github issue.

@jasnell
Copy link
Member

jasnell commented Aug 11, 2016

LGTM with the nit @addaleax mentions fixed. We'll definitely need a CITGM run on this (@thealphanerd)

@imyller
url: url.format() encodes all # in search
e801b24 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: nodejs#8064

Signed-off-by: Ilkka Myller <[email protected]>
@imyller
Copy link
Member Author

imyller commented Aug 11, 2016

@addaleax Thanks for pointing those out. Fixed.

@imyller
Copy link
Member Author

imyller commented Aug 11, 2016

CI check failure with RasPi seems to be unrelated to this PR:

https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=addons,label=pi2-raspbian-wheezy/3263/console

https://ci.nodejs.org/job/node-test-binary-arm/RUN_SUBSET=addons,label=pi1-raspbian-wheezy/3263/console

+ make test-ci-native
Makefile:77: *** Stale config.gypi, please re-run ./configure.  Stop.
Build step 'Execute shell' marked build as failure

@mscdex
Copy link
Contributor

mscdex commented Aug 12, 2016

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker-xunit/46/

@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

CITGM looks good. Only known expected failures. Still LGTM

@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

One more CI before landing: https://ci.nodejs.org/job/node-test-pull-request/3730/

jasnell pushed a commit that referenced this pull request Aug 18, 2016
@imyller @jasnell
url: url.format() encodes all # in search
1d0385f 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064
PR-URL: #8072
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

Landed in 1d0385f

@jasnell jasnell closed this Aug 18, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
Closed
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
@imyller @Fishrock123
url: url.format() encodes all # in search
13da7f1 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: nodejs#8064
PR-URL: nodejs#8072
Reviewed-By: James M Snell <[email protected]>

 Conflicts:
	test/parallel/test-url.js
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
@imyller @Fishrock123
url: url.format() encodes all # in search
eec5d02 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064
PR-URL: #8072
Reviewed-By: James M Snell <[email protected]>

 Conflicts:
	test/parallel/test-url.js
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
@imyller
url: url.format() encodes all # in search
2069ba9 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064
PR-URL: #8072
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
@imyller
url: url.format() encodes all # in search
a8ef7a0 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064
PR-URL: #8072
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
@imyller
url: url.format() encodes all # in search
ce2ca5e 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064
PR-URL: #8072
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@imyller @rvagg
url: url.format() encodes all # in search
0cfd01a 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064
PR-URL: #8072
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@imyller @MylesBorins
url: url.format() encodes all # in search
96cfa92 
This commit fixes an error where only the first occurrence of `#` in
`search` parameter is URL encoded, and subsequent occurrences are not.

Also added a test for the case.

Fixes: #8064
PR-URL: #8072
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Closed
MylesBorins added a commit that referenced this pull request Oct 26, 2016
@MylesBorins
2016-10-26, Version 4.6.2 'Argon' (LTS)
0bcb2f2 
This LTS release comes with 219 commits. This includes 80 commits that
are docs related, 58 commits that are test related, 20 commits that
are build / tool related, and 9 commits that are updates to
dependencies.

Notable Changes

* build:
  - It is now possible to build the documentation from the release
    tarball (Anna Henningsen)
    #8413
* buffer:
  - Buffer will no longer incorrectly return a zero filled buffer when
    an encoding is passed (Teddy Katz)
    #9238
* deps:
  - upgrade npm in LTS to 2.15.11 (Kat Marchán)
    #8928
* repl:
  - Enable tab completion for global properties (Lance Ball)
    #7369
* url:
  - `url.format()` will now encode all `#` in `search` (Ilkka Myller)
    #8072

PR-URL: #9298
MylesBorins added a commit that referenced this pull request Oct 26, 2016
@MylesBorins
2016-10-26, Version 4.6.2 'Argon' (LTS)
7c009cc 
This LTS release comes with 219 commits. This includes 80 commits that
are docs related, 58 commits that are test related, 20 commits that
are build / tool related, and 9 commits that are updates to
dependencies.

Notable Changes

* build:
  - It is now possible to build the documentation from the release
    tarball (Anna Henningsen)
    #8413
* buffer:
  - Buffer.alloc() will no longer incorrectly return a zero filled
    buffer when an encoding is passed (Teddy Katz)
    #9238
* deps:
  - upgrade npm in LTS to 2.15.11 (Kat Marchán)
    #8928
* repl:
  - Enable tab completion for global properties (Lance Ball)
    #7369
* url:
  - `url.format()` will now encode all `#` in `search` (Ilkka Myller)
    #8072

PR-URL: #9298
MylesBorins added a commit that referenced this pull request Nov 7, 2016
@MylesBorins
2016-11-08, Version 4.6.2 'Argon' (LTS)
4db97b8 
This LTS release comes with 219 commits. This includes 80 commits that
are docs related, 58 commits that are test related, 20 commits that
are build / tool related, and 9 commits that are updates to
dependencies.

Notable Changes

* build:
  - It is now possible to build the documentation from the release
    tarball (Anna Henningsen)
    #8413
* buffer:
  - Buffer.alloc() will no longer incorrectly return a zero filled
    buffer when an encoding is passed (Teddy Katz)
    #9238
* deps:
  - upgrade npm in LTS to 2.15.11 (Kat Marchán)
    #8928
* repl:
  - Enable tab completion for global properties (Lance Ball)
    #7369
* url:
  - `url.format()` will now encode all `#` in `search` (Ilkka Myller)
    #8072

PR-URL: #9298
MylesBorins added a commit that referenced this pull request Nov 8, 2016
@MylesBorins
2016-11-08, Version 4.6.2 'Argon' (LTS)
3562658 
This LTS release comes with 219 commits. This includes 80 commits that
are docs related, 58 commits that are test related, 20 commits that
are build / tool related, and 9 commits that are updates to
dependencies.

Notable Changes

* build:
  - It is now possible to build the documentation from the release
    tarball (Anna Henningsen)
    #8413
* buffer:
  - Buffer.alloc() will no longer incorrectly return a zero filled
    buffer when an encoding is passed (Teddy Katz)
    #9238
* deps:
  - upgrade npm in LTS to 2.15.11 (Kat Marchán)
    #8928
* repl:
  - Enable tab completion for global properties (Lance Ball)
    #7369
* url:
  - `url.format()` will now encode all `#` in `search` (Ilkka Myller)
    #8072

PR-URL: #9298
imyller added a commit to imyller/meta-nodejs that referenced this pull request Nov 11, 2016
@imyller
2016-11-08, Version 4.6.2 'Argon' (LTS)
45616ae 
    This LTS release comes with 219 commits. This includes 80 commits that
    are docs related, 58 commits that are test related, 20 commits that
    are build / tool related, and 9 commits that are updates to
    dependencies.

    Notable Changes

    * build:
      - It is now possible to build the documentation from the release
        tarball (Anna Henningsen)
        nodejs/node#8413
    * buffer:
      - Buffer.alloc() will no longer incorrectly return a zero filled
        buffer when an encoding is passed (Teddy Katz)
        nodejs/node#9238
    * deps:
      - upgrade npm in LTS to 2.15.11 (Kat Marchan)
        nodejs/node#8928
    * repl:
      - Enable tab completion for global properties (Lance Ball)
        nodejs/node#7369
    * url:
      - `url.format()` will now encode all `#` in `search` (Ilkka Myller)
        nodejs/node#8072

    PR-URL: nodejs/node#9298

Signed-off-by: Ilkka Myller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

url.format() encodes '#' inconsistently
6 participants
@imyller @addaleax @jasnell @mscdex @MylesBorins @nodejs-github-bot