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: prevent pathname setter from erasing path of path-only URLs #39060

Closed
wants to merge 1 commit into from
Closed

url: prevent pathname setter from erasing path of path-only URLs #39060

wants to merge 1 commit into from

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Jun 17, 2021 •
edited
Loading

This change prevents the pathname setter from erasing the path of
path-only URLs as that would make them cannot-be-a-base URLs.

The changes in all files except src/node_url.cc have been done by
running:

git node wpt url

Fixes: #39059
Signed-off-by: Darshan Sen darshan.sen@postman.com

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jun 17, 2021
@RaisinTen RaisinTen mentioned this pull request Jun 17, 2021
Closed
@RaisinTen RaisinTen added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@addaleax
Copy link
Member

Can you add a meaningful commit description or commit subject line?

@RaisinTen RaisinTen changed the title url: pathname setter change url: prevent pathname setter from erasing path of path-only URLs Jun 18, 2021
@RaisinTen
Copy link
Contributor Author

@addaleax Updated. :) PTAL.

RaisinTen
RaisinTen commented Jun 18, 2021
View reviewed changes
src/node_url.cc Outdated Show resolved Hide resolved
@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 18, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen RaisinTen added blocked PRs that are blocked by other issues or PRs. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 19, 2021
@RaisinTen
Copy link
Contributor Author

Blocked till web-platform-tests/wpt#27720 lands.

@RaisinTen RaisinTen added request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Jun 26, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen
url: prevent pathname setter from erasing path of path-only URLs
0a7e75d 
This change prevents the pathname setter from erasing the path of
path-only URLs as that would make them cannot-be-a-base URLs.

The changes in all files except `src/node_url.cc` have been done by
running:

```console
git node wpt url
```

Fixes: nodejs#39059
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
@RaisinTen RaisinTen added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 17, 2021 •
edited by RaisinTen
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/39097/ (⭐)

@RaisinTen
Copy link
Contributor Author

Also, cc @nodejs/url

@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 18, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 18, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 18, 2021
@github-actions
Copy link
Contributor

Landed in fdf625b...00cac65

@github-actions github-actions bot closed this Jul 18, 2021
nodejs-github-bot pushed a commit that referenced this pull request Jul 18, 2021
@RaisinTen @nodejs-github-bot
url: prevent pathname setter from erasing path of path-only URLs
00cac65 
This change prevents the pathname setter from erasing the path of
path-only URLs as that would make them cannot-be-a-base URLs.

The changes in all files except `src/node_url.cc` have been done by
running:

```console
git node wpt url
```

Fixes: #39059
Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #39060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@RaisinTen RaisinTen deleted the url/pathname-setter-change branch July 18, 2021 05:45
targos pushed a commit that referenced this pull request Jul 20, 2021
@RaisinTen @targos
url: prevent pathname setter from erasing path of path-only URLs
4b52a34 
This change prevents the pathname setter from erasing the path of
path-only URLs as that would make them cannot-be-a-base URLs.

The changes in all files except `src/node_url.cc` have been done by
running:

```console
git node wpt url
```

Fixes: #39059
Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #39060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
Merged
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
@RaisinTen @BethGriggs
url: prevent pathname setter from erasing path of path-only URLs
68fd6d5 
This change prevents the pathname setter from erasing the path of
path-only URLs as that would make them cannot-be-a-base URLs.

The changes in all files except `src/node_url.cc` have been done by
running:

```console
git node wpt url
```

Fixes: #39059
Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #39060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@annevk annevk annevk left review comments

@aduh95 aduh95 aduh95 left review comments

@addaleax addaleax Awaiting requested review from addaleax

@jasnell jasnell Awaiting requested review from jasnell

@XadillaX XadillaX Awaiting requested review from XadillaX

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

URL: pathname setter change
9 participants
@RaisinTen @nodejs-github-bot @addaleax @jasnell @TimothyGu @annevk @XadillaX @aduh95 @targos