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

andrerom · 2018-09-14T04:19:47Z

Question	Answer
JIRA issue	EZP-29613
New feature	yes
Target version	`master`
BC breaks	no
Tests pass	yes
Doc needed	yes

Like in #2328 where Location->getContent() was added, this is about adding Content->getContentType() to avoid having to deal with loading logic and also furthermore to avoid repeated loads of content types in the system when we already have the Type (as we need it to build Content anyway) and can thus pass it around on Content Items.

This breaks the internal Repository\Helper\DomainMapper->buildContentDomainObject() a bit in that it now requires ContentType to be passed instead of implicit loading it for you from SPI if not provided. This is done to make us more aware of the loading going on and take advantage to optimize (done several places in this PR).

This implicit involves improving performance:

No repeated content types loading when calling field functions in Twig, as they can now use this
- No implicit repeated loads for same language when having to build these content types
Add possibility to bulk load ContentTypes in SPI in order to handle this even more efficient when bulk loading content (when bulk load Content and on Location/Content Search results)

Reduced cache lookups just with this on clean install: 84/184 => 44/139
Heavy use of twig field functions will see more noticeable gains, and there might be cases in other bundles (PageBuilder?) where we can now optimize and take advantage of getContent() and getContentType(). But for other cases re-introducing a safe form of inMemory cache will provide grater gains (For more context/info on that see JIRA issue).

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.
Take advantage in Admin UI: EZP-29613: As a developer I want access ContentType on Content to avoid re-loading it ezplatform-admin-ui#633

adamwojs · 2018-09-17T12:18:09Z

eZ/Publish/SPI/Persistence/Content/Type/Handler.php

+     * Missing items (NotFound) will be missing from the array and not cause an exception, it's up
+     * to calling logic to determine if this should cause exception or not.
+     *
+     * @param mixed $contentTypeIds


NItpick: Wrong type hint for $contentTypeIds param

adamwojs · 2018-09-17T12:19:24Z

eZ/Publish/Core/Repository/Helper/DomainMapper.php

