PR #24: Fix using the wrong domain when loading from Drupal vars

justafish · May 11, 2015 · 35b25e8 · 35b25e8
2 parents 4487e16 + 2f1bde6
commit 35b25e8
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 16 deletions.
diff --git a/src/StreamWrapper.php b/src/StreamWrapper.php
@@ -22,6 +22,13 @@ class StreamWrapper extends \Aws\S3\StreamWrapper implements \DrupalStreamWrappe
   use DrupalAdapter\Common;
   use DrupalAdapter\FileMimetypes;
 
+  /**
+   * The base domain of S3.
+   *
+   * @const string
+   */
+  const S3_API_DOMAIN = 's3.amazonaws.com';
+
   /**
    * The path to the image style generation callback.
    *
@@ -361,19 +368,27 @@ public function getOptions() {
     return $options;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function mkdir($path, $mode, $options) {
+    $this->setUri($path);
+    return parent::mkdir($path, $mode, $options);
+  }
+
   /**
    * {@inheritdoc}
    */
   public function stream_open($path, $mode, $options, &$opened_path) {
-    $this->uri = S3Url::factory($path);
+    $this->setUri($path);
     return parent::stream_open($path, $mode, $options, $opened_path);
   }
 
   /**
    * {@inheritdoc}
    */
   public function url_stat($path, $flags) {
-    $this->uri = S3Url::factory($path);
+    $this->setUri($path);
     return parent::url_stat($path, $flags);
   }
 
@@ -452,7 +467,7 @@ protected function useRrs() {
    *   The URL to modify.
    */
   protected function injectCname($url) {
-    if ($this->config->getDomain() != $url->getHost()) {
+    if (strpos($url->getHost(), $this->config->getDomain()) === FALSE) {
       $url->setHost($this->config->getDomain());
     }
   }

diff --git a/src/StreamWrapperConfiguration.php b/src/StreamWrapperConfiguration.php
@@ -62,13 +62,7 @@ public static function fromConfig(array $config = array(), array $defaults = arr
     }
 
     if (!$data['domain']) {
-      $data['domain'] = 's3.amazonaws.com';
-
-      // If the bucket does not contain dots, the S3 SDK generates URLs that
-      // use the bucket name in the host.
-      if (strpos($data['bucket'], '.') === FALSE) {
-        $data['domain'] = $data['bucket'] . '.' . $data['domain'];
-      }
+      $data['domain'] = self::getS3Domain($data['bucket']);
     }
 
     return new static($data);
@@ -105,6 +99,25 @@ protected static function required() {
     return $required;
   }
 
+  /**
+   * Get the S3 domain for a bucket.
+   *
+   * @param string $bucket
+   *   The bucket to get the domain for.
+   * @return string
+   *   The S3 domain, such as bucket.s3.amazonaws.com.
+   */
+  protected static function getS3Domain($bucket) {
+    $domain = StreamWrapper::S3_API_DOMAIN;
+    // If the bucket does not contain dots, the S3 SDK generates URLs that
+    // use the bucket name in the host.
+    if (strpos($bucket, '.') === FALSE) {
+      $domain = $bucket . '.' . $domain;
+    }
+
+    return $domain;
+  }
+
 
   /**
    * Get the API hostname.
@@ -353,7 +366,7 @@ public static function fromDrupalVariables() {
       }
     }
     else {
-      $config->setDomain('s3.amazonaws.com');
+      $config->setDomain(static::getS3Domain($config->getBucket()));
     }
 
     // Check whether local file caching is turned on.

diff --git a/tests/StreamWrapperConfigurationTest.php b/tests/StreamWrapperConfigurationTest.php
@@ -57,11 +57,11 @@ public function testSetters() {
     $config->disableCaching();
     $this->assertFalse($config->isCaching());
 
-    $config->setDomain('api.example.com');
-    $this->assertEquals('api.example.com', $config->getDomain());
+    $config->setDomain('cdn.example.com');
+    $this->assertEquals('cdn.example.com', $config->getDomain());
 
-    $config->setHostname('cdn.example.com');
-    $this->assertEquals('cdn.example.com', $config->getHostname());
+    $config->setHostname('api.example.com');
+    $this->assertEquals('api.example.com', $config->getHostname());
 
     $config->serveWithCloudFront();
     $this->assertTrue($config->isCloudFront());
@@ -123,6 +123,7 @@ public function testDefaultHostname() {
 
   /**
    * @covers Drupal\amazons3\StreamWrapperConfiguration::fromDrupalVariables
+   * @covers Drupal\amazons3\StreamWrapperConfiguration::getS3Domain
    */
   public function testFromDrupalVariables() {
     StreamWrapperConfiguration::setVariableData([
@@ -161,17 +162,27 @@ public function testFromDrupalVariables() {
     $this->assertEquals($config->getBucket(), $config->getDomain());
     $this->assertFalse($config->isCaching());
 
+    // When the bucket has a dot, check that the bucket is not in the domain.
     StreamWrapperConfiguration::setVariableData([
       'amazons3_bucket' => 'default.example.com',
     ]);
     $config = StreamWrapperConfiguration::fromDrupalVariables();
     $this->assertEquals('s3.amazonaws.com', $config->getDomain());
 
+    // When the bucket does not have a dot, check the bucket is in the
+    // subdomain.
+    StreamWrapperConfiguration::setVariableData([
+      'amazons3_bucket' => 'bucket',
+    ]);
+    $config = StreamWrapperConfiguration::fromDrupalVariables();
+    $this->assertEquals('bucket.s3.amazonaws.com', $config->getDomain());
+
     StreamWrapperConfiguration::setVariableData(array());
   }
 
   /**
    * @covers Drupal\amazons3\StreamWrapperConfiguration::fromConfig
+   * @covers Drupal\amazons3\StreamWrapperConfiguration::getS3Domain
    * @expectedException \InvalidArgumentException
    */
   public function testEmptyRequiredStringFails() {

diff --git a/tests/StreamWrapperTest.php b/tests/StreamWrapperTest.php
@@ -521,6 +521,7 @@ public function testReducedRedundancyStorage() {
 
   /**
    * @covers \Drupal\amazons3\StreamWrapper::stream_open
+   * @covers \Drupal\amazons3\StreamWrapper::mkdir
    * @covers \Drupal\amazons3\StreamWrapper::url_stat
    */
   public function testAutomaticUri() {
@@ -563,11 +564,16 @@ public function testAutomaticUri() {
     \Guzzle\Tests\GuzzleTestCase::setServiceBuilder($aws);
     $client = $this->getServiceBuilder()->get('s3', true);
 
-    $this->setMockResponse($client, array(new Response(200)));
+    // The 404 is for the first check to mkdir() that checks to see if a
+    // directory exists.
+    $this->setMockResponse($client, array(new Response(200), new Response(404), new Response(200)));
 
     $wrapper = new StreamWrapper();
     $wrapper->setClient($client);
     $wrapper->url_stat($uri, 0);
     $this->assertEquals($uri, $wrapper->getUri());
+
+    $wrapper->mkdir('s3://bucket.example.com/directory', 0, 0);
+    $this->assertEquals('s3://bucket.example.com/directory', $wrapper->getUri());
   }
 }