Issue #1481156 by lucascaro, npiacentine: Fixed Incorrect logic in creating url to fetch information about project updates.

2 files changed, 56 insertions, 2 deletions

diff --git a/modules/update/update.fetch.inc b/modules/update/update.fetch.inc
index 88132cec3..ee5d77b16 100644
--- a/modules/update/update.fetch.inc
+++ b/modules/update/update.fetch.inc
@@ -281,7 +281,7 @@ function _update_build_fetch_url($project, $site_key = '') {
   // in the first place, and if this is not a disabled module or theme. We do
   // not want to record usage statistics for disabled code.
   if (!empty($site_key) && (strpos($project['project_type'], 'disabled') === FALSE)) {
-    $url .= (strpos($url, '?') === TRUE) ? '&' : '?';
+    $url .= (strpos($url, '?') !== FALSE) ? '&' : '?';
     $url .= 'site_key=';
     $url .= rawurlencode($site_key);
     if (!empty($project['info']['version'])) {
diff --git a/modules/update/update.test b/modules/update/update.test
index e8051a27d..8daa82155 100644
--- a/modules/update/update.test
+++ b/modules/update/update.test
@@ -676,7 +676,7 @@ class UpdateTestUploadCase extends UpdateTestHelper {
    * Ensure that archiver extensions are properly merged in the UI.
    */
   function testFileNameExtensionMerging() {
-    $this->drupalGet('admin/modules/install');    
+    $this->drupalGet('admin/modules/install');
     // Make sure the bogus extension supported by update_test.module is there.
     $this->assertPattern('/file extensions are supported:.*update-test-extension/', t("Found 'update-test-extension' extension"));
     // Make sure it didn't clobber the first option from core.
@@ -724,3 +724,57 @@ class UpdateTestUploadCase extends UpdateTestHelper {
   }
 
 }
+
+class UpdateCoreUnitTestCase extends DrupalUnitTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => "Unit tests",
+      'description' => 'Test update funcionality unrelated to the database.',
+      'group' => 'Update',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('update');
+    module_load_include('inc', 'update', 'update.fetch');
+  }
+
+  /**
+   * Tests _update_build_fetch_url according to issue 1481156
+   */
+  function testUpdateBuildFetchUrl() {
+    //first test that we didn't break the trivial case
+    $project['name'] = 'update_test';
+    $project['project_type'] = '';
+    $project['info']['version'] = '';
+    $project['info']['project status url'] = 'http://www.example.com';
+    $site_key = '';
+    $expected = 'http://www.example.com/' . $project['name'] . '/' . DRUPAL_CORE_COMPATIBILITY;
+    $url = _update_build_fetch_url($project, $site_key);
+    $this->assertEqual($url, $expected, "'$url' when no site_key provided should be '$expected'.");
+
+    //For disabled projects it shouldn't add the site key either.
+    $site_key = 'site_key';
+    $project['project_type'] = 'disabled';
+    $expected = 'http://www.example.com/' . $project['name'] . '/' . DRUPAL_CORE_COMPATIBILITY;
+    $url = _update_build_fetch_url($project, $site_key);
+    $this->assertEqual($url, $expected, "'$url' should be '$expected' for disabled projects.");
+
+    //for enabled projects, adding the site key
+    $project['project_type'] = '';
+    $expected = 'http://www.example.com/' . $project['name'] . '/' . DRUPAL_CORE_COMPATIBILITY;
+    $expected .= '?site_key=site_key';
+    $url = _update_build_fetch_url($project, $site_key);
+    $this->assertEqual($url, $expected, "When site_key provided, '$url' should be '$expected'.");
+
+    // http://drupal.org/node/1481156 test incorrect logic when url contains
+    // a question mark.
+    $project['info']['project status url'] = 'http://www.example.com/?project=';
+    $expected = 'http://www.example.com/?project=/' . $project['name'] . '/' . DRUPAL_CORE_COMPATIBILITY;
+    $expected .= '&site_key=site_key';
+    $url = _update_build_fetch_url($project, $site_key);
+    $this->assertEqual($url, $expected, "When ? is present, '$url' should be '$expected'.");
+
+  }
+}
\ No newline at end of file