From faa86e06ba3032fdb98f3c0f79c72e8581d3c96f Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sun, 25 Sep 2016 11:21:13 +0200 Subject: [PATCH 1/4] Fix tags count in menu Move enable cache for Tag in the Entity because function `find*` should return result and not a Query --- .../Controller/WallabagRestController.php | 4 +-- .../CoreBundle/Controller/TagController.php | 26 +++++--------- .../CoreBundle/Repository/TagRepository.php | 34 +++++++++++++++++-- .../CoreBundle/Twig/WallabagExtension.php | 28 +++------------ 4 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/Wallabag/ApiBundle/Controller/WallabagRestController.php b/src/Wallabag/ApiBundle/Controller/WallabagRestController.php index fb7c6c1f1..dd17ef976 100644 --- a/src/Wallabag/ApiBundle/Controller/WallabagRestController.php +++ b/src/Wallabag/ApiBundle/Controller/WallabagRestController.php @@ -322,9 +322,7 @@ class WallabagRestController extends FOSRestController $tags = $this->getDoctrine() ->getRepository('WallabagCoreBundle:Tag') - ->findAllTags($this->getUser()->getId()) - ->getQuery() - ->getResult(); + ->findAllTagsWithEntries($this->getUser()->getId()); $json = $this->get('serializer')->serialize($tags, 'json'); diff --git a/src/Wallabag/CoreBundle/Controller/TagController.php b/src/Wallabag/CoreBundle/Controller/TagController.php index bc95a4d34..07cd3edb3 100644 --- a/src/Wallabag/CoreBundle/Controller/TagController.php +++ b/src/Wallabag/CoreBundle/Controller/TagController.php @@ -84,16 +84,11 @@ class TagController extends Controller { $tags = $this->getDoctrine() ->getRepository('WallabagCoreBundle:Tag') - ->findAllTags($this->getUser()->getId()) - ->getQuery() - ->getResult(); + ->findAllTagsWithEntries($this->getUser()->getId()); - return $this->render( - 'WallabagCoreBundle:Tag:tags.html.twig', - [ - 'tags' => $tags, - ] - ); + return $this->render('WallabagCoreBundle:Tag:tags.html.twig', [ + 'tags' => $tags, + ]); } /** @@ -127,13 +122,10 @@ class TagController extends Controller } } - return $this->render( - 'WallabagCoreBundle:Entry:entries.html.twig', - [ - 'form' => null, - 'entries' => $entries, - 'currentPage' => $page, - ] - ); + return $this->render('WallabagCoreBundle:Entry:entries.html.twig',[ + 'form' => null, + 'entries' => $entries, + 'currentPage' => $page, + ]); } } diff --git a/src/Wallabag/CoreBundle/Repository/TagRepository.php b/src/Wallabag/CoreBundle/Repository/TagRepository.php index 41f616079..f5c4ea6a0 100644 --- a/src/Wallabag/CoreBundle/Repository/TagRepository.php +++ b/src/Wallabag/CoreBundle/Repository/TagRepository.php @@ -7,17 +7,45 @@ use Doctrine\ORM\EntityRepository; class TagRepository extends EntityRepository { /** - * Find Tags. + * Find all tags per user. + * + * @param int $userId + * @param int $cacheLifeTime Duration of the cache for this query + * + * @return array + */ + public function findAllTags($userId, $cacheLifeTime = null) + { + $query = $this->createQueryBuilder('t') + ->select('t') + ->leftJoin('t.entries', 'e') + ->where('e.user = :userId')->setParameter('userId', $userId) + ->groupBy('t.slug') + ->getQuery(); + + if (null !== $cacheLifeTime) { + $query->useQueryCache(true); + $query->useResultCache(true); + $query->setResultCacheLifetime($cacheLifeTime); + } + + return $query->getArrayResult(); + } + + /** + * Find all tags with associated entries per user. * * @param int $userId * * @return array */ - public function findAllTags($userId) + public function findAllTagsWithEntries($userId) { return $this->createQueryBuilder('t') ->leftJoin('t.entries', 'e') - ->where('e.user = :userId')->setParameter('userId', $userId); + ->where('e.user = :userId')->setParameter('userId', $userId) + ->getQuery() + ->getResult(); } /** diff --git a/src/Wallabag/CoreBundle/Twig/WallabagExtension.php b/src/Wallabag/CoreBundle/Twig/WallabagExtension.php index 3780b13e7..0a6896c95 100644 --- a/src/Wallabag/CoreBundle/Twig/WallabagExtension.php +++ b/src/Wallabag/CoreBundle/Twig/WallabagExtension.php @@ -85,10 +85,11 @@ class WallabagExtension extends \Twig_Extension implements \Twig_Extension_Globa ->groupBy('e.id') ->getQuery(); - $data = $this->enableCache($query) - ->getArrayResult(); + $query->useQueryCache(true); + $query->useResultCache(true); + $query->setResultCacheLifetime($this->lifeTime); - return count($data); + return count($query->getArrayResult()); } /** @@ -104,30 +105,11 @@ class WallabagExtension extends \Twig_Extension implements \Twig_Extension_Globa return 0; } - $qb = $this->tagRepository->findAllTags($user->getId()); - - $data = $this->enableCache($qb->getQuery()) - ->getArrayResult(); + $data = $this->tagRepository->findAllTags($user->getId()); return count($data); } - /** - * Enable cache for a query. - * - * @param Query $query - * - * @return Query - */ - private function enableCache(Query $query) - { - $query->useQueryCache(true); - $query->useResultCache(true); - $query->setResultCacheLifetime($this->lifeTime); - - return $query; - } - public function getName() { return 'wallabag_extension'; From 401135852c6b25c8d5ab97beaefb02d1bd023ec9 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sun, 25 Sep 2016 11:26:15 +0200 Subject: [PATCH 2/4] Use scheduled entity insertions to avoid tag duplicate Using `getScheduledEntityInsertions()` we can retrieve not yet flushed but already persisted entities and then avoid tags duplication on import. --- .../CoreBundle/Helper/ContentProxy.php | 25 +++++++++++++++---- .../ImportBundle/Import/PocketImport.php | 3 ++- .../ImportBundle/Import/WallabagImport.php | 3 ++- .../CoreBundle/Helper/ContentProxyTest.php | 23 +++++++++++++++++ .../ImportBundle/Import/PocketImportTest.php | 14 +++++++++++ .../Import/WallabagV1ImportTest.php | 14 +++++++++++ .../Import/WallabagV2ImportTest.php | 14 +++++++++++ 7 files changed, 89 insertions(+), 7 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 5dd684f2d..a65a21e8e 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -96,13 +96,24 @@ class ContentProxy * * @param Entry $entry * @param array|string $tags An array of tag or a string coma separated of tag + * @param array $entitiesReady Entities from the EntityManager which are persisted but not yet flushed + * It is mostly to fix duplicate tag on import + * @see http://stackoverflow.com/a/7879164/569101 */ - public function assignTagsToEntry(Entry $entry, $tags) + public function assignTagsToEntry(Entry $entry, $tags, array $entitiesReady = []) { if (!is_array($tags)) { $tags = explode(',', $tags); } + // keeps only Tag entity from the "not yet flushed entities" + $tagsNotYetFlushed = []; + foreach ($entitiesReady as $entity) { + if ($entity instanceof Tag) { + $tagsNotYetFlushed[$entity->getLabel()] = $entity; + } + } + foreach ($tags as $label) { $label = trim($label); @@ -111,11 +122,15 @@ class ContentProxy continue; } - $tagEntity = $this->tagRepository->findOneByLabel($label); + if (isset($tagsNotYetFlushed[$label])) { + $tagEntity = $tagsNotYetFlushed[$label]; + } else { + $tagEntity = $this->tagRepository->findOneByLabel($label); - if (is_null($tagEntity)) { - $tagEntity = new Tag(); - $tagEntity->setLabel($label); + if (is_null($tagEntity)) { + $tagEntity = new Tag(); + $tagEntity->setLabel($label); + } } // only add the tag on the entry if the relation doesn't exist diff --git a/src/Wallabag/ImportBundle/Import/PocketImport.php b/src/Wallabag/ImportBundle/Import/PocketImport.php index e00eb44b3..327e25001 100644 --- a/src/Wallabag/ImportBundle/Import/PocketImport.php +++ b/src/Wallabag/ImportBundle/Import/PocketImport.php @@ -227,7 +227,8 @@ class PocketImport extends AbstractImport if (isset($importedEntry['tags']) && !empty($importedEntry['tags'])) { $this->contentProxy->assignTagsToEntry( $entry, - array_keys($importedEntry['tags']) + array_keys($importedEntry['tags']), + $this->em->getUnitOfWork()->getScheduledEntityInsertions() ); } diff --git a/src/Wallabag/ImportBundle/Import/WallabagImport.php b/src/Wallabag/ImportBundle/Import/WallabagImport.php index 043bb0a23..3754e4a95 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagImport.php +++ b/src/Wallabag/ImportBundle/Import/WallabagImport.php @@ -111,7 +111,8 @@ abstract class WallabagImport extends AbstractImport if (array_key_exists('tags', $data)) { $this->contentProxy->assignTagsToEntry( $entry, - $data['tags'] + $data['tags'], + $this->em->getUnitOfWork()->getScheduledEntityInsertions() ); } diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index 7abb07378..5d772602a 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -296,6 +296,29 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $this->assertEquals('tag2', $entry->getTags()[1]->getLabel()); } + public function testAssignTagsNotFlushed() + { + $graby = $this->getMockBuilder('Graby\Graby') + ->disableOriginalConstructor() + ->getMock(); + + $tagRepo = $this->getTagRepositoryMock(); + $tagRepo->expects($this->never()) + ->method('__call'); + + $proxy = new ContentProxy($graby, $this->getTaggerMock(), $tagRepo, $this->getLogger()); + + $tagEntity = new Tag(); + $tagEntity->setLabel('tag1'); + + $entry = new Entry(new User()); + + $proxy->assignTagsToEntry($entry, 'tag1', [$tagEntity]); + + $this->assertCount(1, $entry->getTags()); + $this->assertEquals('tag1', $entry->getTags()[0]->getLabel()); + } + private function getTaggerMock() { return $this->getMockBuilder('Wallabag\CoreBundle\Helper\RuleBasedTagger') diff --git a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php index 952521a2a..9ec7935c9 100644 --- a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php @@ -41,6 +41,20 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase ->disableOriginalConstructor() ->getMock(); + $this->uow = $this->getMockBuilder('Doctrine\ORM\UnitOfWork') + ->disableOriginalConstructor() + ->getMock(); + + $this->em + ->expects($this->any()) + ->method('getUnitOfWork') + ->willReturn($this->uow); + + $this->uow + ->expects($this->any()) + ->method('getScheduledEntityInsertions') + ->willReturn([]); + $pocket = new PocketImport( $this->em, $this->contentProxy diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php index 5ab4ad008..82dc4c7e1 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php @@ -26,6 +26,20 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase ->disableOriginalConstructor() ->getMock(); + $this->uow = $this->getMockBuilder('Doctrine\ORM\UnitOfWork') + ->disableOriginalConstructor() + ->getMock(); + + $this->em + ->expects($this->any()) + ->method('getUnitOfWork') + ->willReturn($this->uow); + + $this->uow + ->expects($this->any()) + ->method('getScheduledEntityInsertions') + ->willReturn([]); + $this->contentProxy = $this->getMockBuilder('Wallabag\CoreBundle\Helper\ContentProxy') ->disableOriginalConstructor() ->getMock(); diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php index 12bd6bdd0..bea89efbd 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php @@ -26,6 +26,20 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase ->disableOriginalConstructor() ->getMock(); + $this->uow = $this->getMockBuilder('Doctrine\ORM\UnitOfWork') + ->disableOriginalConstructor() + ->getMock(); + + $this->em + ->expects($this->any()) + ->method('getUnitOfWork') + ->willReturn($this->uow); + + $this->uow + ->expects($this->any()) + ->method('getScheduledEntityInsertions') + ->willReturn([]); + $this->contentProxy = $this->getMockBuilder('Wallabag\CoreBundle\Helper\ContentProxy') ->disableOriginalConstructor() ->getMock(); From 82fc3290d4fec45ede270e2c1ad2079fe3020adc Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sun, 25 Sep 2016 12:03:12 +0200 Subject: [PATCH 3/4] CS --- src/Wallabag/CoreBundle/Controller/TagController.php | 2 +- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 7 +++---- src/Wallabag/CoreBundle/Twig/WallabagExtension.php | 1 - 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/TagController.php b/src/Wallabag/CoreBundle/Controller/TagController.php index 07cd3edb3..623a61461 100644 --- a/src/Wallabag/CoreBundle/Controller/TagController.php +++ b/src/Wallabag/CoreBundle/Controller/TagController.php @@ -122,7 +122,7 @@ class TagController extends Controller } } - return $this->render('WallabagCoreBundle:Entry:entries.html.twig',[ + return $this->render('WallabagCoreBundle:Entry:entries.html.twig', [ 'form' => null, 'entries' => $entries, 'currentPage' => $page, diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index a65a21e8e..8019df42a 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -95,10 +95,9 @@ class ContentProxy * Assign some tags to an entry. * * @param Entry $entry - * @param array|string $tags An array of tag or a string coma separated of tag - * @param array $entitiesReady Entities from the EntityManager which are persisted but not yet flushed - * It is mostly to fix duplicate tag on import - * @see http://stackoverflow.com/a/7879164/569101 + * @param array|string $tags An array of tag or a string coma separated of tag + * @param array $entitiesReady Entities from the EntityManager which are persisted but not yet flushed + * It is mostly to fix duplicate tag on import @see http://stackoverflow.com/a/7879164/569101 */ public function assignTagsToEntry(Entry $entry, $tags, array $entitiesReady = []) { diff --git a/src/Wallabag/CoreBundle/Twig/WallabagExtension.php b/src/Wallabag/CoreBundle/Twig/WallabagExtension.php index 0a6896c95..45dc591db 100644 --- a/src/Wallabag/CoreBundle/Twig/WallabagExtension.php +++ b/src/Wallabag/CoreBundle/Twig/WallabagExtension.php @@ -2,7 +2,6 @@ namespace Wallabag\CoreBundle\Twig; -use Doctrine\ORM\Query; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface; use Wallabag\CoreBundle\Repository\EntryRepository; use Wallabag\CoreBundle\Repository\TagRepository; From 289875836a09944f5993d33753042abfef13809e Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sun, 25 Sep 2016 12:23:44 +0200 Subject: [PATCH 4/4] Fix tag count for PostgreSQL --- src/Wallabag/CoreBundle/Repository/TagRepository.php | 10 +++++----- src/Wallabag/CoreBundle/Twig/WallabagExtension.php | 4 +--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Wallabag/CoreBundle/Repository/TagRepository.php b/src/Wallabag/CoreBundle/Repository/TagRepository.php index f5c4ea6a0..9d127da71 100644 --- a/src/Wallabag/CoreBundle/Repository/TagRepository.php +++ b/src/Wallabag/CoreBundle/Repository/TagRepository.php @@ -7,17 +7,17 @@ use Doctrine\ORM\EntityRepository; class TagRepository extends EntityRepository { /** - * Find all tags per user. + * Count all tags per user. * * @param int $userId * @param int $cacheLifeTime Duration of the cache for this query * - * @return array + * @return int */ - public function findAllTags($userId, $cacheLifeTime = null) + public function countAllTags($userId, $cacheLifeTime = null) { $query = $this->createQueryBuilder('t') - ->select('t') + ->select('t.slug') ->leftJoin('t.entries', 'e') ->where('e.user = :userId')->setParameter('userId', $userId) ->groupBy('t.slug') @@ -29,7 +29,7 @@ class TagRepository extends EntityRepository $query->setResultCacheLifetime($cacheLifeTime); } - return $query->getArrayResult(); + return count($query->getArrayResult()); } /** diff --git a/src/Wallabag/CoreBundle/Twig/WallabagExtension.php b/src/Wallabag/CoreBundle/Twig/WallabagExtension.php index 45dc591db..fb4c74123 100644 --- a/src/Wallabag/CoreBundle/Twig/WallabagExtension.php +++ b/src/Wallabag/CoreBundle/Twig/WallabagExtension.php @@ -104,9 +104,7 @@ class WallabagExtension extends \Twig_Extension implements \Twig_Extension_Globa return 0; } - $data = $this->tagRepository->findAllTags($user->getId()); - - return count($data); + return $this->tagRepository->countAllTags($user->getId()); } public function getName()