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

Improved cache-invalidation for media/contact/account #3778

Conversation

wachterjohannes
Copy link
Member

@wachterjohannes wachterjohannes commented Feb 19, 2018

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets none
Related issues/PRs none
License MIT
Documentation PR none

What's in this PR?

This PR add the reference-store to media/contact/account content-types to improve cache-invalidation.

To Do

  • Invalidation
  • Tests

@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch 2 times, most recently from 337c76c to 71d6953 Compare February 21, 2018 15:32
@wachterjohannes wachterjohannes changed the title [WIP] Added reference-store to media/contact/account [WIP] Improved cache-invalidation for media/contact/account Feb 22, 2018
@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch 3 times, most recently from 78e0794 to 9778e92 Compare February 22, 2018 10:24
@wachterjohannes wachterjohannes changed the title [WIP] Improved cache-invalidation for media/contact/account Improved cache-invalidation for media/contact/account Feb 22, 2018
Copy link
Contributor

@danrot danrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you clarified with @chirimoya if we want that feature?

return [];
}

$ids = $this->converter->convertIdsToGroupedIds($value, ['a' => [], 'c' => []]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe introduce constants in this class for a and c?

if ($alias) {
$entity->getId()->willReturn(1);
$this->invalidationHandler->invalidateReference($alias, 1)->shouldBeCalled();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else {
    $this->invalidationhandler->invalidateReference(Argument::any)->shouldNotBeCalled();
}

?


foreach ($result as $item) {
if ($item->getMediaId()) {
$mediaReferenceStore->add($item->getMediaId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't also be other stores considered here? AFAIK there could also e.g. pages returned here, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already done in preResolve method

/**
* Invalidate references when account/contact are persisted.
*/
class CacheInvalidationListener
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we have one of these listeners, instead of adding one in each bundle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in which bundle would you add that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpCacheBundle?

@danrot danrot self-assigned this Feb 23, 2018
@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch from 9778e92 to 09fb6a3 Compare February 23, 2018 14:20
@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch from 09fb6a3 to f588be9 Compare February 26, 2018 07:14
@wachterjohannes
Copy link
Member Author

@danrot this should go green now

@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch from f588be9 to f27debe Compare February 26, 2018 08:07
@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch 3 times, most recently from f6355c4 to 4b8f184 Compare February 26, 2018 12:06
Copy link
Contributor

@danrot danrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating e.g. a new contact or deleting an existing contact with a category does clear the cache as expected

@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch from 4b8f184 to f18b17f Compare February 26, 2018 12:24
Copy link
Contributor

@danrot danrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing e.g. a contact is still not working for me.

@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch from f18b17f to 320702b Compare February 26, 2018 13:59
@wachterjohannes wachterjohannes force-pushed the enhancement/account-contact-media-reference-store branch from 320702b to 32879b6 Compare February 26, 2018 14:00
@danrot danrot mentioned this pull request Feb 26, 2018
@danrot danrot merged commit 9d6f146 into sulu:master Feb 26, 2018
@wachterjohannes wachterjohannes deleted the enhancement/account-contact-media-reference-store branch February 26, 2018 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants