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

EZP-29613: As a developer I want access ContentType on Content to avoid re-loading it #633

Merged
merged 2 commits into from Dec 4, 2018

Conversation

7 participants
@andrerom
Copy link
Member

commented Sep 19, 2018

Question Answer
Tickets EZP-29613
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

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

Besides avoiding executing unneeded PHP code, 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)

EDIT: These numbers are now outdated, newer versions of admin-ui adds new load calls for among others users

* So without 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 language load and much much more will go away once we have inMemory cache again (v2.4).

Checklist:

  • Coding standards ($ composer fix-cs)
  • Tests NOTE: There is a TMP commit here to get travis to test against kernel PR for it to pass
  • Ready for Code Review
@ezrobot

This comment was marked as resolved.

Copy link

commented Sep 19, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/src/lib/UI/Module/Subitems/ContentViewParameterSupplier.php b/src/lib/UI/Module/Subitems/ContentViewParameterSupplier.php
index 36758cf..84e9a59 100644
--- a/src/lib/UI/Module/Subitems/ContentViewParameterSupplier.php
+++ b/src/lib/UI/Module/Subitems/ContentViewParameterSupplier.php
@@ -11,7 +11,6 @@ namespace EzSystems\EzPlatformAdminUi\UI\Module\Subitems;
 use eZ\Publish\API\Repository\ContentService;
 use eZ\Publish\API\Repository\ContentTypeService;
 use eZ\Publish\API\Repository\LocationService;
-use eZ\Publish\API\Repository\Values\Content\ContentInfo;
 use eZ\Publish\API\Repository\Values\Content\Location;
 use eZ\Publish\API\Repository\Values\ContentType\ContentType;
 use eZ\Publish\Core\MVC\Symfony\View\ContentView;
diff --git a/src/lib/View/Builder/ContentTranslateViewBuilder.php b/src/lib/View/Builder/ContentTranslateViewBuilder.php
index b258298..97df0fc 100644
--- a/src/lib/View/Builder/ContentTranslateViewBuilder.php
+++ b/src/lib/View/Builder/ContentTranslateViewBuilder.php
@@ -12,7 +12,6 @@ use eZ\Publish\API\Repository\Repository;
 use eZ\Publish\API\Repository\Values\Content\Content;
 use eZ\Publish\API\Repository\Values\Content\Language;
 use eZ\Publish\API\Repository\Values\Content\Location;
-use eZ\Publish\API\Repository\Values\ContentType\ContentType;
 use eZ\Publish\Core\Base\Exceptions\InvalidArgumentException;
 use eZ\Publish\Core\MVC\Symfony\View\Builder\ViewBuilder;
 use eZ\Publish\Core\MVC\Symfony\View\Configurator;
@ezrobot

This comment was marked as resolved.

Copy link

commented Sep 19, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/src/lib/Tests/Validator/Constraint/LocationIsContainerValidatorTest.php b/src/lib/Tests/Validator/Constraint/LocationIsContainerValidatorTest.php
index 84192b2..13e3a19 100644
--- a/src/lib/Tests/Validator/Constraint/LocationIsContainerValidatorTest.php
+++ b/src/lib/Tests/Validator/Constraint/LocationIsContainerValidatorTest.php
@@ -8,7 +8,6 @@ declare(strict_types=1);
 
 namespace EzSystems\EzPlatformAdminUi\Tests\Validator\Constraint;
 
-use eZ\Publish\API\Repository\ContentTypeService;
 use eZ\Publish\API\Repository\Values\Content\Content;
 use eZ\Publish\API\Repository\Values\Content\Location;
 use eZ\Publish\API\Repository\Values\ContentType\ContentType;
@@ -44,7 +43,7 @@ class LocationIsContainerValidatorTest extends TestCase
             ->method('getContent')
             ->willReturn($content);
 
-        $this->contentType =  $this->createMock(ContentType::class);
+        $this->contentType = $this->createMock(ContentType::class);
 
         $content
             ->method('getContentType')
diff --git a/src/lib/Validator/Constraints/LocationIsContainerValidator.php b/src/lib/Validator/Constraints/LocationIsContainerValidator.php
index 3efedae..b965fce 100644
--- a/src/lib/Validator/Constraints/LocationIsContainerValidator.php
+++ b/src/lib/Validator/Constraints/LocationIsContainerValidator.php
@@ -8,8 +8,6 @@ declare(strict_types=1);
 
 namespace EzSystems\EzPlatformAdminUi\Validator\Constraints;
 
-use eZ\Publish\API\Repository\ContentTypeService;
-use eZ\Publish\API\Repository\Exceptions\NotFoundException;
 use EzSystems\EzPlatformAdminUi\Specification\Location\IsContainer;
 use Symfony\Component\Validator\Constraint;
 use Symfony\Component\Validator\ConstraintValidator;
$this->locationService->loadLocation($location->contentInfo->mainLocationId)
),
'contentType' => $location->getContent()->getContentType(),
'pathLocations' => $this->pathService->loadPathLocations($location),

This comment has been minimized.

Copy link
@andrerom

andrerom Sep 19, 2018

Author Member

Review note: I assumed we don't need to load main location but rather want path for the actual location bookmark is for.

@andrerom andrerom changed the title [WIP] EZP-29613: As a developer I want access ContentType on Content to avoid re-loading it EZP-29613: As a developer I want access ContentType on Content to avoid re-loading it Sep 19, 2018

@andrerom andrerom requested review from webhdx and ViniTou and removed request for webhdx Sep 19, 2018

