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-29041: Content still visible in ezobjectrelationlist after sending it to trash #2515

Merged
merged 1 commit into from Jan 24, 2019

Conversation

@mikadamczyk
Copy link
Contributor

mikadamczyk commented Jan 7, 2019

Question Answer
JIRA issue EZP-29041
Bug yes
New feature no
Target version 7.3
BC breaks no
Tests pass yes
Doc needed no

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@mikadamczyk mikadamczyk self-assigned this Jan 7, 2019

@mikadamczyk mikadamczyk requested review from adamwojs, alongosz and andrerom Jan 7, 2019

@mikadamczyk mikadamczyk force-pushed the mikadamczyk:EZP-29041 branch from 3f68029 to ea5dc1f Jan 7, 2019

$contentId = $trashedItem->contentId;
$reverseRelations = $this->persistenceHandler->contentHandler()->loadReverseRelations($contentId);
if (!empty($reverseRelations)) {

This comment has been minimized.

Copy link
@andrerom

andrerom Jan 7, 2019

Member

given it always returns array we can skip this if right?
But we should probably move it to invalidateTags() so we don't call that with empty tags array. Same below

This comment has been minimized.

Copy link
@mikadamczyk

mikadamczyk Jan 8, 2019

Author Contributor

Thanks @andrerom, it is fixed now

@mikadamczyk mikadamczyk force-pushed the mikadamczyk:EZP-29041 branch 2 times, most recently from 51371db to 788a4a9 Jan 7, 2019

$contentId = $trashedItem->contentId;
$reverseRelations = $this->persistenceHandler->contentHandler()->loadReverseRelations($contentId);
$tags[] = array_map(function (Relation $relation) {

This comment has been minimized.

Copy link
@adamwojs

adamwojs Jan 7, 2019

Member

You can avoid array_merge by replacing array_map with array_walk (or just inner loop).

@andrerom
Copy link
Member

andrerom left a comment

Besides comment by @adamwojs 👍

@alongosz
Copy link
Member

alongosz left a comment

@mikadamczyk once you're back, could you please check if changes done in #2520 could simplify logic here as well (probably not for loading relations, but maybe at least for finding items before calling trash action)

@mikadamczyk mikadamczyk force-pushed the mikadamczyk:EZP-29041 branch 2 times, most recently from 1fcd292 to fd4162d Jan 21, 2019

@mikadamczyk mikadamczyk force-pushed the mikadamczyk:EZP-29041 branch from fd4162d to 356ac65 Jan 21, 2019

@mikadamczyk

This comment has been minimized.

Copy link
Contributor Author

mikadamczyk commented Jan 21, 2019

@alongosz, we can not use the return value of emptyTrash method because, in the next step, we are not able to fetch the reverse relations of deleted content.

@alongosz
Copy link
Member

alongosz left a comment

Ok, then no other choice it seems :)
+1

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jan 21, 2019

Ok, then no other choice it seems :)

Well... From initial look we could add tags to allow us to clear based on that info now added in #2520, it's a matter of reverse relationships right?

So current item, needs to have tags for all it's relations, so that when they change they can clear reverse relations.

So basically in ContentHandler::getCacheTagsForContent() iterate relations and add tags for them like we do in HttpCache (relation-<content-id> tag in that case)

Side: Looking into reducing tags lookup in cache impl, so the performance downside of potentially adding many hundred relation tags should hopefully be gone in parallel for 2.5 release. (We currently have such an issue when there are hundreds of locations on a single content item, for relations such an issue will be more common)

@barbaragr barbaragr self-assigned this Jan 22, 2019

@barbaragr barbaragr added QA approved and removed Ready for QA labels Jan 24, 2019

@lserwatka lserwatka merged commit 77d0bb8 into ezsystems:7.3 Jan 24, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
ezrobot/phpcsfixer Code review by ezrobot
Details
@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Jan 24, 2019

You can merge it up.

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.