IBX-6312: View matcher ParentContentType should not throw execption if parent is not available #383
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
|
@ezsystems/php-dev-team : review ping |
There was a problem hiding this comment.
Are you sure this actually resolves an issue?
While it will mitigate 404 error, wrong template is going to get matched because the matcher, which is expected to work for a location with such parent, will return false. So while editor would be able to load preview, he won't see the actual template he's supposed to see.
TBH to me we should just simply cover preview loading failure with a better exception:
https://github.com/ezsystems/ezplatform-kernel/blob/1.3/eZ/Publish/Core/MVC/Symfony/Controller/Content/PreviewController.php#L130
(something similar to what was done in the block above, for custom controller)
Given the nature of Page Builder, it should not be possible to work on a landing page which is not accessible on front-end.
However, if you really want to load that preview, matching the configured template, what you can do is to load location for all languages instead of the ones forced by SA-aware layer. Proxy for Lazy properties does this OOTB AFAIR (see below):
| $parentContentType = $this->repository->sudo( | ||
| static function (Repository $repository) use ($location) { | ||
| $parent = $repository->getLocationService()->loadLocation($location->parentLocationId); | ||
| try { | ||
| $parentContentType = $this->repository->sudo( | ||
| static function (Repository $repository) use ($location) { | ||
| $parent = $repository->getLocationService()->loadLocation($location->parentLocationId); | ||
|
|
||
| return $repository | ||
| ->getContentTypeService() | ||
| ->loadContentType($parent->getContentInfo()->contentTypeId); | ||
| } | ||
| ); | ||
| return $repository | ||
| ->getContentTypeService() | ||
| ->loadContentType($parent->getContentInfo()->contentTypeId); | ||
| } | ||
| ); | ||
| } catch (\eZ\Publish\API\Repository\Exceptions\NotFoundException $e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This code is a bit outdated. All the information can be lazy-loaded, so matchLocation could look like:
try {
$parentContentType = $this->repository->sudo(
static function () use ($location) {
$parent = $location->getParentLocation();
return $parent->getContentInfo()->getContentType();
}
);
return isset($this->values[$parentContentType->identifier]);
} catch (\Throwable $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
return false;
}Where logger is defined as follows:
use LoggerAwareTrait;
public function __construct(?LoggerAwareInterface $logger = null)
{
$this->logger = $logger ?? new NullLogger();
}Note that now for front-site root location / it will log a TypeError, because we have some issue when it comes to loading virtual (non-existent) content info for root location = 1. This probably needs to be resolved as well to avoid flooding logs for everyone who uses this matcher.
Alternatively we could use partially lazy-loaded properties, keeping "old-school" content type loading:
$parentContentType = $this->repository->sudo(
static function (Repository $repository) use ($location) {
$parent = $location->getParentLocation();
return $repository->getContentTypeService()->loadContentType(
$parent->getContentInfo()->contentTypeId
);
}
);However it still would be good to try-catch(Throwable) -> log, return false
| $parent = $this->loadParentLocation( | ||
| $view->getLocation()->parentLocationId | ||
| ); | ||
| } catch (\eZ\Publish\API\Repository\Exceptions\NotFoundException $e) { |
There was a problem hiding this comment.
rather \Throwable as well. NotFound should not occur here, unless someone explicitly used non-existent location id.
|
@alongosz, you are right that the "wrong" template will be used and that the user might be confused by that. (However, with current implementation, nothing is presented the user due to the exception) If I understand you correctly, you want me to instead catch all exceptions in Or you want me to incorporate your review comments for |
I would rather go for handling exceptions in POV ping @ezsystems/php-dev-team |
As for the rendering, it also depends on the first question. If it's a logic error, then we should provide an alternative render. As is the case with Page Builder functionality, we opt to use an event to allow application developers to take control and display a different view, with ourselves providing some default, debug one (controlled by debug state as to how much information should be presented). |
…f parent is not available
…cption if parent is not available" This reverts commit fd39333.
…f parent is not available
vidarl
force-pushed
the
ibx-6312_view_matcher_ParentContentType_when_parent_is_not_available
branch
from
db0db7e to
5267ac5
Compare
|
@Steveb-p : Difficult to answer your questions really.. It is a state that can happen if editors does something wrong basically ( but it is not a coding error ). If you have "Always available" unchecked, parent in [eng-GB] and child in [nor-NO], the child will be available in admin (because languages available in that siteaccess are ['eng-GB, 'nor-NO]. On site-nor siteaccess however, child is not available (because parent is not available in nor-NO. Preview controller will fail with uncough exception if ParentContentType matcher is used because it will try to fetch parent which is not available on site-nor siteaccess |
…ption if parent is not available - fixed test
|
@alongosz IMO, the exception handling got a bit messy though |
…ow execption if parent is not available - fixed test
|
There was a problem hiding this comment.
I've tried to reproduce this issue to get better understanding of what's really going on there, so I can give better feedback on how the system should behave. However I cannot reproduce it.
I've made landing page not always available, created /parent/polish_child structure BUT I've also configured admin to work with all needed languages (as everyone should) and that gives me no error when trying to load page via this matcher
My configuration is as follows:
ezplatform:
system:
admin_group:
languages: [ eng-GB, pol-PL ]
site_group:
languages: [ pol-PL ]
content_view:
full:
page_not_always_available:
template: '@ezdesign/full/landing_page_not_always_available.html.twig'
match:
Identifier\ParentContentType: [ landing_page ]
Are you sure that your setup contains
admin_group:
languages: [ eng-GB, pol-PL ]?
That's a hard requirement and system limitation - admin-ui needs to have explicitly configured all languages it operates on. Whatever we do to improve preview error handling, there's no way to load page which triggers previewAction controller without that configuration.
// Update: the above is not relevant since preview is executed in the context of site, not admin. My bad.
What am I missing?
I've just unchecked always available in landing_page content type, because the task doesn't specify details of the mentioned landing_page_not_always_available. I don't think that this should matter, but maybe I'm wrong.
Is it possible to have simpler steps involving Folder (always available) and/or Article (not always available) content types, so it's reproducible on Open Source? Previews in OSS are available when editing a page (there's Preview button).
Update: reproduced 👍
| @@ -110,6 +110,8 @@ services: | |||
| $authorizationChecker: '@security.authorization_checker' | |||
| $locationProvider: '@ezpublish.content_preview.location_provider' | |||
| $controllerChecker: '@ezpublish.view.custom_location_controller_checker' | |||
| $debugMode: '%kernel.debug%' | |||
| $logger: '@logger' | |||
| tags: | |||
| - { name: controller.service_arguments } | |||
There was a problem hiding this comment.
Nowadays we try to send logs to specific monolog channels. To do so please add:
| - { name: controller.service_arguments } | |
| - { name: controller.service_arguments } | |
| - { name: monolog.logger, channel: ibexa.controller } |
note that channel is arbitrary / gut feeling choice. I briefly discussed this with @Steveb-p and we've chosen here ibexa.controller.
The ticket explains how to reproduce it : https://issues.ibexa.co/browse/IBX-6312
So the goal of the PR is ( taken from ticket):
I hope this makes it clearer |
|
@vidarl can you please elaborate what is |
|
I have now updated description for IBX-6312: I am 99% sure I was able to reproduce the problem simply by editing a polish object, but now I do indeed have to add a polish translation on an existing one. By {{landing_page_not_always_available }} I mean a copy of the {{landing_page}} ContentType where the property "Make content available even with missing translations " is unchecked, ie disabled. |
There was a problem hiding this comment.
@vidarl after reproducing the issue and reviewing the behavior with and without the fix, I have some suggested changes that I've put into a separate PR #404. If you agree with them feel free to merge #404 into this one via squash so the Team can review the whole thing.
That refactoring mostly includes cognitive complexity improvements. I've kept the behavior of throwing an exception in debug mode, but that should be an API exception, not a generic one. I've chosen here BadStateException, since NotFound pre-formats message in a way which would look strange.
Foremost, the Customer needs to understand that it's not possible to load editable preview in his case as there's no site preview candidate and allowing an editor to edit the page could lead to unexpected results. Moreover it would suggest that the page is available, while on the front it would still be 404.
|
|
Okay, @alongosz . I have merged in your PR |
|
Closing in favor of ibexa/core#438 |
vidarl
deleted the
ibx-6312_view_matcher_ParentContentType_when_parent_is_not_available
branch
v3.3When matcher is checking parent's contenttype, it might throw NotFoundException if the parent is not available ( for instance if it is not available in any languages configured in the current siteaccess). I don't think the matcher should throw an exception in this case, just simply return false.
The change have no functional change on the front-end site access. User will anyway be given a 404 ( but the NotFound exception is no longer originating from the matcher). However, in admin-ui there is a big difference. A Landing page is not editable in admin-ui without the fix because the preview will be rendered with the front-end siteaccess scope and the matcher will then throw the exception
LocationService::loadLocation() might also throw {{UnauthorizedException}} exception but unsure if it could have security implications if catching that exception so leave that as-is
Checklist:
$ composer fix-cs).@ezsystems/engineering-team).