From 4e163800775e2821abf6b4a3b79ce8fa387c0a99 Mon Sep 17 00:00:00 2001
From: Ivo Valchev <Ivo.V.Valchev@gmail.com>
Date: Fri, 19 Jun 2020 11:04:11 +0200
Subject: [PATCH 1/2] Make canonical record URL consistent across routes

---
 src/Controller/Frontend/DetailController.php |  20 ++--
 src/Twig/ContentExtension.php                |  59 ++--------
 src/Utils/ContentHelper.php                  | 110 +++++++++++++++++++
 tests/php/Twig/ContentExtensionTestCase.php  |  42 ++++---
 4 files changed, 149 insertions(+), 82 deletions(-)

diff --git a/src/Controller/Frontend/DetailController.php b/src/Controller/Frontend/DetailController.php
index c9fd6bc53..ca6d5d355 100644
--- a/src/Controller/Frontend/DetailController.php
+++ b/src/Controller/Frontend/DetailController.php
@@ -10,6 +10,7 @@
 use Bolt\Enum\Statuses;
 use Bolt\Repository\ContentRepository;
 use Bolt\TemplateChooser;
+use Bolt\Utils\ContentHelper;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Symfony\Component\Routing\Annotation\Route;
@@ -22,10 +23,14 @@ class DetailController extends TwigAwareController implements FrontendZoneInterf
     /** @var ContentRepository */
     private $contentRepository;
 
-    public function __construct(TemplateChooser $templateChooser, ContentRepository $contentRepository)
+    /** @var ContentHelper */
+    private $contentHelper;
+
+    public function __construct(TemplateChooser $templateChooser, ContentRepository $contentRepository, ContentHelper $contentHelper)
     {
         $this->templateChooser = $templateChooser;
         $this->contentRepository = $contentRepository;
+        $this->contentHelper = $contentHelper;
     }
 
     /**
@@ -52,12 +57,7 @@ public function record($slugOrId, ?string $contentTypeSlug = null, bool $require
             $record = $this->contentRepository->findOneBySlug($slugOrId, $contentType);
         }
 
-        // Update the canonical, with the correct path
-        $this->canonical->setPath(null, [
-            'contentTypeSlug' => $record ? $record->getContentTypeSingularSlug() : null,
-            'slugOrId' => $record ? $record->getSlug() : null,
-            '_locale' => $this->request->getLocale(),
-        ]);
+        $this->contentHelper->setCanonicalPath($record);
 
         return $this->renderSingle($record, $requirePublished);
     }
@@ -67,11 +67,7 @@ public function contentByFieldValue(string $contentTypeSlug, string $field, stri
         $contentType = ContentType::factory($contentTypeSlug, $this->config->get('contenttypes'));
         $record = $this->contentRepository->findOneByFieldValue($field, $value, $contentType);
 
-        // Update the canonical, with the correct path
-        $this->canonical->setPath(null, [
-            'field' => $field,
-            'value' => $value,
-        ]);
+        $this->contentHelper->setCanonicalPath($record);
 
         return $this->renderSingle($record);
     }
diff --git a/src/Twig/ContentExtension.php b/src/Twig/ContentExtension.php
index 86f73bb52..cfe06266c 100644
--- a/src/Twig/ContentExtension.php
+++ b/src/Twig/ContentExtension.php
@@ -72,6 +72,9 @@ class ContentExtension extends AbstractExtension
     /** @var Canonical */
     private $canonical;
 
