From f896696fde996b94a4057a68d074aa25d61d2b70 Mon Sep 17 00:00:00 2001
From: vrana <jakubv@fb.com>
Date: Thu, 17 Jan 2013 11:50:49 -0800
Subject: [PATCH] Don't pop invalid test environment

Summary: If `unset($env)` throws then we pop some other environment instead which is impossible to pop later.

Test Plan:
  $ arc unit src/infrastructure/env/__tests__ src/applications/calendar/storage/__tests__

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4488
---
 src/infrastructure/env/PhabricatorEnv.php     |  1 +
 .../env/__tests__/PhabricatorEnvTestCase.php  | 26 +++----------------
 .../testing/PhabricatorTestCase.php           |  3 +++
 3 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php
index f9af13b0b8..6b5b263a8a 100644
--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -293,6 +293,7 @@ final class PhabricatorEnv {
     $source = self::$sourceStack->popSource();
     $stack_key = spl_object_hash($source);
     if ($stack_key !== $key) {
+      self::$sourceStack->pushSource($source);
       throw new Exception(
         "Scoped environments were destroyed in a diffent order than they ".
         "were initialized.");
diff --git a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
index 4adbe11459..13b5d9c5e2 100644
--- a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
+++ b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
@@ -130,15 +130,11 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase {
 
   public function testOverrideOrder() {
     $outer = PhabricatorEnv::beginScopedEnv();
-    $middle = PhabricatorEnv::beginScopedEnv();
     $inner = PhabricatorEnv::beginScopedEnv();
 
     $caught = null;
     try {
-      if (phutil_is_hiphop_runtime()) {
-        $middle->__destruct();
-      }
-      unset($middle);
+      $outer->__destruct();
     } catch (Exception $ex) {
       $caught = $ex;
     }
@@ -149,25 +145,11 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase {
       "Destroying a scoped environment which is not on the top of the stack ".
       "should throw.");
 
-    $caught = null;
-    try {
-      if (phutil_is_hiphop_runtime()) {
-        $inner->__destruct();
-      }
-      unset($inner);
-    } catch (Exception $ex) {
-      $caught = $ex;
+    if (phutil_is_hiphop_runtime()) {
+      $inner->__destruct();
     }
+    unset($inner);
 
-    $this->assertEqual(
-      true,
-      $caught instanceof Exception,
-      "Destroying a scoped environment which is not on the top of the stack ".
-      "should throw.");
-
-    // Although we popped the other two out-of-order, we still expect to end
-    // up in the right state after handling the exceptions, so this should
-    // execute without issues.
     if (phutil_is_hiphop_runtime()) {
       $outer->__destruct();
     }
diff --git a/src/infrastructure/testing/PhabricatorTestCase.php b/src/infrastructure/testing/PhabricatorTestCase.php
index 660fd17670..dbdd1ef126 100644
--- a/src/infrastructure/testing/PhabricatorTestCase.php
+++ b/src/infrastructure/testing/PhabricatorTestCase.php
@@ -91,6 +91,9 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
     }
 
     try {
+      if (phutil_is_hiphop_runtime()) {
+        $this->env->__destruct();
+      }
       unset($this->env);
     } catch (Exception $ex) {
       throw new Exception(