Merge branch '1.x' of https://github.com/City-of-Helsinki/drupal-modu…

…le-helfi-proxy into 1.x
City-of-Helsinki · Jan 31, 2022 · 5b8ffa9 · 5b8ffa9
2 parents ec4142b + b0bf0a6
commit 5b8ffa9
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 41 deletions.
diff --git a/README.md b/README.md
@@ -62,7 +62,7 @@ robots_paths:
 
 Enable modules with `drush en varnish_purger varnish_purge_tags purge_drush purge_processor_cron purge_queuer_coretags purge_tokens`.
 
-Copy configuration from `helfi_proxy/config/optional` to your `conf/cmi` folder if you enabled this module before Varnish/Purge modules and import configuration changes.
+Copy configuration from `helfi_proxy/config/optional` to your `conf/cmi` folder if you enabled this module before Varnish/Purge modules.
 
 Add these to your settings.php to use varnish cache:
 

diff --git a/helfi_proxy.services.yml b/helfi_proxy.services.yml
@@ -16,7 +16,7 @@ services:
     tags:
       # Must be run before PathProcessorFront (200 weight).
       - { name: path_processor_inbound, priority: 201 }
-      # Must to be run after LanguageProcessorLanguage (100 weight).
+      # Must be run after LanguageProcessorLanguage (100 weight).
       - { name: path_processor_outbound, priority: 99 }
 
   cache_context.site_prefix:

diff --git a/src/ProxyManager.php b/src/ProxyManager.php
@@ -33,32 +33,6 @@ public function __construct(ConfigFactoryInterface $configFactory) {
     $this->config = $configFactory->get('helfi_proxy.settings');
   }
 
