From 5ab98a55bf3976033694a0dd5f4a3d2bbbe47c38 Mon Sep 17 00:00:00 2001 From: Benjamin Date: Sun, 20 Jan 2019 21:53:19 -0800 Subject: [PATCH] process: check env->EmitProcessEnvWarning() last Calling env->EmitProcessEnvWarning() prevents additional warnings from being set it should therefore be called only if a warning will emit. --- src/node_env_var.cc | 8 ++++++-- test/parallel/test-process-env-deprecation.js | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index d1ebb064e2417e..92312e954b8abf 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -72,8 +72,12 @@ static void EnvSetter(Local property, Local value, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); - if (env->options()->pending_deprecation && env->EmitProcessEnvWarning() && - !value->IsString() && !value->IsNumber() && !value->IsBoolean()) { + // calling env->EmitProcessEnvWarning() sets a variable indicating that + // warnings have been emitted. It should be called last after other + // conditions leading to a warning have been met. + if (env->options()->pending_deprecation && !value->IsString() && + !value->IsNumber() && !value->IsBoolean() && + env->EmitProcessEnvWarning()) { if (ProcessEmitDeprecationWarning( env, "Assigning any value other than a string, number, or boolean to a " diff --git a/test/parallel/test-process-env-deprecation.js b/test/parallel/test-process-env-deprecation.js index 68817b320b4717..0396d8ff68a47f 100644 --- a/test/parallel/test-process-env-deprecation.js +++ b/test/parallel/test-process-env-deprecation.js @@ -12,5 +12,9 @@ common.expectWarning( 'DEP0104' ); +// Make sure setting a valid environment variable doesn't +// result in warning being suppressed, see: +// https://github.com/nodejs/node/pull/25157 +process.env.FOO = 'apple'; process.env.ABC = undefined; assert.strictEqual(process.env.ABC, 'undefined');