Merge pull request #1819 from wallabag/cleanup

Some cleanup

src/Wallabag/AnnotationBundle/Entity/Annotation.php

@@ -82,7 +82,7 @@ class Annotation
     /*
      * @param User     $user
      */
-    public function __construct(\Wallabag\UserBundle\Entity\User $user)
+    public function __construct(User $user)
     {
         $this->user = $user;
     }
@@ -204,7 +204,7 @@ class Annotation
     /**
      * Set user.
      *
-     * @param string $user
+     * @param User $user
      *
      * @return Annotation
      */
@@ -218,7 +218,7 @@ class Annotation
     /**
      * Get user.
      *
-     * @return string
+     * @return User
      */
     public function getUser()
     {

src/Wallabag/CoreBundle/Command/InstallCommand.php

@@ -83,7 +83,8 @@ class InstallCommand extends ContainerAwareCommand
             $help = 'Needs one of sqlite, mysql or pgsql PDO drivers';
         }
 
-        $rows[] = array($label, $status, $help);
+        $rows = [];
+        $rows[] = [$label, $status, $help];
 
         foreach ($this->functionExists as $functionRequired) {
             $label = '<comment>'.$functionRequired.'</comment>';
@@ -97,12 +98,12 @@ class InstallCommand extends ContainerAwareCommand
                 $help = 'You need the '.$functionRequired.' function activated';
             }
 
-            $rows[] = array($label, $status, $help);
+            $rows[] = [$label, $status, $help];
         }
 
         $table = new Table($this->defaultOutput);
         $table
-            ->setHeaders(array('Checked', 'Status', 'Recommendation'))
+            ->setHeaders(['Checked', 'Status', 'Recommendation'])
             ->setRows($rows)
             ->render();
 
@@ -126,7 +127,7 @@ class InstallCommand extends ContainerAwareCommand
             $this->defaultOutput->writeln('Droping database, creating database and schema, clearing the cache');
 
             $this
-                ->runCommand('doctrine:database:drop', array('--force' => true))
+                ->runCommand('doctrine:database:drop', ['--force' => true])
                 ->runCommand('doctrine:database:create')
                 ->runCommand('doctrine:schema:create')
                 ->runCommand('cache:clear')
@@ -158,7 +159,7 @@ class InstallCommand extends ContainerAwareCommand
             $this->defaultOutput->writeln('Droping database, creating database and schema');
 
             $this
-                ->runCommand('doctrine:database:drop', array('--force' => true))
+                ->runCommand('doctrine:database:drop', ['--force' => true])
                 ->runCommand('doctrine:database:create')
                 ->runCommand('doctrine:schema:create')
             ;
@@ -168,7 +169,7 @@ class InstallCommand extends ContainerAwareCommand
                 $this->defaultOutput->writeln('Droping schema and creating schema');
 
                 $this
-                    ->runCommand('doctrine:schema:drop', array('--force' => true))
+                    ->runCommand('doctrine:schema:drop', ['--force' => true])
                     ->runCommand('doctrine:schema:create')
                 ;
             }
@@ -388,19 +389,19 @@ class InstallCommand extends ContainerAwareCommand
      * @param string $command
      * @param array  $parameters Parameters to this command (usually 'force' => true)
      */
-    protected function runCommand($command, $parameters = array())
+    protected function runCommand($command, $parameters = [])
     {
         $parameters = array_merge(
-            array('command' => $command),
+            ['command' => $command],
             $parameters,
-            array(
+            [
                 '--no-debug' => true,
                 '--env' => $this->defaultInput->getOption('env') ?: 'dev',
-            )
+            ]
         );
 
         if ($this->defaultInput->getOption('no-interaction')) {
-            $parameters = array_merge($parameters, array('--no-interaction' => true));
+            $parameters = array_merge($parameters, ['--no-interaction' => true]);
         }
 
         $this->getApplication()->setAutoExit(false);

src/Wallabag/CoreBundle/Controller/EntryController.php

@@ -92,13 +92,11 @@ class EntryController extends Controller
     }
 
     /**
-     * @param Request $request
-     *
      * @Route("/new", name="new")
      *
      * @return \Symfony\Component\HttpFoundation\Response
      */
-    public function addEntryAction(Request $request)
+    public function addEntryAction()
     {
         return $this->render('WallabagCoreBundle:Entry:new.html.twig');
     }

