Skip to content
This repository has been archived by the owner on Aug 18, 2024. It is now read-only.

Do not invalidate group content list cache tags when the group itself changes #509

Merged
merged 2 commits into from
May 17, 2019

Conversation

pfrenssen
Copy link
Contributor

This PR is resulting out of an exception which has been thrown in one of the tests in my project while using the latest 8.x-1.x branch of OG (ref. https://app.continuousphp.com/git-hub/ec-europa/joinup-dev/build/074b4465-414f-41b1-8cf1-4a186270c5e7/logs/e70792da-5a92-42da-be11-d58a857d1fd3).

In #499 we added an exception to detect cases where MembershipManager::getGroupIds() is being called on entities which are not groups, which is an invalid use case. It turns out we have a case in og_invalidate_group_content_cache_tags() where this happens.

The purpose of this function is to invalidate cached listings of group content whenever group content changes. When this function was originally written the idea was also to invalidate group content listings when the group itself changes. This was probably considered since it is common for a group content listing to include the group label itself (a typical example is the RecentGroupContentBlock).

Now this intended functionality seems to have been broken for a long time. There were some checks to see if the passed in entity is a group that has recently changed, but nothing is then done with this inside the function.

Now the reason this still works as expected is because we do not actually need to invalidate the listing's cache tags if the group itself changes. It is only needed when group content changes. If the listing would include any data from the group itself (such as the label), then the cache tags of the group will also be attached to the listing, and it will be invalidated automatically when the group is saved. For an example of this mechanism in action check EntityReferenceLabelFormatter::viewElements() which will attach the cache tags of the referenced group if this label is shown.

I have created a test that demonstrates the circumstances where the exception occurs, and some additional test coverage that asserts the content listings will only be invalidated when the group's cache tags are present.

… audiences change.

This also exposes an exception that was occurring when invalidating group
content list cache tags when updating a group entity.
…up itself changes.

If the group content list includes any data from the group itself then the
list will be tagged with the group cache tags and will be invalidated
automatically if the group is saved.
Copy link
Collaborator

@claudiu-cristea claudiu-cristea left a comment

Choose a reason for hiding this comment

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

The changes make perfectly sense. At the beginning it was missed the fact that showing the group on a group item bubble's up the group cache tags.

@claudiu-cristea
Copy link
Collaborator

However, I cannot merge this PR as I don't have permissions. Will kindly ask a maintainer to do it.

@pfrenssen
Copy link
Contributor Author

Thanks! I was hoping you would be the one to review this, since this is building on your earlier fix + test!

@pfrenssen pfrenssen merged commit 380756a into 8.x-1.x May 17, 2019
@pfrenssen pfrenssen deleted the cache-invalidation-exception branch May 17, 2019 15:03
@MPParsley
Copy link
Collaborator

Didn't have the time yet to provide feedback.

We also encountered the exception when saving a group so this PR makes sense (and fixed our issue).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants