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..623a61461 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/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 5dd684f2d..8019df42a 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -95,14 +95,24 @@ 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|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 +121,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/CoreBundle/Repository/TagRepository.php b/src/Wallabag/CoreBundle/Repository/TagRepository.php index 41f616079..9d127da71 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. + * Count all tags per user. + * + * @param int $userId + * @param int $cacheLifeTime Duration of the cache for this query + * + * @return int + */ + public function countAllTags($userId, $cacheLifeTime = null) + { + $query = $this->createQueryBuilder('t') + ->select('t.slug') + ->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 count($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..fb4c74123 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; @@ -85,10 +84,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,28 +104,7 @@ class WallabagExtension extends \Twig_Extension implements \Twig_Extension_Globa return 0; } - $qb = $this->tagRepository->findAllTags($user->getId()); - - $data = $this->enableCache($qb->getQuery()) - ->getArrayResult(); - - 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; + return $this->tagRepository->countAllTags($user->getId()); } public function getName() 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();