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.

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

url.port = "" sets the port number to null. #113

Merged
merged 3 commits into from
Oct 28, 2016

Conversation

SimonSapin
Copy link
Contributor

It was previously (per spec) a no-op.

This matches Firefox. (Chromium sets the port to zero.)
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4054

(url.html not regenerated since the latest version of Bikeshed does various things differently, generating unrelated diff churn.)

@SimonSapin
Copy link
Contributor Author

This also affects url.host = "example.net:"

@SimonSapin SimonSapin changed the title url.port = '' sets the port number to null. url.port = "" sets the port number to null. Apr 13, 2016
@annevk
Copy link
Member

annevk commented Apr 14, 2016

The example.net does not seem to match Firefox (although Chrome does set it to zero there).

@annevk
Copy link
Member

annevk commented Apr 14, 2016

We could also define this as part of https://url.spec.whatwg.org/#dom-url-port or check that state override is "port state"...

@annevk
Copy link
Member

annevk commented Apr 14, 2016

Probably defining it in the port setter would be clearer.

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request May 11, 2016
See #2830 (comment).

This can be un-reverted when the following are fixed in the spec:

- whatwg/url#113
- whatwg/url#104
arronei pushed a commit to arronei/web-platform-tests that referenced this pull request Jun 14, 2016
See web-platform-tests#2830 (comment).

This can be un-reverted when the following are fixed in the spec:

- whatwg/url#113
- whatwg/url#104
ivanzr pushed a commit to ivanzr/web-platform-tests that referenced this pull request Jun 29, 2016
See web-platform-tests#2830 (comment).

This can be un-reverted when the following are fixed in the spec:

- whatwg/url#113
- whatwg/url#104
@annevk
Copy link
Member

annevk commented Oct 19, 2016

@achristensen07 @cdumez what do you think about changing the behavior of url.port = ""?

Presumably we should also change HTML if we change this here. Did you check location?

@achristensen07
Copy link
Collaborator

I agree with this change. If someone wants port 0, they would set url.port to 0, not the empty string. I was planning on changing the behavior of port 0 vs. no port anyways.

@annevk
Copy link
Member

annevk commented Oct 20, 2016

It seems Firefox and Safari use the default port (not 0) for location. Safari does still use 0 elsewhere, but it seems Alex is willing to change that, yay. I guess I'll change this PR to be specifically for the port setter and create a corresponding patch for HTML.

annevk added a commit to whatwg/html that referenced this pull request Oct 28, 2016
Setting it to the empty string should set the underlying port concept
to null. See web-platform-tests/wpt#4101 for
test changes and whatwg/url#113 for the change
to the URL Standard.
@annevk annevk merged commit 05ffaa6 into whatwg:master Oct 28, 2016
@annevk
Copy link
Member

annevk commented Oct 28, 2016

Thanks for the review @zcorpan and thanks @SimonSapin for the original patch (although do note I went with a different fix).

zcorpan pushed a commit to whatwg/html that referenced this pull request Oct 28, 2016
Setting it to the empty string should set the underlying port concept
to null. See web-platform-tests/wpt#4101 for
test changes and whatwg/url#113 for the change
to the URL Standard.
@SimonSapin SimonSapin deleted the empty-port branch November 8, 2016 08:28
nox added a commit to nox/servo that referenced this pull request Nov 8, 2016
targos added a commit to targos/node that referenced this pull request Jan 1, 2017
This patch contains the following changes:

url: make IPv4 parser more spec compliant

* Return int64_t from ParseNumber to prevent overflow for valid big numbers
* Don't throw when there are more than 4 parts (it cannot be an IP
address)
* Correctly interpret the address and don't always throw when there are
numbers > 255

Ref: https://url.spec.whatwg.org/#concept-ipv4-parser
Fixes: nodejs#10306

url: percent encode fragment to follow spec change

Ref: whatwg/url#150
Ref: whatwg/url@373dbed

url: fix URL#search setter

The check for empty string must be done before removing the leading '?'.

Ref: https://url.spec.whatwg.org/#dom-url-search

url: set port to null if an empty string is given

This is to follow a spec change.

Ref: whatwg/url#113

url: fix parsing of paths with Windows drive letter

test: update WHATWG URL test fixtures

PR-URL: nodejs#10317
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this pull request Jan 3, 2017
This patch contains the following changes:

url: make IPv4 parser more spec compliant

* Return int64_t from ParseNumber to prevent overflow for valid big numbers
* Don't throw when there are more than 4 parts (it cannot be an IP
address)
* Correctly interpret the address and don't always throw when there are
numbers > 255

Ref: https://url.spec.whatwg.org/#concept-ipv4-parser
Fixes: #10306

url: percent encode fragment to follow spec change

Ref: whatwg/url#150
Ref: whatwg/url@373dbed

url: fix URL#search setter

The check for empty string must be done before removing the leading '?'.

Ref: https://url.spec.whatwg.org/#dom-url-search

url: set port to null if an empty string is given

This is to follow a spec change.

Ref: whatwg/url#113

url: fix parsing of paths with Windows drive letter

test: update WHATWG URL test fixtures

PR-URL: #10317
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this pull request Jan 4, 2017
This patch contains the following changes:

url: make IPv4 parser more spec compliant

* Return int64_t from ParseNumber to prevent overflow for valid big numbers
* Don't throw when there are more than 4 parts (it cannot be an IP
address)
* Correctly interpret the address and don't always throw when there are
numbers > 255

Ref: https://url.spec.whatwg.org/#concept-ipv4-parser
Fixes: #10306

url: percent encode fragment to follow spec change

Ref: whatwg/url#150
Ref: whatwg/url@373dbed

url: fix URL#search setter

The check for empty string must be done before removing the leading '?'.

Ref: https://url.spec.whatwg.org/#dom-url-search

url: set port to null if an empty string is given

This is to follow a spec change.

Ref: whatwg/url#113

url: fix parsing of paths with Windows drive letter

test: update WHATWG URL test fixtures

PR-URL: #10317
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
Setting it to the empty string should set the underlying port concept
to null. See web-platform-tests/wpt#4101 for
test changes and whatwg/url#113 for the change
to the URL Standard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants