From 4edfd35503c3809a91f81a5820f73268da9b479e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 13 Dec 2011 17:34:41 -0800
Subject: [PATCH] Fix qsprintf() '%nd' conversion

Summary:
I broke this a little bit in my overzealous D1174, since this block validates
both '%nd' (nullable integer) and '%d' (non-nullable integer).

Clean up the conditional checks so we catch the bad case ('%d' on a PHID
converting to 0) but let the good case ('%nd' with null) through.

Test Plan: Unit tests failed; applied patch; unit tests pass.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T670

Differential Revision: 1201
---
 src/__phutil_library_map__.php                |  2 +
 .../__tests__/QueryFormattingTestCase.php     | 56 +++++++++++++++++++
 src/storage/qsprintf/__tests__/__init__.php   | 16 ++++++
 src/storage/qsprintf/qsprintf.php             |  9 +--
 4 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 src/storage/qsprintf/__tests__/QueryFormattingTestCase.php
 create mode 100644 src/storage/qsprintf/__tests__/__init__.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index a534a3e965..8ee7cc84e6 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -724,6 +724,7 @@ phutil_register_library_map(array(
     'PhrictionEditController' => 'applications/phriction/controller/edit',
     'PhrictionHistoryController' => 'applications/phriction/controller/history',
     'PhrictionListController' => 'applications/phriction/controller/list',
+    'QueryFormattingTestCase' => 'storage/qsprintf/__tests__',
   ),
   'function' =>
   array(
@@ -1337,6 +1338,7 @@ phutil_register_library_map(array(
     'PhrictionEditController' => 'PhrictionController',
     'PhrictionHistoryController' => 'PhrictionController',
     'PhrictionListController' => 'PhrictionController',
+    'QueryFormattingTestCase' => 'PhabricatorTestCase',
   ),
   'requires_interface' =>
   array(
diff --git a/src/storage/qsprintf/__tests__/QueryFormattingTestCase.php b/src/storage/qsprintf/__tests__/QueryFormattingTestCase.php
new file mode 100644
index 0000000000..2d015948fc
--- /dev/null
+++ b/src/storage/qsprintf/__tests__/QueryFormattingTestCase.php
@@ -0,0 +1,56 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class QueryFormattingTestCase extends PhabricatorTestCase {
+
+  public function testQueryFormatting() {
+    $conn_r = id(new PhabricatorUser())->establishConnection('r');
+
+    $this->assertEqual(
+      'NULL',
+      qsprintf($conn_r, '%nd', null));
+
+    $this->assertEqual(
+      '0',
+      qsprintf($conn_r, '%nd', 0));
+
+    $this->assertEqual(
+      '0',
+      qsprintf($conn_r, '%d', 0));
+
+    $raised = null;
+    try {
+      qsprintf($conn_r, '%d', 'derp');
+    } catch (Exception $ex) {
+      $raised = $ex;
+    }
+    $this->assertEqual(
+      (bool)$raised,
+      true,
+      'qsprintf should raise exception for invalid %d conversion.');
+
+    $this->assertEqual(
+      "'<S>'",
+      qsprintf($conn_r, '%s', null));
+
+    $this->assertEqual(
+      'NULL',
+      qsprintf($conn_r, '%ns', null));
+  }
+
+}
diff --git a/src/storage/qsprintf/__tests__/__init__.php b/src/storage/qsprintf/__tests__/__init__.php
new file mode 100644
index 0000000000..e929d136e1
--- /dev/null
+++ b/src/storage/qsprintf/__tests__/__init__.php
@@ -0,0 +1,16 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/people/storage/user');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+phutil_require_module('phabricator', 'storage/qsprintf');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('QueryFormattingTestCase.php');
diff --git a/src/storage/qsprintf/qsprintf.php b/src/storage/qsprintf/qsprintf.php
index d955417cef..e0323be161 100644
--- a/src/storage/qsprintf/qsprintf.php
+++ b/src/storage/qsprintf/qsprintf.php
@@ -278,15 +278,10 @@ function _qsprintf_check_scalar_type($value, $type, $query) {
       break;
 
     case 'Ld': case 'd': case 'f':
-      if (!is_null($value) && !is_scalar($value)) {
+      if (!is_null($value) && !is_numeric($value)) {
         throw new AphrontQueryParameterException(
           $query,
-          "Expected a scalar or null for %{$type} conversion.");
-      }
-      if (!is_numeric($value)) {
-        throw new AphrontQueryParameterException(
-          $query,
-          "Expected numeric value for %{$type} conversion.");
+          "Expected a numeric scalar or null for %{$type} conversion.");
       }
       break;