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

crypto: callback's error of crypto.pbkdf2 is set to undefined #18458

Closed
wants to merge 1 commit into from
Closed

crypto: callback's error of crypto.pbkdf2 is set to undefined #18458

wants to merge 1 commit into from

Conversation

BufoViridis
Copy link
Contributor

@BufoViridis BufoViridis commented Jan 30, 2018

As the documentation for crypto.pbkdf2 states, the function on success will set the first argument to null.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

As the documentation for crypto.pbkdf2 (https://nodejs.org/dist/latest-v8.x/docs/api/crypto.html#crypto_crypto_pbkdf2_password_salt_iterations_keylen_digest_callback) say, the function on success will return the first argument set to null and not undefined.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Jan 30, 2018
@tniessen
Copy link
Member

cc @nodejs/crypto, are we being consistent here?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but a regression test would be nice.

@tniessen Most callbacks in core use null.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

LGTM with @bnoordhuis' comment.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@tniessen tniessen self-assigned this Jan 30, 2018
@tniessen
Copy link
Member

The commit message should be refined when landing.

@tniessen
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 6, 2018
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: nodejs#18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Feb 6, 2018

Landed in 6a29630

@BufoViridis thanks a lot! I fixed the subsystem and the commit message according to our guidlines while landing the PR.

@BridgeAR BridgeAR closed this Feb 6, 2018
@BufoViridis BufoViridis deleted the patch-1 branch February 7, 2018 16:49
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: nodejs#18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants