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

Update tag for preview cache #153

Merged
merged 3 commits into from Jul 31, 2018

Conversation

zerustech
Copy link
Contributor

@zerustech zerustech commented Jul 8, 2018

The admin preview function does not display the draft content that is
currently being edited, this is because the preview cache is not purged,
and the preview function always fetches content from the view cache,
refer to ezsystems/ezpublish-kernel#2396 for details.

To fix this issue, PersistenceCachePurger.php::contentVersion() has
been changed to invalidate cache with the new tags.

Question Answer
JIRA issue n/a
Bug yes
New feature no
Target version v2.0.0
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.

zerustech added a commit to zerustech/ezpublish-legacy that referenced this pull request Jul 9, 2018
The admin preview function does not work as expected: it always fetches
content from the view cache, so the draft content being edited never
shows up.

To fix this issue, view cache should be purged by triggering listeners
attached to `content/cache/translations`.

This PR is related to ezsystems/LegacyBridge#153

| Question           | Answer
| ------------------ | ------------------
| **JIRA issue**     | n/a
| **Bug**            | yes
| **New feature**    | no
| **Target version** | `2017.12`
| **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`).
- [x] Ask for Code Review.
@andrerom
Copy link
Contributor

andrerom commented Jul 19, 2018

Sorry for not giving input sooner, but your PR's made me aware we where lacking a way to invalidate versions properly.

So added that here: ezsystems/ezpublish-kernel#2396

It means you can instead just edit exisitng contentVersion() and change it's last line to:

         $this->cache->invalidateTags(["content-{$contentId}-version-{$versionNo}"]);

Which will now make sure to clear all occurrences of the cache for the specific version, not just the listing of versions.

Hope this makes sense to you to, it at least simplifies this a lot ;)

EDIT: You can now also drop this from line before $this->cache->deleteItem("ez-content-version-info-${contentId}-${versionNo}");.

The admin preview function does not display the draft content that is
currently being edited, this is because the preview cache is not purged,
and the preview function always fetches content from the view cache,
refer to ezsystems/ezpublish-kernel#2396 for details.

To fix this issue, `PersistenceCachePurger.php::contentVersion()` has
been changed to invalidate cache with the new tags.

| Question           | Answer
| ------------------ | ------------------
| **JIRA issue**     | n/a
| **Bug**            | yes
| **New feature**    | no
| **Target version** | `v2.0.0`
| **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`).
- [x] Ask for Code Review.
@zerustech zerustech changed the title Add method for purging preview cache Update tag for preview cache Jul 20, 2018
@zerustech
Copy link
Contributor Author

Hi @andrerom, I have revised the code accordingly. Thanks!

@@ -168,7 +168,7 @@ public function contentVersion($contentId, $versionNo)
}

$this->cache->deleteItem("ez-content-version-info-${contentId}-${versionNo}");
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also drop this one

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore that

Copy link
Contributor

@andrerom andrerom Jul 20, 2018

Choose a reason for hiding this comment

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

As we want to make sure this works also for <2.2.2, instead change the lines here to for instance:

// todo Once we can require 2.2.2+, simplify to just invalidate "content-{$contentId}-version-{$versionNo}"
$this->cache->deleteItem("ez-content-version-info-${contentId}-${versionNo}");
$this->cache->invalidateTags(["content-${contentId}-version-list", "content-{$contentId}-version-{$versionNo}"]);

Note that it re-adds content-${contentId}-version-list, for <2.2.2 releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrerom So I don't need do anything, do I?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not anymore at least ;)

@andrerom andrerom requested a review from emodric July 26, 2018 13:14
@andrerom andrerom merged commit 0bc476a into ezsystems:master Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants