Skip to content

Commit

Permalink
EZP-29613: As a developer I want access ContentType on Content to avo…
Browse files Browse the repository at this point in the history
…id re-loading it

Uses ezsystems/ezpublish-kernel#2444, and $location->getContent()
(v2.2) in order avoid several repository loads not needed anymore.

Here are some numbers for cache lookups on clean install, expect difference to be bigger when you have more data (demo or project):

dashboard:
418 / 1240 (33.71%) _(master)_
468 / 1315 (35.59%) _(with kernel PR, only)_*
364 / 1043 (34.9%) _(with this PR + kernel PR)_

content structure:
478 / 1401 (34.12%)  _(master)_
450 / 1359 (33.11%) _(with kernel PR, only)_
436 / 1327 (32.86%) _(with this PR + kernel PR)_

_* So withouth this patch there is a slight regression with more cache loads with the kernel PR on dashboard.
Probably due to kernel now always loading content types from API instead of falling back to using SPI,
and via api language loading and mapping is needed. With this PR we get rid of the duplicated loads,
and for other apps the extra lanfguage load and much more will go away once we have inMemory cache again (v2.4)._
  • Loading branch information
andrerom committed Nov 19, 2018
1 parent 32505a0 commit f6830f3
Show file tree
Hide file tree
Showing 21 changed files with 110 additions and 183 deletions.
5 changes: 1 addition & 4 deletions src/bundle/Controller/ContentViewController.php
Expand Up @@ -208,10 +208,7 @@ private function supplyPathLocations(ContentView $view): void
*/
private function supplyContentType(ContentView $view): void
{
$content = $view->getContent();
$contentType = $this->contentTypeService->loadContentType($content->contentInfo->contentTypeId, $this->siteAccessLanguages);

$view->addParameters(['contentType' => $contentType]);
$view->addParameters(['contentType' => $view->getContent()->getContentType()]);
}

/**
Expand Down
14 changes: 3 additions & 11 deletions src/bundle/Controller/LocationController.php
Expand Up @@ -117,11 +117,7 @@ public function moveAction(Request $request): Response
$location = $data->getLocation();
$newParentLocation = $data->getNewParentLocation();

$newParentContentType = $this->contentTypeService->loadContentType(
$newParentLocation->getContentInfo()->contentTypeId
);

if (!$newParentContentType->isContainer) {
if (!$newParentLocation->getContent()->getContentType()->isContainer) {
throw new InvalidArgumentException(
'$newParentLocation',
'Cannot move location to a parent that is not a container'
Expand Down Expand Up @@ -173,11 +169,7 @@ public function copyAction(Request $request): Response
$location = $data->getLocation();
$newParentLocation = $data->getNewParentLocation();

$newParentContentType = $this->contentTypeService->loadContentType(
$newParentLocation->getContentInfo()->contentTypeId
);

if (!$newParentContentType->isContainer) {
if (!$newParentLocation->getContent()->getContentType()->isContainer) {
throw new InvalidArgumentException(
'$newParentLocation',
'Cannot copy location to a parent that is not a container'
Expand Down Expand Up @@ -283,7 +275,7 @@ public function swapAction(Request $request): Response
$newLocation = $data->getNewLocation();

$childCount = $this->locationService->getLocationChildCount($currentLocation);
$contentType = $this->contentTypeService->loadContentType($newLocation->getContentInfo()->contentTypeId);
$contentType = $newLocation->getContent()->getContentType();

if (!$contentType->isContainer && $childCount) {
throw new \InvalidArgumentException(
Expand Down
2 changes: 1 addition & 1 deletion src/bundle/Controller/SectionController.php
Expand Up @@ -244,7 +244,7 @@ public function viewSectionContentAction(Section $section, int $page = 1, int $l
$assignedContent[] = [
'id' => $content->id,
'name' => $content->getName(),
'type' => $this->contentTypeService->loadContentType($content->contentInfo->contentTypeId)->getName(),
'type' => $content->getContentType()->getName(),
'path' => $this->pathService->loadPathLocations(
$this->locationService->loadLocation($content->contentInfo->mainLocationId)
),
Expand Down
2 changes: 1 addition & 1 deletion src/bundle/Controller/TrashController.php
Expand Up @@ -145,7 +145,7 @@ public function listAction(Request $request): Response

/** @var \eZ\Publish\API\Repository\Values\Content\TrashItem $item */
foreach ($pagerfanta->getCurrentPageResults() as $item) {
$contentType = $this->contentTypeService->loadContentType($item->contentInfo->contentTypeId);
$contentType = $item->getContent()->getContentType();
$ancestors = $this->uiPathService->loadPathLocations($item);

$trashItemsList[] = new TrashItemData($item, $contentType, $ancestors);
Expand Down
Expand Up @@ -53,9 +53,8 @@ public function onFilterViewParameters(FilterViewParametersEvent $event)
}