-  /**
-   * Converts the given URL to relative.
-   *
-   * @param string|null $value
-   *   The value.
-   *
-   * @return string|null
-   *   the value.
-   */
-  private function convertAbsoluteToRelative(?string $value) : ? string {
-    if (!$value) {
-      return $value;
-    }
-    $parts = parse_url($value);
-
-    // Value is already relative.
-    if (empty($parts['host'])) {
-      return $value;
-    }
-
-    if (isset($parts['path'])) {
-      return $parts['path'] . (isset($parts['query']) ? '?' . $parts['query'] : NULL);
-    }
-    return $value;
-  }
-
   /**
    * Checks if given URL is hosted from a CDN.
    *
@@ -95,6 +69,24 @@ private function isCdnAddress(?string $value) : bool {
     return FALSE;
   }
 
+  /**
+   * Checks whether the asset is hosted locally.
+   *
+   * @param string $value
+   *   The path to given asset.
+   *
+   * @return bool
+   *   TRUE if asset is local.
+   */
+  private function isLocalAsset(string $value) : bool {
+    return match(TRUE) {
+      str_starts_with($value, '/sites'),
+      str_starts_with($value, '/core'),
+      str_starts_with($value, '/themes') => TRUE,
+      default => FALSE,
+    };
+  }
+
   /**
    * Handles tags with 'alwaysAbsolute' option.
    *
@@ -107,12 +99,24 @@ private function isCdnAddress(?string $value) : bool {
    *   The value.
    */
   private function handleAlwaysAbsolute(Request $request, string $value) : ? string {
-    $value = $this->convertAbsoluteToRelative($value);
+    if (!$value) {
+      return $value;
+    }
+    $parts = parse_url($value);
 
-    // Skip non-relative values.
-    if (!str_starts_with($value, '/')) {
+    // Value is already relative.
+    if (empty($parts['host']) || empty($parts['path'])) {
       return $value;
     }
+
+    // Skip non-local assets.
+    if (!$this->isLocalAsset($parts['path'])) {
+      return $value;
+    }
+
+    if (isset($parts['path'])) {
+      $value = $parts['path'] . (isset($parts['query']) ? '?' . $parts['query'] : NULL);
+    }
     return sprintf('%s%s', $request->getSchemeAndHttpHost(), $this->addAssetPath($value));
   }
 
@@ -138,14 +142,16 @@ private function addAssetPath(string $value) : ? string {
    *   The value.
    * @param string $separator
    *   The separator.
+   * @param callable $callback
+   *   The callback to run value through.
    *
    * @return string|null
    *   The value.
    */
-  private function handleMultiValue(string $value, string $separator) : ? string {
+  private function handleMultiValue(string $value, string $separator, callable $callback) : ? string {
     $parts = [];
     foreach (explode($separator, $value) as $item) {
-      $parts[] = $this->addAssetPath(trim($item));
+      $parts[] = $callback(trim($item));
     }
     return implode($separator, $parts);
   }
@@ -176,6 +182,53 @@ private function handleSitePrefix(Request $request, string $value) : ? string {
     return sprintf('%s/%s', $prefix, ltrim($value, '/'));
   }
 
+  /**
+   * Checks if the given value is an image style.
+   *
+   * @param string $value
+   *   The value.
+   *
+   * @return bool
+   *   TRUE if image is image style.
+   */
+  private function isImageStyle(string $value) : bool {
+    return str_contains($value, '/files/styles/');
+  }
+
+  /**
+   * Special handling for image styles.
+   *
+   * @param \Drupal\helfi_proxy\Selector\Selector $tag
+   *   The tag.
+   * @param string $value
+   *   The value with domain added to it.
+   *
+   * @return string|null
+   *   The image style url.
+   */
+  private function handleImageStyle(Selector $tag, string $value) : ? string {
+    if ($tag->multipleValues) {
+      return $this->handleMultiValue($value, $tag->multivalueSeparator,
+        function (string $value): string {
+          return $this->addDomain($value);
+        });
+    }
+    return $this->addDomain($value);
+  }
+
+  /**
+   * Adds domain to relative URL.
+   *
+   * @param string $value
+   *   The value.
+   *
+   * @return string
+   *   The value with domain added to it.
+   */
+  private function addDomain(string $value) : string {
+    return sprintf('//%s/%s', $this->getHostname(), ltrim($value, '/'));
+  }
+
   /**
    * {@inheritdoc}
    */
@@ -184,9 +237,14 @@ public function getAttributeValue(Request $request, Selector $tag, ?string $valu
     if (!$value || $this->isCdnAddress($value)) {
       return $value;
     }
-    // Certain elements are absolute URLs already (such as og:image:url)
-    // so we need to convert them to relative first and then back to
-    // absolute.
+
+    // Image styles need special handling because they need to be run through
+    // PHP before they are uploaded to CDN.
+    if ($this->isImageStyle($value)) {
+      return $this->handleImageStyle($tag, $value);
+    }
+    // Certain elements might be absolute URLs already (such as og:image:url).
+    // Make sure locally hosted files are always served from correct domain.
     if ($tag->alwaysAbsolute) {
       return $this->handleAlwaysAbsolute($request, $value);
     }
@@ -202,7 +260,11 @@ public function getAttributeValue(Request $request, Selector $tag, ?string $valu
     }
 
     if ($tag->multipleValues) {
-      return $this->handleMultiValue($value, $tag->multivalueSeparator);
+      return $this->handleMultiValue($value, $tag->multivalueSeparator,
+        function (string $value) : string {
+          return $this->addAssetPath($value);
+        }
+      );
     }
 
     return $this->addAssetPath($value);

diff --git a/src/ProxyTrait.php b/src/ProxyTrait.php
@@ -27,6 +27,7 @@ public function getHostname() : string {
     }
 
     $variables = [
+      'DRUPAL_BACKEND_DOMAIN',
       'DRUPAL_ROUTES',
       'DRUPAL_REVERSE_PROXY_ADDRESS',
     ];

diff --git a/src/Selector/Selector.php b/src/Selector/Selector.php
@@ -15,7 +15,7 @@ class Selector {
    * @param string $xpath
    *   The tag.
    * @param string $attribute
-   *   The atribute.
+   *   The attribute.
    * @param bool $alwaysAbsolute
    *   Whether the given value needs to be converted to relative url.
    * @param bool $sitePrefix

diff --git a/tests/src/Kernel/ProxyManagerTest.php b/tests/src/Kernel/ProxyManagerTest.php
@@ -174,9 +174,9 @@ public function testMetaTags() : void {
     $this->setAssetPath('test-assets');
 
     foreach (['og:image', 'og:image:url'] as $tag) {
-      $this->assertEquals('http://' . $this->getHostname() . '/test-assets/path/to/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get($tag), 'https://www.hel.fi/path/to/og-image.png'));
+      $this->assertEquals('http://' . $this->getHostname() . '/test-assets/themes/hdbt/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get($tag), 'https://www.hel.fi/themes/hdbt/og-image.png'));
     }
-    $this->assertEquals('http://' . $this->getHostname() . '/test-assets/path/to/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get('twitter:image'), 'https://www.hel.fi/path/to/og-image.png'));
+    $this->assertEquals('http://' . $this->getHostname() . '/test-assets/themes/hdbt/og-image.png', $this->proxyManager()->getAttributeValue($request, Selectors::get('twitter:image'), 'https://www.hel.fi/themes/hdbt/og-image.png'));
   }
 
   /**
@@ -217,7 +217,7 @@ public function testSourceSrcSet() : void {
 
     foreach ($values as $value) {
       $this->assertEquals(
-        '/test-assets' . $value,
+        '//' . $this->getHostname() . $value,
         $this->proxyManager()->getAttributeValue($request, Selectors::get('source'), $value)
       );
     }