Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-6312: Fixed ParentContentType View Matcher for not available parent #438

5 changes: 5 additions & 0 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -11315,6 +11315,11 @@ parameters:
count: 1
path: src/lib/MVC/Symfony/ConfigDumperInterface.php

-
message: "#^Cannot call method warning\\(\\) on Psr\\\\Log\\\\LoggerInterface\\|null\\.$#"
count: 3
path: src/lib/MVC/Symfony/Controller/Content/PreviewController.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue that needs to be resolved, not added to the baseline, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz : Adam said I could solve it this way : #438 (comment)

How to do you want it fixed then ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vidarl ok, but then see #438 (comment) about how to handle expected "unresolvable" issues. Baseline is sort of "todo" list. On main we could tackle it with ?-> operator. Here we would need to use if or ||, so that might become unreadable indeed.

btw. for that reason I'm not a huge fan of traits, especially that one coming from PSR. They're meant to be very flexible which leads to such trivial issues.

But the solution could be as Steve described in the mentioned comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I had missed @Steveb-p comment in #438 (comment)

Moved error to phpstan.neon.dist in ad68fc7
Better ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved error to phpstan.neon.dist in ad68fc7 Better ?

Almost 🙃
See: #438 (review)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz and now? ref 025414d

-
message: "#^Method Ibexa\\\\Core\\\\MVC\\\\Symfony\\\\Controller\\\\Content\\\\QueryController\\:\\:runPagingQuery\\(\\) has no return type specified\\.$#"
count: 1
Expand Down
2 changes: 2 additions & 0 deletions src/bundle/Core/Resources/config/services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ services:
$authorizationChecker: "@security.authorization_checker"
$locationProvider: '@Ibexa\Core\Helper\PreviewLocationProvider'
$controllerChecker: '@Ibexa\Core\MVC\Symfony\View\CustomLocationControllerChecker'
$debugMode: '%kernel.debug%'
$logger: '@logger'
tags:
- { name: controller.service_arguments }

Expand Down
74 changes: 62 additions & 12 deletions src/lib/MVC/Symfony/Controller/Content/PreviewController.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@

use Exception;
use Ibexa\Contracts\Core\Repository\ContentService;
use Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException as APINotFoundException;
use Ibexa\Contracts\Core\Repository\Exceptions\NotImplementedException;
use Ibexa\Contracts\Core\Repository\Exceptions\UnauthorizedException;
use Ibexa\Contracts\Core\Repository\LocationService;
use Ibexa\Contracts\Core\Repository\Values\Content\Content;
use Ibexa\Contracts\Core\Repository\Values\Content\Location;
use Ibexa\Core\Base\Exceptions\BadStateException;
use Ibexa\Core\Helper\ContentPreviewHelper;
use Ibexa\Core\Helper\PreviewLocationProvider;
use Ibexa\Core\MVC\Symfony\Routing\Generator\UrlAliasGenerator;
Expand All @@ -21,6 +23,9 @@
use Ibexa\Core\MVC\Symfony\SiteAccess;
use Ibexa\Core\MVC\Symfony\View\CustomLocationControllerChecker;
use Ibexa\Core\MVC\Symfony\View\ViewManagerInterface;
use Psr\Log\LoggerAwareTrait;
use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpKernelInterface;
Expand All @@ -29,6 +34,8 @@