$contentInfo = $view->getContent()->contentInfo;
$contentType = $this->contentTypeService->loadContentType(
$contentInfo->contentTypeId
);
$contentType = $view->getContent()->getContentType();

$event->getParameterBag()->add([
'form' => $view->getFormView(),
'location' => $view->getLocation(),
Expand Down
13 changes: 5 additions & 8 deletions src/lib/EventListener/RequestAttributesListener.php
Expand Up @@ -63,7 +63,7 @@ public function addRequestAttributes(FilterViewBuilderParametersEvent $event)
$parameterBag = $event->getParameters();

if ($parameterBag->has('locationId') && $request->get('_route') === '_ezpublishLocation') {
$location = $this->loadLocation($parameterBag->get('locationId'));
$location = $this->loadLocation($parameterBag->get('locationId'), $parameterBag->get('languageCode'));
$parameterBag->remove('locationId');
$parameterBag->set('location', $location);
}
Expand All @@ -72,10 +72,7 @@ public function addRequestAttributes(FilterViewBuilderParametersEvent $event)
/** @var Location $location */
$location = $parameterBag->get('location');

$languageCode = $parameterBag->get('languageCode') ?? $location->contentInfo->mainLanguageCode;

$content = $this->loadContent($location->contentInfo->id, $languageCode);
$parameterBag->set('content', $content);
$parameterBag->set('content', $location->getContent());
}
}