@@ -99,24 +106,18 @@ public function __construct(
     * Builds a Content domain object from value object returned from persistence.
     *
     * @param \eZ\Publish\SPI\Persistence\Content $spiContent
-     * @param ContentType|SPIType $contentType
+     * @param ContentType $contentType


Nitpick: Please use FQCN

andrerom · 2018-09-17T14:36:37Z

eZ/Publish/Core/Persistence/Legacy/Content/Type/MemoryCachingHandler.php

+            if (isset($this->contentTypes[$contentTypeId])) {
+                $contentTypes[$contentTypeId] = $this->contentTypes[$contentTypeId];
+            } else {
+                $contentTypes[$contentTypeId] = $this->contentTypes[$contentTypeId] = $this->innerHandler->load($contentTypeId);


should improve this here and in loadGroups as otherwise it means singular load is always done.

…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 · 2018-09-23T15:02:30Z

Passing ✅

andrerom · 2018-10-17T18:19:41Z

With 7.3 branched, rebased and ready for final review @alongosz

Edit: Fixed merge conflict causing test failure.

alongosz

+1, but with nitpicks 😜

alongosz · 2018-11-14T22:28:42Z

eZ/Publish/Core/Persistence/Legacy/Content/Type/Gateway.php

+     *
+     * @return array Data rows.
+     */
+    abstract public function loadTypesDataList(array $typeIds): array;


Naming seems awkward to me, loadTypesListData makes more sense, but for Content we used loadContentList so I would stick to that convention and name it simply loadTypesList. However, since we have loadTypeData later on, I'm fine with both.

alongosz · 2018-11-14T22:28:54Z

eZ/Publish/Core/Persistence/Legacy/Content/Type/Gateway.php

@@ -185,6 +185,15 @@
     */
    abstract public function updateType($typeId, $status, UpdateStruct $updateStruct);

+    /**
+     * Loads an array with data about several Types in defined status..


Nitpick: double dot.

alongosz · 2018-11-14T22:41:56Z

eZ/Publish/Core/Persistence/Legacy/Content/Type/Gateway/DoctrineDatabase.php

+                'c',
+                'ezcontentclass_attribute',
+                'a',
+                'c.id = a.contentclass_id AND c.version = a.version'


Nitpick: please use $q->expr()->eq() and $q->expr()->andX().

I personally find that harder to read, but if there is consensus in engineering to rather use that, that's ok.

You never know what next database vendor will figure out for comparison and logic expressions ;)

If that vendor dismisses SQL standards more then current bunch of vendors do, then we have bigger problemas and it won't be supported anytime soon I think.

Besides, feels like you build your argument on the same assumption I did wrongly ;)
ref: doctrine/dbal#3302
"Doc + PHP Doc should clarify QueryBuilder features not supported across databases"

Aka QueryBuilder does not currently handle DB differences at all.

If that vendor dismisses SQL standards more then current bunch of vendors do, then we have bigger problemas and it won't be supported anytime soon I think.

True.

Besides, feels like you build your argument on the same assumption I did wrongly ;)
ref: doctrine/dbal#3302
"Doc + PHP Doc should clarify QueryBuilder features not supported across databases"

Aka QueryBuilder does not currently handle DB differences at all.

Well, it doesn't mean that it won't be supported in the future. Once it will, you'll end up with a lot of code to refactor to benefit from that. This is why I prefer using Expressions everywhere it's possible.

Pinging @adamwojs for POV on that ;)

Well, it doesn't mean that it won't be supported in the future.

I know, and personally I think that should be added. Maybe we should contribute it even as we rely on it and it would be helpful when starting to look into supporting oracle..

So I'm not against for more complex queries, including inserts and updates, but maybe we should do it for all.

Fixed in c89fb22 ;)

Now let's make sure we try to set aside some time to propose imporvments to DBAL beginning of next year.. ;)

eZ/Publish/Core/Repository/Helper/DomainMapper.php

@@ -85,12 +90,14 @@ public function __construct(
        ContentHandler $contentHandler,
        LocationHandler $locationHandler,
        TypeHandler $contentTypeHandler,
+        ContentTypeDomainMapper $contentTypeDomainMapper,


alongosz · 2018-11-14T23:09:08Z

eZ/Publish/Core/Search/Legacy/Tests/Content/AbstractTestCase.php

+    {
+        if (!self::$setup) {
+            parent::setUp();
+            $this->insertDatabaseFixture(__DIR__ . '/../_fixtures/full_dump.php');


Hmm, but then it means we're testing on SQLite only, since it is eZ\Publish\Core\Search\Legacy\Tests\Content namespace.

Update: ok... seems it was like that before in child class, so it's rather a follow up for later

alongosz · 2018-11-14T23:15:11Z

eZ/Publish/SPI/Persistence/Content/Type/Handler.php

+     * Missing items (NotFound) will be missing from the array and not cause an exception, it's up
+     * to calling logic to determine if this should cause exception or not.
+     *
+     * @param mixed[] $contentTypeIds


NItpick: Wrong type hint for $contentTypeIds param

This is still valid nitpick.

…d not single load()

…t for all loads for rtest coverage and consistency

…plication

…r new loadContentListByContentInfo()

…ssue

…dler

…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)._

micszo

Tested with ezsystems/ezplatform-admin-ui#633.

andrerom · 2018-12-04T12:30:20Z

@alongosz / @micszo Ok if I merge this and rebase UI PR so it can get final label?

…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 requested review from adamwojs and alongosz September 14, 2018 04:19

adamwojs approved these changes Sep 17, 2018

View reviewed changes

andrerom commented Sep 17, 2018

View reviewed changes

andrerom force-pushed the content_type_on_content branch from 474bdb7 to 649f103 Compare September 18, 2018 07:02

This comment has been minimized.

Sign in to view

andrerom force-pushed the content_type_on_content branch 2 times, most recently from 46849a6 to 98c1642 Compare September 18, 2018 11:44

This comment has been minimized.

Sign in to view

andrerom mentioned this pull request Sep 19, 2018

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

Merged

3 tasks

This comment has been minimized.

Sign in to view

andrerom force-pushed the content_type_on_content branch 4 times, most recently from 91c61cd to 042481c Compare September 23, 2018 08:27

andrerom mentioned this pull request Sep 24, 2018

[POC] EZP-29659: As a v2 User I want support for InMemory SPI cache #2453

Closed

4 tasks

andrerom force-pushed the content_type_on_content branch from 042481c to 3fa8c00 Compare October 17, 2018 18:15

andrerom closed this Oct 17, 2018

andrerom reopened this Oct 17, 2018

andrerom mentioned this pull request Oct 21, 2018

EZP-29734: As an Administrator I want to add Content Type limitation for Section/Assign policy ezsystems/ezplatform-admin-ui#696

Merged

2 tasks

andrerom requested a review from mikadamczyk November 5, 2018 10:13

alongosz approved these changes Nov 14, 2018

View reviewed changes

andrerom added 3 commits November 16, 2018 19:30

Inital simple addtion of ->getContentType API

4200ba5

Adapt FieldRenderingExtension to use Contnet->getCotnentType()

5f57d9b

Optimize FieldHelper and it's usage in ContentExtension

7e12146

andrerom added 13 commits November 16, 2018 19:30

Add unit test for Legacy/Content/Type/Handler::loadContentTypeList()

c44bacc

fix review comments

205a006

Change MemoryCachingHandler to correctly use loadContentTypeList() an…

cbda26e

…d not single load()

Change to use Doctrine QueryBuilder to fix loadTypesDataList(), use i…

5683a26

…t for all loads for rtest coverage and consistency

Fix Leagacy Search test to keep connection, also refactor to avoid du…

498b8df

…plication

Expose ContentTypeService->loadContentTypeList() in order to adapt fo…

1cf087a

…r new loadContentListByContentInfo()

Remove a not so relevant todo

cde64c5

CS + REST client fix

48fd929

Make sure MemoryCachingHandler cache is array type to avoid operand i…

e06fa6b

…ssue

Fix casting issue in Persistence/Legacy/Content/Type/MemoryCachingHan…

a3f8697

…dler

Change back to sort by content class identifier

6e197f7

Fix tests after merge issue

f9259df

Fix review comments

fa9ee61

andrerom force-pushed the content_type_on_content branch from e701b2b to fa9ee61 Compare November 16, 2018 18:39

Use QueryBuilder for where() conditions

c89fb22

alongosz approved these changes Nov 17, 2018

View reviewed changes

andrerom added the Ready for QA label Nov 18, 2018

micszo self-assigned this Nov 30, 2018

micszo approved these changes Dec 4, 2018

View reviewed changes

micszo added QA approved and removed Ready for QA labels Dec 4, 2018

micszo removed their assignment Dec 4, 2018

andrerom merged commit 0dc4f63 into master Dec 4, 2018

andrerom deleted the content_type_on_content branch December 4, 2018 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andrerom commented Sep 14, 2018 •

edited

adamwojs Sep 17, 2018

adamwojs Sep 17, 2018

andrerom Sep 17, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

andrerom commented Sep 23, 2018 •

edited

andrerom commented Oct 17, 2018 •

edited

alongosz left a comment

alongosz Nov 14, 2018

alongosz Nov 14, 2018

alongosz Nov 14, 2018

andrerom Nov 15, 2018

alongosz Nov 15, 2018

andrerom Nov 15, 2018 •

edited

alongosz Nov 16, 2018

andrerom Nov 16, 2018 •

edited

andrerom Nov 17, 2018

This comment was marked as resolved.

This comment was marked as resolved.

alongosz Nov 14, 2018

alongosz Nov 14, 2018

micszo left a comment

andrerom commented Dec 4, 2018

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

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

Conversation

andrerom commented Sep 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

andrerom commented Sep 23, 2018 • edited

andrerom commented Oct 17, 2018 • edited

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Nov 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Nov 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micszo left a comment

Choose a reason for hiding this comment

andrerom commented Dec 4, 2018

andrerom commented Sep 14, 2018 •

edited

andrerom commented Sep 23, 2018 •

edited

andrerom commented Oct 17, 2018 •

edited

andrerom Nov 15, 2018 •

edited

andrerom Nov 16, 2018 •

edited