src/Wallabag/CoreBundle/Entity/Entry.php

@@ -187,7 +187,7 @@ class Entry
     /*
      * @param User     $user
      */
-    public function __construct(\Wallabag\UserBundle\Entity\User $user)
+    public function __construct(User $user)
     {
         $this->user = $user;
         $this->tags = new ArrayCollection();

src/Wallabag/CoreBundle/Helper/ContentProxy.php

@@ -32,14 +32,21 @@ class ContentProxy
      * Fetch content using graby and hydrate given entry with results information.
      * In case we couldn't find content, we'll try to use Open Graph data.
      *
+     * We can also force the content, in case of an import from the v1 for example, so the function won't
+     * fetch the content from the website but rather use information given with the $content parameter.
+     *
      * @param Entry  $entry   Entry to update
      * @param string $url     Url to grab content for
+     * @param array  $content An array with AT LEAST keys title, html, url, language & content_type to skip the fetchContent from the url
      *
      * @return Entry
      */
-    public function updateEntry(Entry $entry, $url)
+    public function updateEntry(Entry $entry, $url, array $content = [])
     {
+        // do we have to fetch the content or the provided one is ok?
+        if (empty($content) || false === $this->validateContent($content)) {
             $content = $this->graby->fetchContent($url);
+        }
 
         $title = $content['title'];
         if (!$title && isset($content['open_graph']['og_title'])) {
@@ -62,7 +69,11 @@ class ContentProxy
         $entry->setLanguage($content['language']);
         $entry->setMimetype($content['content_type']);
         $entry->setReadingTime(Utils::getReadingTime($html));
-        $entry->setDomainName(parse_url($entry->getUrl(), PHP_URL_HOST));
+
+        $domainName = parse_url($entry->getUrl(), PHP_URL_HOST);
+        if (false !== $domainName) {
+            $entry->setDomainName($domainName);
+        }
 
         if (isset($content['open_graph']['og_image'])) {
             $entry->setPreviewPicture($content['open_graph']['og_image']);
@@ -113,4 +124,17 @@ class ContentProxy
             }
         }
     }
+
+    /**
+     * Validate that the given content as enough value to be used
+     * instead of fetch the content from the url.
+     *
+     * @param array $content
+     *
+     * @return bool true if valid otherwise false
+     */
+    private function validateContent(array $content)
+    {
+        return isset($content['title']) && isset($content['html']) && isset($content['url']) && isset($content['language']) && isset($content['content_type']);
+    }
 }

src/Wallabag/CoreBundle/Helper/EntriesExport.php

@@ -81,27 +81,9 @@ class EntriesExport
      */
     public function exportAs($format)
     {
-        switch ($format) {
+        $functionName = 'produce'.ucfirst($format);
-            case 'epub':
+        if (method_exists($this, $functionName)) {
-                return $this->produceEpub();
+            return $this->$functionName();
-
-            case 'mobi':
-                return $this->produceMobi();
-
-            case 'pdf':
-                return $this->producePDF();
-
-            case 'csv':
-                return $this->produceCSV();
-
-            case 'json':
-                return $this->produceJSON();
-
-            case 'xml':
-                return $this->produceXML();
-
-            case 'txt':
-                return $this->produceTXT();
         }
 
         throw new \InvalidArgumentException(sprintf('The format "%s" is not yet supported.', $format));
@@ -242,7 +224,7 @@ class EntriesExport
     /**
      * Use TCPDF to dump a .pdf file.
      */
-    private function producePDF()
+    private function producePdf()
     {
         $pdf = new \TCPDF(PDF_PAGE_ORIENTATION, PDF_UNIT, PDF_PAGE_FORMAT, true, 'UTF-8', false);
 
@@ -296,7 +278,7 @@ class EntriesExport
     /**
      * Inspired from CsvFileDumper.
      */
-    private function produceCSV()
+    private function produceCsv()
     {
         $delimiter = ';';
         $enclosure = '"';
@@ -336,7 +318,7 @@ class EntriesExport
         );
     }
 
-    private function produceJSON()
+    private function produceJson()
     {
         return Response::create(
             $this->prepareSerializingContent('json'),
@@ -349,7 +331,7 @@ class EntriesExport
         );
     }
 
-    private function produceXML()
+    private function produceXml()
     {
         return Response::create(
             $this->prepareSerializingContent('xml'),
@@ -362,7 +344,7 @@ class EntriesExport
         );
     }
 
-    private function produceTXT()
+    private function produceTxt()
     {
         $content = '';
         $bar = str_repeat('=', 100);

src/Wallabag/CoreBundle/Tests/Helper/ContentProxyTest.php

@@ -10,6 +10,40 @@ use Wallabag\UserBundle\Entity\User;
 
 class ContentProxyTest extends \PHPUnit_Framework_TestCase
 {
+    public function testWithBadUrl()
+    {
+        $tagger = $this->getTaggerMock();
+        $tagger->expects($this->once())
+            ->method('tag');
+
+        $graby = $this->getMockBuilder('Graby\Graby')
+            ->setMethods(array('fetchContent'))
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $graby->expects($this->any())
+            ->method('fetchContent')
+            ->willReturn(array(
+                'html' => false,
+                'title' => '',
+                'url' => '',
+                'content_type' => '',
+                'language' => '',
+            ));
+
+        $proxy = new ContentProxy($graby, $tagger, $this->getTagRepositoryMock(), $this->getLogger());
+        $entry = $proxy->updateEntry(new Entry(new User()), 'http://user@:80');
+
+        $this->assertEquals('http://user@:80', $entry->getUrl());
+        $this->assertEmpty($entry->getTitle());
+        $this->assertEquals('<p>Unable to retrieve readable content.</p>', $entry->getContent());
+        $this->assertEmpty($entry->getPreviewPicture());
+        $this->assertEmpty($entry->getMimetype());
+        $this->assertEmpty($entry->getLanguage());
+        $this->assertEquals(0.0, $entry->getReadingTime());
+        $this->assertEquals(false, $entry->getDomainName());
+    }
+
     public function testWithEmptyContent()
     {
         $tagger = $this->getTaggerMock();
@@ -121,6 +155,57 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('1.1.1.1', $entry->getDomainName());
     }
 
+    public function testWithForcedContent()
+    {
+        $tagger = $this->getTaggerMock();
+        $tagger->expects($this->once())
+            ->method('tag');
+
+        $graby = $this->getMockBuilder('Graby\Graby')->getMock();
+
+        $proxy = new ContentProxy($graby, $tagger, $this->getTagRepositoryMock(), $this->getLogger());
+        $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0', [
+            'html' => str_repeat('this is my content', 325),
+            'title' => 'this is my title',
+            'url' => 'http://1.1.1.1',
+            'content_type' => 'text/html',
+            'language' => 'fr',
+        ]);
+
+        $this->assertEquals('http://1.1.1.1', $entry->getUrl());
+        $this->assertEquals('this is my title', $entry->getTitle());
+        $this->assertContains('this is my content', $entry->getContent());
+        $this->assertEquals('text/html', $entry->getMimetype());
+        $this->assertEquals('fr', $entry->getLanguage());
+        $this->assertEquals(4.0, $entry->getReadingTime());
+        $this->assertEquals('1.1.1.1', $entry->getDomainName());
+    }
+
+    public function testTaggerThrowException()
+    {
+        $graby = $this->getMockBuilder('Graby\Graby')
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $tagger = $this->getTaggerMock();
+        $tagger->expects($this->once())
+            ->method('tag')
+            ->will($this->throwException(new \Exception()));
+
+        $tagRepo = $this->getTagRepositoryMock();
+        $proxy = new ContentProxy($graby, $tagger, $tagRepo, $this->getLogger());
+
+        $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0', [
+            'html' => str_repeat('this is my content', 325),
+            'title' => 'this is my title',
+            'url' => 'http://1.1.1.1',
+            'content_type' => 'text/html',
+            'language' => 'fr',
+        ]);
+
+        $this->assertCount(0, $entry->getTags());
+    }
+
     public function testAssignTagsWithArrayAndExtraSpaces()
     {
         $graby = $this->getMockBuilder('Graby\Graby')

src/Wallabag/ImportBundle/Controller/PocketController.php

@@ -38,6 +38,15 @@ class PocketController extends Controller
         $requestToken = $this->get('wallabag_import.pocket.import')
             ->getRequestToken($this->generateUrl('import', array(), UrlGeneratorInterface::ABSOLUTE_URL));
 
+        if (false === $requestToken) {
+            $this->get('session')->getFlashBag()->add(
+                'notice',
+                'flashes.import.notice.failed'
+            );
+
+            return $this->redirect($this->generateUrl('import_pocket'));
+        }
+
         $this->get('session')->set('import.pocket.code', $requestToken);
         $this->get('session')->set('mark_as_read', $request->request->get('form')['mark_as_read']);
 

src/Wallabag/ImportBundle/Import/PocketImport.php

@@ -68,7 +68,7 @@ class PocketImport implements ImportInterface
      *
      * @param string $redirectUri Redirect url in case of error
      *
-     * @return string request_token for callback method
+     * @return string|false request_token for callback method
      */
     public function getRequestToken($redirectUri)
     {

src/Wallabag/ImportBundle/Import/WallabagV1Import.php

@@ -7,7 +7,6 @@ use Psr\Log\NullLogger;
 use Doctrine\ORM\EntityManager;
 use Wallabag\CoreBundle\Entity\Entry;
 use Wallabag\UserBundle\Entity\User;
-use Wallabag\CoreBundle\Tools\Utils;
 use Wallabag\CoreBundle\Helper\ContentProxy;
 
 class WallabagV1Import implements ImportInterface
@@ -153,19 +152,25 @@ class WallabagV1Import implements ImportInterface
                 continue;
             }
 
-            // @see ContentProxy->updateEntry
+            $data = [
-            $entry = new Entry($this->user);
+                'title' => $importedEntry['title'],
-            $entry->setUrl($importedEntry['url']);
+                'html' => $importedEntry['content'],
+                'url' => $importedEntry['url'],
+                'content_type' => '',
+                'language' => '',
+            ];
 
+            // force content to be refreshed in case on bad fetch in the v1 installation
             if (in_array($importedEntry['title'], $untitled)) {
-                $entry = $this->contentProxy->updateEntry($entry, $importedEntry['url']);
+                $data = [];
-            } else {
-                $entry->setContent($importedEntry['content']);
-                $entry->setTitle($importedEntry['title']);
-                $entry->setReadingTime(Utils::getReadingTime($importedEntry['content']));
-                $entry->setDomainName(parse_url($importedEntry['url'], PHP_URL_HOST));
             }
 
+            $entry = $this->contentProxy->updateEntry(
+                new Entry($this->user),
+                $importedEntry['url'],
+                $data
+            );
+
             if (array_key_exists('tags', $importedEntry) && $importedEntry['tags'] != '') {
                 $this->contentProxy->assignTagsToEntry(
                     $entry,

src/Wallabag/ImportBundle/Import/WallabagV2Import.php

@@ -47,25 +47,29 @@ class WallabagV2Import extends WallabagV1Import implements ImportInterface
                 continue;
             }
 
-            // @see ContentProxy->updateEntry
+            $importedEntry['html'] = $importedEntry['content'];
-            $entry = new Entry($this->user);
+            $importedEntry['content_type'] = $importedEntry['mimetype'];
-            $entry->setUrl($importedEntry['url']);
+
-            $entry->setTitle($importedEntry['title']);
+            $entry = $this->contentProxy->updateEntry(
-            $entry->setArchived($importedEntry['is_archived'] || $this->markAsRead);
+                new Entry($this->user),
-            $entry->setStarred($importedEntry['is_starred']);
+                $importedEntry['url'],
-            $entry->setContent($importedEntry['content']);
+                $importedEntry
-            $entry->setReadingTime($importedEntry['reading_time']);
+            );
-            $entry->setDomainName($importedEntry['domain_name']);
+
-            if (isset($importedEntry['mimetype'])) {
+            if (array_key_exists('tags', $importedEntry) && !empty($importedEntry['tags'])) {
-                $entry->setMimetype($importedEntry['mimetype']);
+                $this->contentProxy->assignTagsToEntry(
-            }
+                    $entry,
-            if (isset($importedEntry['language'])) {
+                    $importedEntry['tags']
-                $entry->setLanguage($importedEntry['language']);
+                );
             }
+
             if (isset($importedEntry['preview_picture'])) {
                 $entry->setPreviewPicture($importedEntry['preview_picture']);
             }
 
+            $entry->setArchived($importedEntry['is_archived'] || $this->markAsRead);
+            $entry->setStarred($importedEntry['is_starred']);
+
             $this->em->persist($entry);
             ++$this->importedEntries;
 

src/Wallabag/ImportBundle/Tests/Controller/PocketControllerTest.php

@@ -17,13 +17,36 @@ class PocketControllerTest extends WallabagCoreTestCase
         $this->assertEquals(1, $crawler->filter('button[type=submit]')->count());
     }
 
-    public function testImportPocketAuth()
+    public function testImportPocketAuthBadToken()
     {
         $this->logInAs('admin');
         $client = $this->getClient();
 
         $crawler = $client->request('GET', '/import/pocket/auth');
 
+        $this->assertEquals(302, $client->getResponse()->getStatusCode());
+    }
+
+    public function testImportPocketAuth()
+    {
+        $this->markTestSkipped('PocketImport: Find a way to properly mock a service.');
+
+        $this->logInAs('admin');
+        $client = $this->getClient();
+
+        $pocketImport = $this->getMockBuilder('Wallabag\ImportBundle\Import\PocketImport')
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $pocketImport
+            ->expects($this->once())
+            ->method('getRequestToken')
+            ->willReturn('token');
+
+        $client->getContainer()->set('wallabag_import.pocket.import', $pocketImport);
+
+        $crawler = $client->request('GET', '/import/pocket/auth');
+
         $this->assertEquals(301, $client->getResponse()->getStatusCode());
         $this->assertContains('getpocket.com/auth/authorize', $client->getResponse()->headers->get('location'));
     }

src/Wallabag/ImportBundle/Tests/Controller/WallabagV2ControllerTest.php

@@ -53,6 +53,7 @@ class WallabagV2ControllerTest extends WallabagCoreTestCase
         $this->assertEmpty($content->getMimetype());
         $this->assertEmpty($content->getPreviewPicture());
         $this->assertEmpty($content->getLanguage());
+        $this->assertEquals(0, count($content->getTags()));
 
         $content = $client->getContainer()
             ->get('doctrine.orm.entity_manager')
@@ -65,6 +66,7 @@ class WallabagV2ControllerTest extends WallabagCoreTestCase
         $this->assertNotEmpty($content->getMimetype());
         $this->assertNotEmpty($content->getPreviewPicture());
         $this->assertNotEmpty($content->getLanguage());
+        $this->assertEquals(2, count($content->getTags()));
     }
 
     public function testImportWallabagWithEmptyFile()

src/Wallabag/ImportBundle/Tests/Import/WallabagV1ImportTest.php

@@ -2,8 +2,9 @@
 
 namespace Wallabag\ImportBundle\Tests\Import;
 
-use Wallabag\UserBundle\Entity\User;
 use Wallabag\ImportBundle\Import\WallabagV1Import;
+use Wallabag\UserBundle\Entity\User;
+use Wallabag\CoreBundle\Entity\Entry;
 use Monolog\Logger;
 use Monolog\Handler\TestHandler;
 
@@ -71,7 +72,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase
             ->getMock();
 
         $this->contentProxy
-            ->expects($this->once())
+            ->expects($this->exactly(3))
             ->method('updateEntry')
             ->willReturn($entry);
 
@@ -99,6 +100,11 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase
             ->method('getRepository')
             ->willReturn($entryRepo);
 
+        $this->contentProxy
+            ->expects($this->exactly(3))
+            ->method('updateEntry')
+            ->willReturn(new Entry($this->user));
+
         // check that every entry persisted are archived
         $this->em
             ->expects($this->any())

src/Wallabag/ImportBundle/Tests/Import/WallabagV2ImportTest.php

@@ -4,6 +4,7 @@ namespace Wallabag\ImportBundle\Tests\Import;
 
 use Wallabag\ImportBundle\Import\WallabagV2Import;
 use Wallabag\UserBundle\Entity\User;
+use Wallabag\CoreBundle\Entity\Entry;
 use Monolog\Logger;
 use Monolog\Handler\TestHandler;
 
@@ -66,6 +67,11 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase
             ->method('getRepository')
             ->willReturn($entryRepo);
 
+        $this->contentProxy
+            ->expects($this->exactly(2))
+            ->method('updateEntry')
+            ->willReturn(new Entry($this->user));
+
         $res = $wallabagV2Import->import();
 
         $this->assertTrue($res);
@@ -90,6 +96,11 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase
             ->method('getRepository')
             ->willReturn($entryRepo);
 
+        $this->contentProxy
+            ->expects($this->exactly(2))
+            ->method('updateEntry')
+            ->willReturn(new Entry($this->user));
+
         // check that every entry persisted are archived
         $this->em
             ->expects($this->any())