class PreviewController
{
use LoggerAwareTrait;

public const PREVIEW_PARAMETER_NAME = 'isPreview';
public const CONTENT_VIEW_ROUTE = 'ibexa.content.view';

Expand All @@ -53,14 +60,19 @@ class PreviewController
/** @var \Ibexa\Core\MVC\Symfony\View\CustomLocationControllerChecker */
private $controllerChecker;

/** @var bool */
private $debugMode;
vidarl marked this conversation as resolved.
Show resolved Hide resolved

public function __construct(
ContentService $contentService,
LocationService $locationService,
HttpKernelInterface $kernel,
ContentPreviewHelper $previewHelper,
AuthorizationCheckerInterface $authorizationChecker,
PreviewLocationProvider $locationProvider,
CustomLocationControllerChecker $controllerChecker
CustomLocationControllerChecker $controllerChecker,
bool $debugMode = false,
?LoggerInterface $logger = null
) {
$this->contentService = $contentService;
$this->locationService = $locationService;
Expand All @@ -69,6 +81,8 @@ public function __construct(
$this->authorizationChecker = $authorizationChecker;
$this->locationProvider = $locationProvider;
$this->controllerChecker = $controllerChecker;
$this->debugMode = $debugMode;
$this->logger = $logger ?? new NullLogger();
}

/**
Expand Down Expand Up @@ -119,18 +133,19 @@ public function previewContentAction(
HttpKernelInterface::SUB_REQUEST,
false
);
} catch (\Exception $e) {
if ($location->isDraft() && $this->controllerChecker->usesCustomController($content, $location)) {
// @todo This should probably be an exception that embeds the original one
$message = <<<EOF
<p>The view that rendered this location draft uses a custom controller, and resulted in a fatal error.</p>
<p>Location View is deprecated, as it causes issues with preview, such as an empty location id when previewing the first version of a content.</p>
EOF;

throw new Exception($message, 0, $e);
} else {
throw $e;
} catch (APINotFoundException $e) {
$message = sprintf('Location (%s) not found or not available in requested language (%s)', $location->id, $language);
$this->logger->warning(
vidarl marked this conversation as resolved.
Show resolved Hide resolved
sprintf('%s %s', $message, 'when loading the preview page'),
['exception' => $e]
);
if ($this->debugMode) {
throw new BadStateException('Preview page', $message, $e);
}

return new Response($message);
} catch (Exception $e) {
return $this->buildResponseForGenericPreviewError($location, $content, $e);
}
$response->headers->addCacheControlDirective('no-cache', true);
$response->setPrivate();
Expand Down Expand Up @@ -187,6 +202,41 @@ protected function getForwardRequest(
$forwardRequestParameters
);
}

/**
* @throws \Ibexa\Core\Base\Exceptions\BadStateException
vidarl marked this conversation as resolved.
Show resolved Hide resolved
*/
private function buildResponseForGenericPreviewError(Location $location, Content $content, Exception $e): Response
{
$message = '';
try {
if ($location->isDraft() && $this->controllerChecker->usesCustomController($content, $location)) {
$message = <<<EOF
<p>The view that rendered this location draft uses a custom controller, and resulted in a fatal error.</p>
<p>Location View is deprecated, as it causes issues with preview, such as an empty location id when previewing the first version of a content.</p>
EOF;
}
} catch (Exception $innerException) {
$message = 'An exception occurred when handling page preview exception';
$this->logger->warning(
'Unable to check if location uses a custom controller when loading the preview page',
['exception' => $innerException]
);
}

$this->logger->warning('Unable to load the preview page', ['exception' => $e]);

$message .= <<<EOF
<p>Unable to load the preview page</p>
<p>See logs for more information</p>
EOF;

if ($this->debugMode) {
throw new BadStateException('Preview page', $message, $e);
}

return new Response($message);
}
}

class_alias(PreviewController::class, 'eZ\Publish\Core\MVC\Symfony\Controller\Content\PreviewController');
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
use Ibexa\Contracts\Core\Repository\LocationService;
use Ibexa\Contracts\Core\Repository\Values\Content\Content;
use Ibexa\Contracts\Core\Repository\Values\Content\Location;
use Ibexa\Core\Base\Exceptions\NotFoundException;
use Ibexa\Core\Base\Exceptions\UnauthorizedException;
use Ibexa\Core\Helper\ContentPreviewHelper;
use Ibexa\Core\Helper\PreviewLocationProvider;
use Ibexa\Core\MVC\Symfony\Controller\Content\PreviewController;
use Ibexa\Core\MVC\Symfony\SiteAccess;
use Ibexa\Core\MVC\Symfony\View\CustomLocationControllerChecker;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpKernelInterface;
Expand Down Expand Up @@ -51,6 +53,9 @@ final class PreviewControllerTest extends TestCase
/** @var \Ibexa\Core\MVC\Symfony\View\CustomLocationControllerChecker&\PHPUnit\Framework\MockObject\MockObject */
protected CustomLocationControllerChecker $controllerChecker;

/** @var \Psr\Log\LoggerInterface&\PHPUnit\Framework\MockObject\MockObject */
private LoggerInterface $logger;

protected function setUp(): void
{
parent::setUp();
Expand All @@ -62,6 +67,7 @@ protected function setUp(): void
$this->authorizationChecker = $this->createMock(AuthorizationCheckerInterface::class);
$this->locationProvider = $this->createMock(PreviewLocationProvider::class);
$this->controllerChecker = $this->createMock(CustomLocationControllerChecker::class);
$this->logger = $this->createMock(LoggerInterface::class);
}

protected function getPreviewController(): PreviewController
Expand All @@ -73,7 +79,9 @@ protected function getPreviewController(): PreviewController
$this->previewHelper,
$this->authorizationChecker,
$this->locationProvider,
$this->controllerChecker
$this->controllerChecker,
false,
$this->logger
);
}

Expand Down Expand Up @@ -128,6 +136,42 @@ public function testPreviewCanUserFail(): void
$controller->previewContentAction(new Request(), $contentId, $versionNo, $lang, 'test');
}

public function testPreviewWithLogMessage(): void
{
$controller = $this->getPreviewController();
$contentId = 123;
$lang = 'eng-GB';
$versionNo = 3;
$content = $this->createMock(Content::class);

$location = $this->createMock(Location::class);
$location->method('__get')->with('id')->willReturn('42');

$siteAccess = $this->createMock(SiteAccess::class);
$this->locationProvider
->method('loadMainLocationByContent')
->with($content)
->willReturn($location)
;
$this->contentService
->method('loadContent')
->with($contentId, [$lang], $versionNo)
->willReturn($content)
;

$this->authorizationChecker->method('isGranted')->willReturn(true);
$siteAccess->name = 'test';
$this->previewHelper->method('getOriginalSiteAccess')->willReturn($siteAccess);
$this->httpKernel->method('handle')->willThrowException(new NotFoundException('Foo Property', 'foobar'));

$this->logger
->expects(self::once())
->method('warning')
->with('Location (42) not found or not available in requested language (eng-GB) when loading the preview page');

$controller->previewContentAction(new Request(), $contentId, $versionNo, $lang, 'test');
}

/**
* @return iterable<string, array{SiteAccess|null, int, string, int, int|null, string|null}>
*/
Expand Down
Loading