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

http: correctly translate HTTP method #52701

Closed
wants to merge 1 commit into from
Closed

http: correctly translate HTTP method #52701

wants to merge 1 commit into from

Conversation

ShogunPanda
Copy link
Contributor

This PR correctly maps the HTTP method reported by llhttp (which is an index of an array) to the right array.

Fixes #51562.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run. labels Apr 26, 2024
@ShogunPanda ShogunPanda mentioned this pull request Apr 26, 2024
Open
mcollina
mcollina approved these changes Apr 26, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@bricss
Copy link

bricss commented Apr 27, 2024

I wonder if HTTP_KNOWN_METHODS(V) should be extended as well to make constants consistent?

@mcollina
Copy link
Member

@ShogunPanda C++ linting is failing

@ShogunPanda
Copy link
Contributor Author

@mcollina Fixed.

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 3, 2024

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/8938415767

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

jasnell pushed a commit that referenced this pull request May 4, 2024
@ShogunPanda @jasnell
http: correctly translate HTTP method
65c8380 
PR-URL: #52701
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented May 4, 2024

Landed in 65c8380

@jasnell jasnell closed this May 4, 2024
@ShogunPanda ShogunPanda deleted the http-search-all-methods branch May 5, 2024 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@panva panva panva approved these changes

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for 'QUERY' method
10 participants
@ShogunPanda @nodejs-github-bot @bricss @mcollina @jasnell @panva @benjamingr @ronag @marco-ippolito @richardlau