+    /** @var ContentHelper */
+    private $contentHelper;
+
     public function __construct(
         UrlGeneratorInterface $urlGenerator,
         ContentRepository $contentRepository,
@@ -82,7 +85,8 @@ public function __construct(
         Query $query,
         TaxonomyRepository $taxonomyRepository,
         TranslatorInterface $translator,
-        Canonical $canonical
+        Canonical $canonical,
+        ContentHelper $contentHelper
     ) {
         $this->urlGenerator = $urlGenerator;
         $this->contentRepository = $contentRepository;
@@ -94,6 +98,7 @@ public function __construct(
         $this->taxonomyRepository = $taxonomyRepository;
         $this->translator = $translator;
         $this->canonical = $canonical;
+        $this->contentHelper = $contentHelper;
     }
 
     /**
@@ -325,27 +330,13 @@ public function allowTwig(Environment $env, Content $content): void
         $content->setTwig($env);
     }
 
-    public function getLink(Content $content, bool $canonical = false, string $locale = ''): ?string
+    public function getLink(Content $content, bool $canonical = false, ?string $locale = null): ?string
     {
         if ($content->getId() === null || $content->getDefinition()->get('viewless')) {
             return null;
         }
 
-        if (empty($locale)) {
-            $locale = $this->request->getLocale();
-        }
-
-        if ($this->isHomepage($content)) {
-            return $this->generateLink('homepage_locale', ['_locale' => $locale], $canonical);
-        }
-
-        $params = [
-            '_locale' => $locale,
-            'slugOrId' => $content->getSlug() ?: $content->getId(),
-            'contentTypeSlug' => $content->getContentTypeSingularSlug(),
-        ];
-
-        return $this->generateLink($content->getDefinition()->get('record_route'), $params, $canonical);
+        return $this->contentHelper->getLink($content, $canonical, $locale);
     }
 
     public function getEditLink(Content $content): ?string
@@ -681,45 +672,17 @@ public function getSpecialFeature(Content $record): string
 
     public function isHomepage(Content $content): bool
     {
-        return $this->isSpecialpage($content, 'homepage');
+        return $this->contentHelper->isHomepage($content);
     }
 
     public function is404(Content $content): bool
     {
-        return $this->isSpecialpage($content, 'notfound');
+        return $this->contentHelper->is404($content);
     }
 
     public function isMaintenance(Content $content): bool
     {
-        return $this->isSpecialpage($content, 'maintenance');
-    }
-
-    private function isSpecialpage(Content $content, string $type): bool
-    {
-        $configSetting = $this->config->get('general/' . $type);
-
-        if (! is_iterable($configSetting)) {
-            $configSetting = (array) $configSetting;
-        }
-
-        foreach ($configSetting as $item) {
-            $item = explode('/', $item);
-
-            // Discard candidate if contentTypes don't match
-            if ($item[0] !== $content->getContentTypeSingularSlug() && $item[0] !== $content->getContentTypeSlug()) {
-                continue;
-            }
-
-            $idOrSlug = $item[1] ?? null;
-
-            // Success if we either have no id/slug for a Singleton, or if the id/slug matches
-            if ((empty($idOrSlug) && $content->getDefinition()->get('singleton')) ||
-                ($idOrSlug === $content->getSlug() || $idOrSlug === (string) $content->getId())) {
-                return true;
-            }
-        }
-
-        return false;
+        return $this->contentHelper->isMaintenance($content);
     }
 
     public function isHomepageListing(ContentType $contentType): bool
diff --git a/src/Utils/ContentHelper.php b/src/Utils/ContentHelper.php
index c81165ef1..5d6b802ce 100644
--- a/src/Utils/ContentHelper.php
+++ b/src/Utils/ContentHelper.php
@@ -4,11 +4,93 @@
 
 namespace Bolt\Utils;
 
+use Bolt\Canonical;
+use Bolt\Configuration\Config;
 use Bolt\Entity\Content;
 use Bolt\Entity\Field\Excerptable;
+use Symfony\Component\HttpFoundation\RequestStack;
 
 class ContentHelper
 {
+    /** @var Canonical */
+    private $canonical;
+
+    /** @var \Symfony\Component\HttpFoundation\Request|null */
+    private $request;
+
+    /** @var Config */
+    private $config;
+
+    public function __construct(Canonical $canonical, RequestStack $requestStack, Config $config)
+    {
+        $this->canonical = $canonical;
+        $this->request = $requestStack->getCurrentRequest();
+        $this->config = $config;
+    }
+
+    public function setCanonicalPath($record, ?string $locale = null): void
+    {
+        if (! $record instanceof Content) {
+            return;
+        }
+
+        $route = $this->getCanonicalRouteAndParams($record, $locale);
+
+        $this->canonical->setPath($route['route'], $route['params']);
+    }
+
+    public function getLink($record, bool $canonical = false, ?string $locale = null): ?string
+    {
+        $route = $this->getCanonicalRouteAndParams($record, $locale);
+
+        // Clone the canonical, as it is a shared service.
+        // We only want to get the url for the current request
+        $canonicalObj = clone $this->canonical;
+        $canonicalObj->setPath($route['route'], $route['params']);
+
+        return $canonical ? $canonicalObj->get() : $canonicalObj->getPath();
+    }
+
+    private function getCanonicalRouteAndParams(Content $record, ?string $locale = null): array
+    {
+        if ($this->isHomepage($record)) {
+            return [
+                'route' => 'homepage_locale',
+                'params' => [
+                    '_locale' => $locale,
+                ],
+            ];
+        }
+
+        if (! $locale) {
+            $locale = $this->request->getLocale();
+        }
+
+        return [
+            'route' => $record->getDefinition()->get('record_route'),
+            'params' => [
+                'contentTypeSlug' => $record->getContentTypeSingularSlug(),
+                'slugOrId' => $record->getSlug(),
+                '_locale' => $locale,
+            ],
+        ];
+    }
+
+    public function isHomepage(Content $content): bool
+    {
+        return $this->isSpecialpage($content, 'homepage');
+    }
+
+    public function is404(Content $content): bool
+    {
+        return $this->isSpecialpage($content, 'notfound');
+    }
+
+    public function isMaintenance(Content $content): bool
+    {
+        return $this->isSpecialpage($content, 'maintenance');
+    }
+
     public static function isSuitable(Content $record, string $which = 'title_format'): bool
     {
         $definition = $record->getDefinition();
@@ -116,4 +198,32 @@ public static function getFieldBasedTitle(Content $content, string $locale = '')
 
         return implode(' ', $titleParts);
     }
+
+    private function isSpecialpage(Content $content, string $type): bool
+    {
+        $configSetting = $this->config->get('general/' . $type);
+
+        if (! is_iterable($configSetting)) {
+            $configSetting = (array) $configSetting;
+        }
+
+        foreach ($configSetting as $item) {
+            $item = explode('/', $item);
+
+            // Discard candidate if contentTypes don't match
+            if ($item[0] !== $content->getContentTypeSingularSlug() && $item[0] !== $content->getContentTypeSlug()) {
+                continue;
+            }
+
+            $idOrSlug = $item[1] ?? null;
+
+            // Success if we either have no id/slug for a Singleton, or if the id/slug matches
+            if ((empty($idOrSlug) && $content->getDefinition()->get('singleton')) ||
+                ($idOrSlug === $content->getSlug() || $idOrSlug === (string) $content->getId())) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }
diff --git a/tests/php/Twig/ContentExtensionTestCase.php b/tests/php/Twig/ContentExtensionTestCase.php
index da75cdc50..f51081fc2 100644
--- a/tests/php/Twig/ContentExtensionTestCase.php
+++ b/tests/php/Twig/ContentExtensionTestCase.php
@@ -13,8 +13,8 @@
 use Bolt\Tests\DbAwareTestCase;
 use Bolt\Twig\ContentExtension;
 use Bolt\Twig\FieldExtension;
-use PHPUnit\Framework\MockObject\MockObject;
 use Doctrine\Common\Collections\ArrayCollection;
+use PHPUnit\Framework\MockObject\MockObject;
 use Twig\Environment;
 
 class ContentExtensionTestCase extends DbAwareTestCase
@@ -62,7 +62,7 @@ public function testTitle(): void
         $this->field->method('isTranslatable')
             ->willReturn(false);
         $this->field->method('__toString')
-            ->willReturn("Hey, this is a title");
+            ->willReturn('Hey, this is a title');
 
         $this->assertSame('(unknown): Hey, this is a title', $this->extension->getTitle($this->content, 'en'));
     }
@@ -70,8 +70,8 @@ public function testTitle(): void
     public function testTitleFields(): void
     {
         $this->definition->method('has')
-        ->withConsecutive(['title_format'])
-        ->willReturn(true);
+            ->withConsecutive(['title_format'])
+            ->willReturn(true);
         $this->definition->method('get')
             ->withConsecutive(['title_format'])
             ->willReturn('{number}: {title}');
@@ -94,7 +94,7 @@ public function testContentImage(): void
         $this->content->method('getFields')
             ->willReturn(new ArrayCollection([$field1, $field2, $imagefield, $field3]));
 
-        $this->assertSame(null, $this->extension->getImage($this->content));
+        $this->assertNull($this->extension->getImage($this->content));
 
         $imagefield->method('get')
             ->withConsecutive(['filename'])
@@ -113,7 +113,7 @@ public function testContentImageWithImagelist(): void
         $image2 = $this->createMock(ImageField::class);
         $imagelist = $this->createMock(ImagelistField::class);
         $field3 = $this->createMock(Field::class);
-                $imagelist->method('getValue')
+        $imagelist->method('getValue')
             ->willReturn([$image1, $image2]);
         $this->content->method('getFields')
             ->willReturn(new ArrayCollection([$field1, $field2, $imagelist, $field3]));
@@ -139,7 +139,7 @@ public function testExceptFromFormatShort(): void
         $field1 = $this->createMock(Field::class);
         $field2 = $this->createMock(Field::class);
         $field1->method('__toString')->willReturn("In this week's news");
-        $field2->method('__toString')->willReturn("Bolt 4 is pretty awesome.");
+        $field2->method('__toString')->willReturn('Bolt 4 is pretty awesome.');
         $this->content->method('getField')
             ->withConsecutive(['subheading'], ['body'])
             ->willReturnOnConsecutiveCalls($field1, $field2);
@@ -150,7 +150,6 @@ public function testExceptFromFormatShort(): void
             ->willReturn(1);
 
         $this->assertSame("In this week's ne…", $this->extension->getExcerpt($this->content, 18));
-
     }
 
     public function testExceptFromFormatFull(): void
@@ -166,7 +165,7 @@ public function testExceptFromFormatFull(): void
         $field1 = $this->createMock(Field::class);
         $field2 = $this->createMock(Field::class);
         $field1->method('__toString')->willReturn("In this week's news");
-        $field2->method('__toString')->willReturn("Bolt 4 is pretty awesome.");
+        $field2->method('__toString')->willReturn('Bolt 4 is pretty awesome.');
         $this->content->method('getField')
             ->withConsecutive(['subheading'], ['body'])
             ->willReturnOnConsecutiveCalls($field1, $field2);
@@ -181,20 +180,19 @@ public function testExceptFromFormatFull(): void
 
     public function testExcerptNoFormat(): void
     {
-
         $title = $this->createConfiguredMock(TextField::class, [
-           'getName' => 'title',
-           '__toString' => 'This field should not be used'
+            'getName' => 'title',
+            '__toString' => 'This field should not be used',
         ]);
 
         $subheading = $this->createConfiguredMock(TextField::class, [
-           'getName' => 'subheading',
-           '__toString' => 'This subheading is OK.'
+            'getName' => 'subheading',
+            '__toString' => 'This subheading is OK.',
         ]);
 
         $body = $this->createConfiguredMock(TextField::class, [
-           'getName' => 'body',
-           '__toString' => 'Here is the long body. It is OK too.'
+            'getName' => 'body',
+            '__toString' => 'Here is the long body. It is OK too.',
         ]);
 
         $this->content->method('getFields')
@@ -207,20 +205,20 @@ public function testExcerptNoFormat(): void
     public function testIsCurrentGlobalTwig(): void
     {
         $envCurrent = $this->createConfiguredMock(Environment::class, [
-           'getGlobals' => ['record' => $this->content]
+            'getGlobals' => [
+                'record' => $this->content,
+            ],
         ]);
 
         $notCurrentContent = $this->createMock(Content::class);
 
         $envNotCurrent = $this->createConfiguredMock(Environment::class, [
-            'getGlobals' => ['record' => $notCurrentContent]
+            'getGlobals' => [
+                'record' => $notCurrentContent,
+            ],
         ]);
 
-
         $this->assertTrue($this->extension->isCurrent($envCurrent, $this->content));
         $this->assertFalse($this->extension->isCurrent($envNotCurrent, $this->content));
     }
-
-    
-
 }

From d96d9554695380dc3b5f15caa9d7738da1f5c3f6 Mon Sep 17 00:00:00 2001
From: Ivo Valchev <Ivo.V.Valchev@gmail.com>
Date: Fri, 19 Jun 2020 11:43:02 +0200
Subject: [PATCH 2/2] Don't break if route issues happen

---
 src/Canonical.php           | 24 ++++++++++++------------
 src/Utils/ContentHelper.php |  4 ++++
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/src/Canonical.php b/src/Canonical.php
index 7b310414d..099203443 100644
--- a/src/Canonical.php
+++ b/src/Canonical.php
@@ -159,15 +159,10 @@ public function setPath(?string $route = null, array $params = []): void
             $route = $this->request->attributes->get('_route');
         }
 
-        try {
-            $this->path = $this->generateLink($route, $params, false);
-        } catch (InvalidParameterException | MissingMandatoryParametersException $e) {
-            // Just use the current URL /shrug
-            $this->path = $this->request->getUri();
-        }
+        $this->path = $this->generateLink($route, $params, false);
     }
 