andrerom added a commit to ezsystems/ezplatform that referenced this pull request Nov 18, 2018

@andrerom andrerom force-pushed the content_type_on_content branch from cdd412e to 186e874 Nov 18, 2018

@ezrobot

This comment was marked as resolved.

Copy link

commented Nov 18, 2018

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/src/lib/UniversalDiscovery/Event/Subscriber/SectionAssign.php b/src/lib/UniversalDiscovery/Event/Subscriber/SectionAssign.php
index 425e5cb..ecd1ebe 100644
--- a/src/lib/UniversalDiscovery/Event/Subscriber/SectionAssign.php
+++ b/src/lib/UniversalDiscovery/Event/Subscriber/SectionAssign.php
@@ -9,7 +9,6 @@ declare(strict_types=1);
 namespace EzSystems\EzPlatformAdminUi\UniversalDiscovery\Event\Subscriber;
 
 use eZ\Publish\API\Repository\ContentTypeService;
-use eZ\Publish\API\Repository\Exceptions\NotFoundException;
 use eZ\Publish\API\Repository\PermissionResolver;
 use eZ\Publish\API\Repository\Values\User\Limitation\ContentTypeLimitation;
 use EzSystems\EzPlatformAdminUi\UniversalDiscovery\Event\ConfigResolveEvent;
diff --git a/src/lib/View/Builder/ContentTranslateViewBuilder.php b/src/lib/View/Builder/ContentTranslateViewBuilder.php
index 4cc1ed0..880e142 100644
--- a/src/lib/View/Builder/ContentTranslateViewBuilder.php
+++ b/src/lib/View/Builder/ContentTranslateViewBuilder.php
@@ -248,6 +248,7 @@ class ContentTranslateViewBuilder implements ViewBuilder
      * @param array $parameters
      *
      * @return \eZ\Publish\API\Repository\Values\Content\Location|null
+     *
      * @param \eZ\Publish\API\Repository\Values\Content\Language|null $language
      *
      * @throws \eZ\Publish\API\Repository\Exceptions\UnauthorizedException

@andrerom andrerom force-pushed the content_type_on_content branch 2 times, most recently from 873853a to 3f4e516 Nov 18, 2018

}
$restrictedContentTypesIds = array_unique(array_merge(...$restrictedContentTypesIds));

This comment has been minimized.

Copy link
@andrerom

andrerom Nov 19, 2018

Author Member

Review Note: I didn't see why array_merge() was used here so removed it in the new code,

@andrerom

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Passing + kernel sent for QA. So this is ready for review now @ViniTou / @webhdx

@andrerom andrerom force-pushed the content_type_on_content branch from 3f4e516 to 52da71c Nov 26, 2018

@lserwatka

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Note for QA: we only need sanities here. ping @micszo

@micszo micszo self-assigned this Nov 30, 2018

@micszo
Copy link
Member

left a comment

  1. With this branch and the one in kernel on EE clean master.
  2. Log in as admin to backoffice.
  3. Go to Forms.
  4. Publish new Form (with Single line and Button).

Actual: Error 500 Type error: Argument 3 passed to EzSystems\EzPlatformFormBuilder\Pagination\Pagerfanta\SubmissionsAdapter::__construct() must be of the type string, null given, called in /var/www/ezplatform-v2-ee-c/vendor/ezsystems/ezplatform-form-builder/src/lib/Tab/LocationView/SubmissionsTab.php on line 172 [in vendor/ezsystems/ezplatform-form-builder/src/lib/Pagination/Pagerfanta/SubmissionsAdapter.php:31]

Does not occur on master w/o these branches.
Form is created but with error. Error occurs also when displaying existing form.

Is this maybe due to a missing rebase?

screenshot 2018-11-30 at 14 20 48

@andrerom andrerom force-pushed the content_type_on_content branch from 52da71c to 1389c30 Nov 30, 2018

@andrerom

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

@micszo That is due to usage of non API (internal) properties on content object:

$submissionsPagerfanta = new Pagerfanta(
    new SubmissionsAdapter($this->formSubmissionService, $content->contentInfo, $content->prioritizedFieldLanguageCode)
);

->prioritizedFieldLanguageCode is an internal property, and should not be used. As this PR instead injects a ContentProxy that property does not exists. Ideally this should have given exception in the first place.


Anyway, after seeing things like this I decided it's best to skip this part of the change here and just continue to load normal core content class, for now. It will result in the same amount of SPI lookup calls in this case, so no harm done.

Updated, rebased and ready for new QA round.

@alongosz
Copy link
Member

left a comment

@andrerom @micszo please remember that there's TMP commit which should be gone before final approval and merging ;)

@micszo

micszo approved these changes Dec 4, 2018

Copy link
Member

left a comment

No new issues during retests. 👍

@micszo micszo removed the Ready for QA label Dec 4, 2018

@micszo

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Not adding next label due to TMP commit!

@micszo micszo removed their assignment Dec 4, 2018

andrerom added some commits Sep 19, 2018

EZP-29613: As a developer I want access ContentType on Content to avo…
…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)._

@andrerom andrerom force-pushed the content_type_on_content branch from 1389c30 to eecc3de Dec 4, 2018

@micszo micszo added the QA approved label Dec 4, 2018

@andrerom andrerom merged commit 0e07bc2 into master Dec 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ezrobot/phpcsfixer Code review by ezrobot
Details

@andrerom andrerom deleted the content_type_on_content branch Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.