[WIP] IBX-9379 Added Grace Period for archived versions #515
Conversation
src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
Outdated
Show resolved
Hide resolved
src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
Outdated
Show resolved
Hide resolved
src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
Outdated
Show resolved
Hide resolved
mateuszbieniek
changed the title
That's the thing - previous archived version is not known. There isn't any entry that indicates what it was, or at least we did not find one. |
|
In theory, we can (version with archived state and "highest" modification date timestamp - but that would be more like a "best guess"), but my test shows that in extreme situations, you can have more than one version change during a request. |
There was a problem hiding this comment.
Overall it seems to me as rather a workaround that should be resolved by better cache invalidation after transaction succeeds, but I don't have an actual better solution in that regard ATM (and I know it's not trivial) 😅, so I'm leaning towards accepting your solution, provided the remarks below are addressed.
- Integration tests coverage is necessary for those changes to be accepted into upstream.
- Other remarks in diff comments:
src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
Outdated
Show resolved
Hide resolved
| if ( | ||
| !$content->getVersionInfo()->isPublished() | ||
| && !$this->permissionResolver->canUser('content', 'versionread', $content) | ||
| ) { | ||
| throw new UnauthorizedException('content', 'versionread', ['contentId' => $contentId, 'versionNo' => $versionNo]); | ||
| if (!$this->isInGracePeriod($content, $this->settings['grace_period_in_seconds'], $versionNo)) { | ||
| throw new UnauthorizedException('content', 'versionread', ['contentId' => $contentId, 'versionNo' => $versionNo]); | ||
| } | ||
| } |
There was a problem hiding this comment.
- That check was not only to disallow viewing "unpublished" versions, but also other drafts, even for content that hasn't been published at all. While I see that your implementation of
isInGracePeriodchecks indeed for archived versions, you should probably check first if content has been archived. If it hasn't then that's a normal draft someone is working on before publishing it, so it should throw unauthorized exception right away if there's noversionreadpolicy. - Try combining that nested if-if into a single chained expression following code style of L389-390.
Ad. 2. It's really useful to review SonarCloud / SonarQube analysis. https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=515&id=ibexa_core&open=AZWkhRnq4uuA193_QeJw
or install SonarQube plugin for PHPStorm.
There was a problem hiding this comment.
Can you explain point 1 in more detail? I don't see a way that the grace period will grant access to a draft for a person that should be blocked by a versionread on the first hand.
There was a problem hiding this comment.
Can you explain point 1 in more detail? I don't see a way that the grace period will grant access to a draft for a person that should be blocked by a
versionreadon the first hand.
Essentially
|| ($content->getVersionInfo()->isArchived() && $this->isInGracePeriod())costs less, performance-wise, than isInGracePeriod alone, for the cases if we're dealing with draft, not an older published version.
Unless I'm missing something...
| @@ -129,6 +129,7 @@ public function __construct( | |||
| // Version archive limit (0-50), only enforced on publish, not on un-publish. | |||
| 'default_version_archive_limit' => 5, | |||
| 'remove_archived_versions_on_publish' => true, | |||
| 'grace_period_in_seconds' => 30, | |||
There was a problem hiding this comment.
For this to be "override-able" you need to:
There was a problem hiding this comment.
That's a plan, yes.
| @@ -491,9 +491,11 @@ public function testLoadContentUnauthorized() | |||
| $contentService->loadContent($contentId); | |||
| } | |||
|
|
|||
| public function testLoadContentNotPublishedStatusUnauthorized() | |||
| public function testLoadContentNotPublishedStatusUnauthorized(bool $expectException = true) | |||
There was a problem hiding this comment.
How exactly does this work? Seems this is always true, since there's no data provider or test dependency here.
There was a problem hiding this comment.
This is leftover from when I wanted to reuse this test case with a grace period enabled, please ignore it.
There was a problem hiding this comment.
This is leftover from when I wanted to reuse this test case with a grace period enabled, please ignore it.
ok, just make sure it's dropped in the final version. I'd rather move coverage of grace period to integration tests layer.
Co-authored-by: Andrew Longosz <[email protected]>
This issue is not related to cache at all, except that the outcome of this issue can mess it up. |
|
There was a problem hiding this comment.
Tricksy problem. I agree it looks like a workaround to a problem that should have some kind of better atomic solution, but if that isn't possible, then we'll have to go for it.
It better not be possible for the end user to influcence the length of the grace period. It looks ok, but please make sure they can't extend it years back. Also it should never work on trashed content, right?
Description:
This PR introduces a "grace period" that allows for archived versions to be accessed by the user, even when they don't have a
versionreadpolicy and the requested version was archived within X seconds from the execution of theloadContentmethod.The default grace period is 30 seconds (the same as probably the most popular response timeout for web servers).
The grace period now acts as a failsafe for situations when the published version number changes in the middle of the request (please check IBX-9379 for more info).
TODO: