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. PR-URL: #25575 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
nodejs · Jan 23, 2019 · 92dd899 · 92dd899
1 parent 5684da5
commit 92dd899
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 2 deletions.
diff --git a/src/node_env_var.cc b/src/node_env_var.cc
@@ -72,8 +72,12 @@ static void EnvSetter(Local<Name> property,
                       Local<Value> value,
                       const PropertyCallbackInfo<Value>& 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
@@ -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');