Skip to content

[v18.x backport] src: replace idna functions with ada::idna#48873

Closed
anonrig wants to merge 3 commits intonodejs:v18.x-stagingfrom
anonrig:backport-47735
Closed

[v18.x backport] src: replace idna functions with ada::idna#48873
anonrig wants to merge 3 commits intonodejs:v18.x-stagingfrom
anonrig:backport-47735

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Jul 21, 2023 •
edited
Loading

This PR includes a slightly modified backport which moves the function declarations to node_url.cc instead of encoding_binding.cc where Node.js v18 does not have. I've also added a new test to validate the correctness of this backport.

Backport #47735
Backport #48878

Fixes #48855
Fixes #48850

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 21, 2023

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Jul 21, 2023
@anonrig anonrig requested review from aduh95 and richardlau July 21, 2023 15:26
@anonrig anonrig mentioned this pull request Jul 21, 2023
Closed
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 21, 2023

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

@lpinca
Copy link
Member

lpinca commented Jul 21, 2023 •
edited
Loading

test/parallel/test-url-parse.js does not exist in #47735. I would not create a new test file but update test/parallel/test-url-parse-format.js and also add it on main.

@anonrig
Copy link
Member Author

anonrig commented Jul 21, 2023

@lpinca I was going to follow up and create test-url-parse.js in main as well.

@aduh95 aduh95 changed the title [v18.x] backport: src: replace idna functions with ada::idna [v18.x backport] src: replace idna functions with ada::idna Jul 21, 2023
@lpinca
Copy link
Member

lpinca commented Jul 21, 2023

Ok but I don't think we need a new test file. Why not reusing test/parallel/test-url-parse-format.js?

@anonrig
Copy link
Member Author

anonrig commented Jul 21, 2023

Ok but I don't think we need a new test file. Why not reusing test/parallel/test-url-parse-format.js?

My only argument is that the file name suggests url.format() where it only contains the calls to url.format().

@lpinca
Copy link
Member

lpinca commented Jul 21, 2023

test-url-parse.js with a single special url in it does not make much sense to me. At least rename it to something like test-url-parse-hostname-with-comma.js

@aduh95
Copy link
Contributor

aduh95 commented Jul 21, 2023

@lpinca I was going to follow up and create test-url-parse.js in main as well.

I think we should land it on main first, fast-track it, and backport it in this PR.

@anonrig
Copy link
Member Author

anonrig commented Jul 21, 2023

test-url-parse.js with a single special url in it does not make much sense to me. At least rename it to something like test-url-parse-hostname-with-comma.js

Well, once you give a man a inferior choice, he'll appreciate the current choice he has, and goes ahead with test-url-parse-format.js. I'll wait for the tests to finish, to check if everything is ok, and later force push the file change. @lpinca

@anonrig anonrig mentioned this pull request Jul 21, 2023
Closed
nodejs-github-bot pushed a commit that referenced this pull request Jul 22, 2023
@anonrig @nodejs-github-bot
test: validate host with commas on url.parse
f4863fa 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jul 22, 2023
@anonrig @nodejs-github-bot
lib: remove invalid parameter to toASCII
9dd574c 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
anonrig and others added 3 commits July 21, 2023 21:34
@anonrig @lemire
src: replace idna functions with ada::idna
6c602b3 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#47735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@anonrig
test: validate host with commas on url.parse
a0edb33 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@anonrig
lib: remove invalid parameter to toASCII
60f1bb5 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@anonrig
Copy link
Member Author

anonrig commented Jul 22, 2023

@lpinca @aduh95 Updated the backport to include the changes in main

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 22, 2023

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

@aduh95
Copy link
Contributor

aduh95 commented Jul 22, 2023

@nodejs/lts it would be nice to have a patch release of v18.x with these commits to fix the regression on legacy url.parse.

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
@anonrig @Ceres6
test: validate host with commas on url.parse
932d92e 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
@anonrig @Ceres6
lib: remove invalid parameter to toASCII
9923dd9 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@anonrig anonrig mentioned this pull request Aug 2, 2023
Closed
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
@anonrig
test: validate host with commas on url.parse
f1d25bc 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
@anonrig
lib: remove invalid parameter to toASCII
c40e40a 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
@anonrig
test: validate host with commas on url.parse
24e9fb4 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
@anonrig
lib: remove invalid parameter to toASCII
161ff90 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@lpinca lpinca mentioned this pull request Aug 10, 2023
Closed
@styfle styfle mentioned this pull request Aug 14, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
test: validate host with commas on url.parse
ba96a46 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
lib: remove invalid parameter to toASCII
c433de3 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
test: validate host with commas on url.parse
6c0c565 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
lib: remove invalid parameter to toASCII
bb7ffa8 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@anonrig @UlisesGascon
test: validate host with commas on url.parse
17a1be6 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@anonrig @UlisesGascon
lib: remove invalid parameter to toASCII
5affce3 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@anonrig @RafaelGSS
test: validate host with commas on url.parse
3680a66 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@anonrig @RafaelGSS
lib: remove invalid parameter to toASCII
420b85c 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 16, 2023
@anonrig @lemire @ruyadorno
src: replace idna functions with ada::idna
e426180 
Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #47735
Backport-PR-URL: #48873
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 16, 2023
@anonrig @ruyadorno
test: validate host with commas on url.parse
2de9868 
PR-URL: #48878
Backport-PR-URL: #48873
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 16, 2023
@anonrig @ruyadorno
lib: remove invalid parameter to toASCII
69aaf8b 
PR-URL: #48878
Backport-PR-URL: #48873
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@ruyadorno
Copy link
Member

ruyadorno commented Aug 16, 2023

Landed in f4617a4...69aaf8b

@ruyadorno ruyadorno closed this Aug 16, 2023
This was referenced Aug 28, 2023
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@richardlau richardlau Awaiting requested review from richardlau

@aduh95 aduh95 Awaiting requested review from aduh95

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@anonrig @nodejs-github-bot @lpinca @aduh95 @ruyadorno @benjamingr

Comments