Expand All @@ -95,11 +92,11 @@ private function hasContentLanguage(Request $request, ParameterBag $parameterBag
*
* @return Location
*/
private function loadLocation($locationId): Location
private function loadLocation($locationId, ?string $languageCode): Location
{
$location = $this->repository->sudo(
function (Repository $repository) use ($locationId) {
return $repository->getLocationService()->loadLocation($locationId);
function (Repository $repository) use ($locationId, $languageCode) {
return $repository->getLocationService()->loadLocation($locationId, $languageCode ? [$languageCode] : null);
}
);

Expand Down
2 changes: 1 addition & 1 deletion src/lib/Menu/ContentCreateRightSidebarBuilder.php
Expand Up @@ -91,7 +91,7 @@ public function createStructure(array $options): ItemInterface
$parentLocation = $options['parentLocation'];
/** @var \eZ\Publish\API\Repository\Values\ContentType\ContentType $contentType */
$contentType = $options['content_type'];
$parentContentType = $this->contentTypeService->loadContentType($parentLocation->contentInfo->contentTypeId);
$parentContentType = $parentLocation->getContent()->getContentType();
/** @var \eZ\Publish\API\Repository\Values\Content\Language $language */
$language = $options['language'];
/** @var \Knp\Menu\ItemInterface|\Knp\Menu\ItemInterface[] $menu */
Expand Down
17 changes: 1 addition & 16 deletions src/lib/Specification/Location/IsContainer.php
Expand Up @@ -12,28 +12,13 @@

class IsContainer extends AbstractSpecification
{
/** @var \eZ\Publish\API\Repository\ContentTypeService */
private $contentTypeService;

/**
* @param \eZ\Publish\API\Repository\ContentTypeService $contentTypeService
*/
public function __construct($contentTypeService)
{
$this->contentTypeService = $contentTypeService;
}

/**
* @param \eZ\Publish\API\Repository\Values\Content\Location $item
*
* @return bool
*
* @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
*/
public function isSatisfiedBy($item): bool
{
return $this->contentTypeService->loadContentType(
$item->getContentInfo()->contentTypeId
)->isContainer;
return $item->getContent()->getContentType()->isContainer;
}
}
4 changes: 2 additions & 2 deletions src/lib/Tab/Dashboard/PagerContentToDataMapper.php
Expand Up @@ -51,7 +51,7 @@ public function map(Pagerfanta $pager): array
$data = [];

foreach ($pager as $content) {
$contentInfo = $this->contentService->loadContentInfo($content->id);
$contentInfo = $content->getVersionInfo()->getContentInfo();

$contributor = (new UserExists($this->userService))->isSatisfiedBy($contentInfo->ownerId)
? $this->userService->loadUser($contentInfo->ownerId) : null;
Expand All @@ -62,7 +62,7 @@ public function map(Pagerfanta $pager): array
'language' => $contentInfo->mainLanguageCode,
'contributor' => $contributor,
'version' => $content->versionInfo->versionNo,
'type' => $this->contentTypeService->loadContentType($contentInfo->contentTypeId)->getName(),
'type' => $content->getContentType()->getName(),
'modified' => $content->versionInfo->modificationDate,
'initialLanguageCode' => $content->versionInfo->initialLanguageCode,
'content_is_user' => (new ContentIsUser($this->userService))->isSatisfiedBy($content),
Expand Down
23 changes: 9 additions & 14 deletions src/lib/Tab/LocationView/RelationsTab.php
Expand Up @@ -106,22 +106,17 @@ public function renderView(array $parameters): string
{
/** @var Content $content */
$content = $parameters['content'];
$versionInfo = $content->getVersionInfo();
$relationsDataset = $this->datasetFactory->relations();
$relationsDataset->load($versionInfo);
$relationsDataset->load($content);

$contentTypes = [];
$contentTypeIds = [];

$relations = $relationsDataset->getRelations();

$viewParameters = [];

foreach ($relations as $relation) {
$contentTypeId = $relation->getDestinationContentInfo()->contentTypeId;

if (!isset($contentTypes[$contentTypeId])) {
$contentTypes[$contentTypeId] = $this->contentTypeService->loadContentType($contentTypeId);
}
$contentTypeIds[] = $relation->getDestinationContentInfo()->contentTypeId;
}

$viewParameters['relations'] = $relations;
Expand All @@ -130,17 +125,17 @@ public function renderView(array $parameters): string
$reverseRelations = $relationsDataset->getReverseRelations();

foreach ($reverseRelations as $relation) {
$contentTypeId = $relation->getSourceContentInfo()->contentTypeId;

if (!isset($contentTypes[$contentTypeId])) {
$contentTypes[$contentTypeId] = $this->contentTypeService->loadContentType($contentTypeId);
}
$contentTypeIds[] = $relation->getSourceContentInfo()->contentTypeId;
}

$viewParameters['reverse_relations'] = $reverseRelations;
}

$viewParameters['contentTypes'] = $contentTypes;
if (!empty($contentTypeIds)) {
$viewParameters['contentTypes'] = $this->contentTypeService->loadContentTypeList(array_unique($contentTypeIds));
} else {
$viewParameters['contentTypes'] = [];
}

return $this->twig->render(
'@ezdesign/content/tab/relations/tab.html.twig',
Expand Down
Expand Up @@ -8,8 +8,7 @@

namespace EzSystems\EzPlatformAdminUi\Tests\Validator\Constraint;

use eZ\Publish\API\Repository\ContentTypeService;
use eZ\Publish\API\Repository\Values\Content\ContentInfo;
use eZ\Publish\API\Repository\Values\Content\Content;
use eZ\Publish\API\Repository\Values\Content\Location;
use eZ\Publish\API\Repository\Values\ContentType\ContentType;
use EzSystems\EzPlatformAdminUi\Validator\Constraints\LocationIsContainer;
Expand All @@ -19,9 +18,6 @@

class LocationIsContainerValidatorTest extends TestCase
{
/** @var \eZ\Publish\API\Repository\ContentTypeService|\PHPUnit\Framework\MockObject\MockObject */
private $contentTypeService;

/** @var \Symfony\Component\Validator\Context\ExecutionContextInterface */
private $executionContext;

Expand All @@ -31,29 +27,35 @@ class LocationIsContainerValidatorTest extends TestCase
/** @var \eZ\Publish\API\Repository\Values\Content\Location|\PHPUnit\Framework\MockObject\MockObject */
private $location;

/** @var \eZ\Publish\API\Repository\Values\ContentType\ContentType|\PHPUnit\Framework\MockObject\MockObject */
private $contentType;

protected function setUp()
{
$this->contentTypeService = $this->createMock(ContentTypeService::class);
$this->executionContext = $this->createMock(ExecutionContextInterface::class);
$this->validator = new LocationIsContainerValidator($this->contentTypeService);
$this->validator = new LocationIsContainerValidator();
$this->validator->initialize($this->executionContext);

$content = $this->createMock(Content::class);

$this->location = $this->createMock(Location::class);
$this->location
->method('getContentInfo')
->willReturn(
$this->createMock(ContentInfo::class)
);
->method('getContent')
->willReturn($content);

$this->contentType = $this->createMock(ContentType::class);

$content
->method('getContentType')
->willReturn($this->contentType);
}

public function testValid()
{
$contentType = $this
->getMockBuilder(ContentType::class)
->setMethodsExcept(['__get'])
->setConstructorArgs([['isContainer' => true]])
->getMock();

$this->contentTypeService->method('loadContentType')->willReturn($contentType);
$this->contentType
->method('__get')
->with('isContainer')
->willReturn(true);

$this->executionContext
->expects($this->never())
Expand All @@ -64,13 +66,10 @@ public function testValid()

public function testInvalid()
{
$contentType = $this
->getMockBuilder(ContentType::class)
->setMethodsExcept(['__get'])
->setConstructorArgs([['isContainer' => false]])
->getMock();

$this->contentTypeService->method('loadContentType')->willReturn($contentType);
$this->contentType
->method('__get')
->with('isContainer')
->willReturn(false);

$this->executionContext
->expects($this->once())
Expand Down
7 changes: 1 addition & 6 deletions src/lib/UI/Config/Provider/User.php
Expand Up @@ -70,12 +70,7 @@ public function getConfig(): array
*/
private function resolveProfilePictureField(ApiUser $user): ?Field
{
try {
$contentType = $this->contentTypeService->loadContentType($user->contentInfo->contentTypeId);
} catch (\Exception $e) {
return null;
}

$contentType = $user->getContentType();
foreach ($user->getFields() as $field) {
$fieldDef = $contentType->getFieldDefinition($field->fieldDefIdentifier);

Expand Down
31 changes: 17 additions & 14 deletions src/lib/UI/Dataset/ContentDraftsDataset.php
Expand Up @@ -13,6 +13,7 @@
use eZ\Publish\API\Repository\Exceptions\UnauthorizedException;
use eZ\Publish\API\Repository\LocationService;
use eZ\Publish\API\Repository\Values\Content\VersionInfo;
use eZ\Publish\API\Repository\Values\ContentType\ContentType;
use eZ\Publish\API\Repository\Values\User\User;
use EzSystems\EzPlatformAdminUi\UI\Value\Content\VersionId;

Expand Down Expand Up @@ -60,16 +61,18 @@ public function load(User $user = null): self
}

$contentDrafts = array_filter($contentDrafts, function (VersionInfo $version) {
$contentInfo = $version->getContentInfo();
// Filter out content that has been sent to trash
return !$version->getContentInfo()->isTrashed();
});

if (null === $contentInfo->mainLocationId) {
$locations = $this->locationService->loadParentLocationsForDraftContent($version);
// empty locations here means Location has been trashed and Draft should be ignored
return !empty($locations);
}
$contentTypes = $contentTypeIds = [];
foreach ($contentDrafts as $contentDraft) {
$contentTypeIds[] = $contentDraft->getContentInfo()->contentTypeId;
}

return true;
});
if (!empty($contentTypeIds)) {
$contentTypes = $this->contentTypeService->loadContentTypeList(array_unique($contentTypeIds));
}

// ContentService::loadContentDrafts returns unsorted list of VersionInfo.
// Sort results by modification date, descending.
Expand All @@ -78,8 +81,11 @@ public function load(User $user = null): self
});

$this->data = array_map(
function (VersionInfo $versionInfo) {
return $this->mapContentDraft($versionInfo);
function (VersionInfo $versionInfo) use ($contentTypes) {
return $this->mapContentDraft(
$versionInfo,
$contentTypes[$versionInfo->getContentInfo()->contentTypeId]
);
},
$contentDrafts
);
Expand All @@ -102,12 +108,9 @@ public function getContentDrafts(): array
*
* @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
*/
private function mapContentDraft(VersionInfo $draft): array
private function mapContentDraft(VersionInfo $draft, ContentType $contentType): array
{
$contentInfo = $draft->getContentInfo();
$contentType = $this->contentTypeService->loadContentType(
$contentInfo->contentTypeId
);

return [
'id' => new VersionId(
Expand Down
4 changes: 2 additions & 2 deletions src/lib/UI/Dataset/PoliciesDataset.php
Expand Up @@ -84,8 +84,8 @@ public function __construct(
public function load(Location $location): self
{
$roleAssignments = [];
$content = $this->contentService->loadContentByContentInfo($location->contentInfo);
$contentType = $this->contentTypeService->loadContentType($content->contentInfo->contentTypeId);
$content = $location->getContent();
$contentType = $content->getContentType();

if ((new ContentTypeIsUser($this->userContentTypeIdentifier))->isSatisfiedBy($contentType)) {
$user = $this->userService->loadUser($content->id);
Expand Down

0 comments on commit f6830f3

Please sign in to comment.