-    public function generateLink(string $route, array $params, $canonical = false): ?string
+    public function generateLink(?string $route, ?array $params, $canonical = false): ?string
     {
         if (isset($params['_locale']) && $params['_locale'] === $this->defaultLocale) {
             unset($params['_locale']);
@@ -181,10 +176,15 @@ public function generateLink(string $route, array $params, $canonical = false):
             }
         }
 
-        return $this->urlGenerator->generate(
-            $route,
-            $params,
-            $canonical ? UrlGeneratorInterface::ABSOLUTE_URL : UrlGeneratorInterface::ABSOLUTE_PATH
-        );
+        try {
+            return $this->urlGenerator->generate(
+                $route,
+                $params,
+                $canonical ? UrlGeneratorInterface::ABSOLUTE_URL : UrlGeneratorInterface::ABSOLUTE_PATH
+            );
+        } catch (InvalidParameterException | MissingMandatoryParametersException | RouteNotFoundException $e) {
+            // Just use the current URL /shrug
+            return$this->request->getUri();
+        }
     }
 }
diff --git a/src/Utils/ContentHelper.php b/src/Utils/ContentHelper.php
index 5d6b802ce..2bc08343c 100644
--- a/src/Utils/ContentHelper.php
+++ b/src/Utils/ContentHelper.php
@@ -41,6 +41,10 @@ public function setCanonicalPath($record, ?string $locale = null): void
 
     public function getLink($record, bool $canonical = false, ?string $locale = null): ?string
     {
+        if (! $record instanceof Content) {
+            return '';
+        }
+
         $route = $this->getCanonicalRouteAndParams($record, $locale);
 
         // Clone the canonical, as it is a shared service.