Merge pull request #4744 from mhsdesign/bugfix/90-fix-convertUrisImpl…

…ementation BUGFIX: Partially repair convert uris implementation according to some 8.3 tests
neos · Nov 17, 2023 · 23089d7 · 23089d7
2 parents 33a3c4b + 0ff614f
commit 23089d7
Show file tree

Hide file tree

Showing 5 changed files with 151 additions and 77 deletions.
diff --git a/Neos.Neos/Classes/FrontendRouting/NodeUriBuilder.php b/Neos.Neos/Classes/FrontendRouting/NodeUriBuilder.php
@@ -15,7 +15,6 @@
 namespace Neos\Neos\FrontendRouting;
 
 use GuzzleHttp\Psr7\Uri;
-use Neos\Neos\FrontendRouting\NodeAddress;
 use Neos\Flow\Http\Exception as HttpException;
 use Neos\Flow\Mvc\ActionRequest;
 use Neos\Flow\Mvc\Exception\NoMatchingRouteException;
@@ -60,13 +59,14 @@ public static function fromUriBuilder(UriBuilder $uriBuilder): static
      *
      * @param NodeAddress $nodeAddress
      * @return UriInterface
-     * @throws NoMatchingRouteException | MissingActionNameException | HttpException
+     * @throws NoMatchingRouteException if the node address does not exist
      */
     public function uriFor(NodeAddress $nodeAddress): UriInterface
     {
         if (!$nodeAddress->isInLiveWorkspace()) {
             return $this->previewUriFor($nodeAddress);
         }
+        /** @noinspection PhpUnhandledExceptionInspection */
         return new Uri($this->uriBuilder->uriFor('show', ['node' => $nodeAddress], 'Frontend\Node', 'Neos.Neos'));
     }
 
@@ -76,7 +76,7 @@ public function uriFor(NodeAddress $nodeAddress): UriInterface
      *
      * @param NodeAddress $nodeAddress
      * @return UriInterface
-     * @throws NoMatchingRouteException | MissingActionNameException | HttpException
+     * @throws NoMatchingRouteException if the node address does not exist
      */
     public function previewUriFor(NodeAddress $nodeAddress): UriInterface
     {

diff --git a/Neos.Neos/Classes/Fusion/ConvertUrisImplementation.php b/Neos.Neos/Classes/Fusion/ConvertUrisImplementation.php
@@ -15,6 +15,8 @@
 namespace Neos\Neos\Fusion;
 
 use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
+use Neos\Flow\Log\Utility\LogEnvironment;
+use Neos\Flow\Mvc\Exception\NoMatchingRouteException;
 use Neos\Neos\FrontendRouting\NodeAddressFactory;
 use Neos\ContentRepository\Core\Projection\ContentGraph\Node;
 use Neos\ContentRepositoryRegistry\ContentRepositoryRegistry;
@@ -26,6 +28,7 @@
 use Neos\Media\Domain\Model\AssetInterface;
 use Neos\Media\Domain\Repository\AssetRepository;
 use Neos\Neos\Domain\Exception as NeosException;
+use Psr\Log\LoggerInterface;
 
 /**
  * A Fusion Object that converts link references in the format "<type>://<UUID>" to proper URIs
@@ -81,6 +84,12 @@ class ConvertUrisImplementation extends AbstractFusionObject
      */
     protected $contentRepositoryRegistry;
 
+    /**
+     * @Flow\Inject
+     * @var LoggerInterface
+     */
+    protected $systemLogger;
+
     /**
      * Convert URIs matching a supported scheme with generated URIs
      *
@@ -139,10 +148,11 @@ function (array $matches) use (&$unresolvedUris, $absolute, $nodeAddress) {
                         $uriBuilder = new UriBuilder();
                         $uriBuilder->setRequest($this->runtime->getControllerContext()->getRequest());
                         $uriBuilder->setCreateAbsoluteUri($absolute);
-
-                        // TODO: multi-site ....
-                        // -> different object than NodeAddress which also contains the CR Identifier.
-                        $resolvedUri = (string)NodeUriBuilder::fromUriBuilder($uriBuilder)->uriFor($nodeAddress);
+                        try {
+                            $resolvedUri = (string)NodeUriBuilder::fromUriBuilder($uriBuilder)->uriFor($nodeAddress);
+                        } catch (NoMatchingRouteException) {
+                            $this->systemLogger->info(sprintf('Could not resolve "%s" to a live node uri. The node was probably deleted.', $matches[0]), LogEnvironment::fromMethodName(__METHOD__));
+                        }
                         $this->runtime->addCacheTag('node', $matches[2]);
                         break;
                     case 'asset':
@@ -183,43 +193,52 @@ function (array $matches) use (&$unresolvedUris, $absolute, $nodeAddress) {
     /**
      * Replace the target attribute of link tags in processedContent with the target
      * specified by externalLinkTarget and resourceLinkTarget options.
-     * Additionally set rel="noopener" for links with target="_blank".
+     * Additionally sets rel="noopener" for links with target="_blank",
+     * and rel="noopener external" for external links.
      */
     protected function replaceLinkTargets(string $processedContent): string
     {
-        $noOpenerString = $this->fusionValue('setNoOpener') ? ' rel="noopener"' : '';
+        $setNoOpener = $this->fusionValue('setNoOpener');
+        $setExternal = $this->fusionValue('setExternal');
         $externalLinkTarget = trim($this->fusionValue('externalLinkTarget'));
         $resourceLinkTarget = trim($this->fusionValue('resourceLinkTarget'));
         if ($externalLinkTarget === '' && $resourceLinkTarget === '') {
             return $processedContent;
         }
         $controllerContext = $this->runtime->getControllerContext();
         $host = $controllerContext->getRequest()->getHttpRequest()->getUri()->getHost();
+        // todo optimize to only use one preg_replace_callback for uri converting
         $processedContent = preg_replace_callback(
             '~<a.*?href="(.*?)".*?>~i',
-            function ($matches) use ($externalLinkTarget, $resourceLinkTarget, $host, $noOpenerString) {
+            function ($matches) use ($externalLinkTarget, $resourceLinkTarget, $host, $setNoOpener, $setExternal) {
                 list($linkText, $linkHref) = $matches;
                 $uriHost = parse_url($linkHref, PHP_URL_HOST);
                 $target = null;
-                if ($externalLinkTarget !== '' && is_string($uriHost) && $uriHost !== $host) {
+                $isExternalLink = is_string($uriHost) && $uriHost !== $host;
+                if ($externalLinkTarget !== '' && $isExternalLink) {
                     $target = $externalLinkTarget;
                 }
-                if ($resourceLinkTarget !== '' && strpos($linkHref, '_Resources') !== false) {
+                if ($resourceLinkTarget !== '' && str_contains($linkHref, '_Resources')) {
                     $target = $resourceLinkTarget;
                 }
                 if ($target === null) {
                     return $linkText;
                 }
-                if (preg_match_all('~target="(.*?)~i', $linkText, $targetMatches)) {
+                // todo merge with "rel" attribute if already existent
+                $relValue = $isExternalLink && $setNoOpener ? 'noopener ' : '';
+                $relValue .= $isExternalLink && $setExternal ? 'external' : '';
+                $relValue = ltrim($relValue);
+                if (str_contains($linkText, 'target="')) {
+                    // todo shouldn't we merge the current target value
                     return preg_replace(
-                        '/target=".*?"/',
-                        sprintf('target="%s"%s', $target, $target === '_blank' ? $noOpenerString : ''),
+                        '/target="[^"]*"/',
+                        sprintf('target="%s"%s', $target, $relValue ? sprintf(' rel="%s"', $relValue) : $relValue),
                         $linkText
                     );
                 }
                 return str_replace(
                     '<a',
-                    sprintf('<a target="%s"%s', $target, $target === '_blank' ? $noOpenerString : ''),
+                    sprintf('<a target="%s"%s', $target, $relValue ? sprintf(' rel="%s"', $relValue) : $relValue),
                     $linkText
                 );
             },

diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/AssetTrait.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/AssetTrait.php
@@ -0,0 +1,49 @@
+<?php
+declare(strict_types=1);
+
+/*
+ * This file is part of the Neos.ContentRepository package.
+ *
+ * (c) Contributors of the Neos Project - www.neos.io
+ *
+ * This package is Open Source Software. For the full copyright and license
+ * information, please view the LICENSE file which was distributed with this
+ * source code.
+ */
+
+use Neos\Flow\Persistence\PersistenceManagerInterface;
+use Neos\Flow\ResourceManagement\ResourceManager;
+use Neos\Media\Domain\Model\Image;
+use Neos\Media\Domain\Repository\AssetRepository;
+use Neos\Utility\ObjectAccess;
+
+/**
+ * Step implementations for tests inside Neos.Neos
+ *
+* @internal only for behat tests within the Neos.Neos package
+*/
+trait AssetTrait
+{
+    /**
+     * @template T of object
+     * @param class-string<T> $className
+     *
+     * @return T
+     */
+    abstract private function getObject(string $className): object;
+
+    /**
+     * @Given an asset exists with id :assetId
+     */
+    public function anAssetExistsWithId(string $assetId): void
+    {
+        $resourceManager = $this->getObject(ResourceManager::class);
+        $resourceContent = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 111.42 125"><defs><style>.cls-1{fill:#010101;}</style></defs><title>neos_avatar_monochrome</title><g id="Ebene_2" data-name="Ebene 2"><g id="Layer_1" data-name="Layer 1"><path class="cls-1" d="M94.59,125H71.37L44.95,87.27v22.24L23.85,125H0V7.28L9.88,0H36.26L66.47,43V15.49L87.57,0h23.85V112.67ZM2.63,8.61V121.09l17.58-12.91V47.35l52.61,75.12h20.7l12.78-9.32H87.19L10,3.17ZM22.78,122.53l19.54-14.35V83.51L22.84,55.59v53.94l-17.72,13ZM12.85,2.63,88.68,110.68h20.11V2.63H89.32V80.16L34.89,2.63ZM69.11,46.79l17.58,25.1v-68L69.11,16.82Z"/></g></g></svg>';
+        $resource = $resourceManager->importResourceFromContent($resourceContent, 'test.svg');
+        $asset = new Image($resource);
+        ObjectAccess::setProperty($asset, 'Persistence_Object_Identifier', $assetId, true);
+
+        $this->getObject(AssetRepository::class)->add($asset);
+        $this->getObject(PersistenceManagerInterface::class)->persistAll();
+    }
+}
diff --git a/Neos.Neos/Tests/Behavior/Features/Bootstrap/FeatureContext.php b/Neos.Neos/Tests/Behavior/Features/Bootstrap/FeatureContext.php
@@ -38,6 +38,8 @@ class FeatureContext implements BehatContext
     use MigrationsTrait;
     use FusionTrait;
 
+    use AssetTrait;
+
     protected Environment $environment;
 
     protected ContentRepositoryRegistry $contentRepositoryRegistry;
@@ -51,6 +53,13 @@ public function __construct()
         $this->setupCRTestSuiteTrait(true);
     }
 
+    /*
+     * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+     *  Please don't add any generic step definitions here and use   *
+     *  a dedicated trait instead to keep this main class tidied up. *
+     * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
+     */
+
     /**
      * @BeforeScenario
      */

diff --git a/Neos.Neos/Tests/Behavior/Features/Fusion/ConvertUris.feature b/Neos.Neos/Tests/Behavior/Features/Fusion/ConvertUris.feature
@@ -83,21 +83,20 @@ Feature: Tests for the "Neos.Neos:ConvertUris" Fusion prototype
     Some value without URI
     """
 
-#  NOTE: This scenario currently breaks because it leads to an exception "Could not resolve a route and its corresponding URI for the given parameters"
-#  Scenario: URI to non-existing node
-#    When I execute the following Fusion code:
-#    """fusion
-#    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
-#    include: resource://Neos.Neos/Private/Fusion/Root.fusion
-#
-#    test = Neos.Neos:ConvertUris {
-#      value = 'Some value with node URI to non-existing node: node://non-existing.'
-#    }
-#    """
-#    Then I expect the following Fusion rendering result:
-#    """
-#    Some value with node URI to non-existing node: .
-#    """
+  Scenario: URI to non-existing node
+    When I execute the following Fusion code:
+    """fusion
+    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
+    include: resource://Neos.Neos/Private/Fusion/Root.fusion
+
+    test = Neos.Neos:ConvertUris {
+      value = 'Some value with node URI to non-existing node: node://non-existing.'
+    }
+    """
+    Then I expect the following Fusion rendering result:
+    """
+    Some value with node URI to non-existing node: .
+    """
 
   Scenario: URI to existing node
     When I execute the following Fusion code:
@@ -114,37 +113,35 @@ Feature: Tests for the "Neos.Neos:ConvertUris" Fusion prototype
     Some value with node URI: /a1.
     """
 
-#  NOTE: This scenario currently breaks because the rel attribute is just "noopener" instead of "noopener external"
-#  Scenario: Anchor tag without node or asset URI
-#    When I execute the following Fusion code:
-#    """fusion
-#    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
-#    include: resource://Neos.Neos/Private/Fusion/Root.fusion
-#
-#    test = Neos.Neos:ConvertUris {
-#      value = 'some <a href="https://neos.io">Link</a>'
-#    }
-#    """
-#    Then I expect the following Fusion rendering result:
-#    """
-#    some <a target="_blank" rel="noopener external" href="https://neos.io">Link</a>
-#    """
-
-#  NOTE: This scenario currently breaks because it leads to an exception "Could not resolve a route and its corresponding URI for the given parameters"
-#  Scenario: Anchor tag with node URI to non-existing node
-#    When I execute the following Fusion code:
-#    """fusion
-#    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
-#    include: resource://Neos.Neos/Private/Fusion/Root.fusion
-#
-#    test = Neos.Neos:ConvertUris {
-#      value = 'some <a href="node://non-existing">Link</a>'
-#    }
-#    """
-#    Then I expect the following Fusion rendering result:
-#    """
-#    some Link
-#    """
+  Scenario: Anchor tag without node or asset URI
+    When I execute the following Fusion code:
+    """fusion
+    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
+    include: resource://Neos.Neos/Private/Fusion/Root.fusion
+
+    test = Neos.Neos:ConvertUris {
+      value = 'some <a href="https://neos.io">Link</a>'
+    }
+    """
+    Then I expect the following Fusion rendering result:
+    """
+    some <a target="_blank" rel="noopener external" href="https://neos.io">Link</a>
+    """
+
+  Scenario: Anchor tag with node URI to non-existing node
+    When I execute the following Fusion code:
+    """fusion
+    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
+    include: resource://Neos.Neos/Private/Fusion/Root.fusion
+
+    test = Neos.Neos:ConvertUris {
+      value = 'some <a href="node://non-existing">Link</a>'
+    }
+    """
+    Then I expect the following Fusion rendering result:
+    """
+    some Link
+    """
 
   Scenario: Anchor tag with URI to existing node
     When I execute the following Fusion code:
@@ -176,18 +173,18 @@ Feature: Tests for the "Neos.Neos:ConvertUris" Fusion prototype
     Some value with node URI to non-existing asset: .
     """
 
-#  Scenario: URI to existing asset
-#    When an asset exists with id "362f3049-b9bb-454d-8769-6b35167e471e"
-#    And I execute the following Fusion code:
-#    """fusion
-#    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
-#    include: resource://Neos.Neos/Private/Fusion/Root.fusion
-#
-#    test = Neos.Neos:ConvertUris {
-#      value = 'Some value with node URI: asset://362f3049-b9bb-454d-8769-6b35167e471e.'
-#    }
-#    """
-#    Then I expect the following Fusion rendering result:
-#    """
-#    Some value with node URI: http://localhost/_Resources/Testing/Persistent/d0a1342bcb0e515bea83269427d8341d5f62a43d/test.svg.
-#    """
+  Scenario: URI to existing asset
+    When an asset exists with id "362f3049-b9bb-454d-8769-6b35167e471e"
+    And I execute the following Fusion code:
+    """fusion
+    include: resource://Neos.Fusion/Private/Fusion/Root.fusion
+    include: resource://Neos.Neos/Private/Fusion/Root.fusion
+
+    test = Neos.Neos:ConvertUris {
+      value = 'Some value with node URI: asset://362f3049-b9bb-454d-8769-6b35167e471e.'
+    }
+    """
+    Then I expect the following Fusion rendering result:
+    """
+    Some value with node URI: http://localhost/_Resources/Testing/Persistent/d0a1342bcb0e515bea83269427d8341d5f62a43d/test.svg.
+    """