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-29899: Content loading can end up loading wrong version under concurrency #2502

Merged
merged 4 commits into from Dec 20, 2018

Conversation

4 participants
@andrerom
Copy link
Member

andrerom commented Dec 14, 2018

Question Answer
JIRA issue EZP-29899
Bug yes
Target version 5.4 and up
BC breaks no
Tests pass yes
Doc needed no

Under concurrency it's possible that current version number we get in content info
is out of date by the time we ask for full content object. So change
SPI to allow loading current version number directly.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.
  • On MERGE remember to adapt 6.13 and up with:
diff --git a/eZ/Publish/Core/Persistence/Cache/ContentHandler.php b/eZ/Publish/Core/Persistence/Cache/ContentHandler.php
index eccd98924d..f28bdc3c41 100644
--- a/eZ/Publish/Core/Persistence/Cache/ContentHandler.php
+++ b/eZ/Publish/Core/Persistence/Cache/ContentHandler.php
@@ -368,6 +373,7 @@ class ContentHandler extends AbstractHandler implements ContentHandlerInterface
             $languageCode
         );
         $this->cache->clear('content', $contentId, $versionNo);
+        $this->cache->clear('content', $contentId, self::PUBLISHED_VERSION);
         $this->cache->clear('content', 'info', $contentId, 'versioninfo', $versionNo);
 
         return $content;
diff --git a/eZ/Publish/Core/Repository/ContentService.php b/eZ/Publish/Core/Repository/ContentService.php
index e38d78d0f4..92575b2ee2 100644
--- a/eZ/Publish/Core/Repository/ContentService.php
+++ b/eZ/Publish/Core/Repository/ContentService.php
@@ -274,8 +274,3 @@ class ContentService implements ContentServiceInterface
         }
-
-         // As we have content info we can avoid that current version is looked up using spi in loadContent() if not set
-         if ($versionNo === null) {
-             $versionNo = $contentInfo->currentVersionNo;
-         }

        return $this->loadContent(

@andrerom andrerom changed the base branch from master to 6.7 Dec 14, 2018

EZP-29899: Content loading can end up loading wrong version under con…
…currency

Under concurrency it's possible that current version number we get in content info
is out of date by the time we ask for full content object. So change
SPI to allow loading current version number directly.

@andrerom andrerom force-pushed the EZP-29899 branch from f375e09 to 9a7ab69 Dec 14, 2018

@andrerom andrerom requested review from alongosz, adamwojs and kmadejski Dec 14, 2018

@alongosz
Copy link
Member

alongosz left a comment

Makes perfect sense, some nitpicks:

return $statement->fetchAll(\PDO::FETCH_ASSOC);
return $list[0];*/

This comment has been minimized.

Copy link
@alongosz

alongosz Dec 17, 2018

Member

Dead code detected 😛

This comment has been minimized.

Copy link
@andrerom

andrerom Dec 17, 2018

Author Member

Fixed :)

Show resolved Hide resolved eZ/Publish/Core/Persistence/Legacy/Content/Gateway.php Outdated
Show resolved Hide resolved eZ/Publish/SPI/Persistence/Content/Handler.php Outdated

alongosz and others added some commits Dec 17, 2018

Update eZ/Publish/Core/Persistence/Legacy/Content/Gateway.php
Co-Authored-By: andrerom <andre.romcke@gmail.com>
Update eZ/Publish/SPI/Persistence/Content/Handler.php
Co-Authored-By: andrerom <andre.romcke@gmail.com>
@m-tyrala
Copy link

m-tyrala left a comment

Sanity tests passed

@m-tyrala m-tyrala added QA approved and removed Ready for QA labels Dec 20, 2018

@andrerom andrerom merged commit 9d44b53 into 6.7 Dec 20, 2018

3 checks passed

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

@andrerom andrerom deleted the EZP-29899 branch Dec 20, 2018

andrerom added a commit to andrerom/ezpublish-kernel that referenced this pull request Jan 3, 2019

EZP-29899: Content loading can end up loading wrong version under con…
…currency (ezsystems#2502)

* EZP-29899: Content loading can end up loading wrong version under concurrency

Under concurrency it's possible that current version number we get in content info
is out of date by the time we ask for full content object. So change
SPI to allow loading current version number directly.

(cherry picked from commit 9d44b53)

kmadejski pushed a commit that referenced this pull request Feb 15, 2019

EZP-29899: Content loading can end up loading wrong version under con…
…currency (#2502)

* EZP-29899: Content loading can end up loading wrong version under concurrency

Under concurrency it's possible that current version number we get in content info
is out of date by the time we ask for full content object. So change
SPI to allow loading current version number directly.

(cherry picked from commit 9d44b53)
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.