From 7aba665e484c5c36ee029219a999a427d864ff22 Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Tue, 6 Dec 2016 22:17:44 -0500 Subject: [PATCH 01/11] Avoid returning objects passed by reference. Objects are always passed by reference, so it doesn't make sense to return an object which is passed by reference as it will always be the same object. This change makes the code a bit more readable. --- .../Controller/EntryRestController.php | 4 +-- .../CoreBundle/Controller/EntryController.php | 8 ++--- .../CoreBundle/Helper/ContentProxy.php | 4 --- .../ImportBundle/Import/AbstractImport.php | 4 +-- .../ImportBundle/Import/BrowserImport.php | 2 +- .../ImportBundle/Import/InstapaperImport.php | 2 +- .../ImportBundle/Import/PinboardImport.php | 2 +- .../ImportBundle/Import/PocketImport.php | 2 +- .../ImportBundle/Import/ReadabilityImport.php | 2 +- .../ImportBundle/Import/WallabagImport.php | 2 +- .../CoreBundle/Helper/ContentProxyTest.php | 30 ++++++++++++------- 11 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/Wallabag/ApiBundle/Controller/EntryRestController.php b/src/Wallabag/ApiBundle/Controller/EntryRestController.php index c3ba1858c..d154c18b3 100644 --- a/src/Wallabag/ApiBundle/Controller/EntryRestController.php +++ b/src/Wallabag/ApiBundle/Controller/EntryRestController.php @@ -315,7 +315,7 @@ class EntryRestController extends WallabagRestController } try { - $entry = $this->get('wallabag_core.content_proxy')->updateEntry( + $this->get('wallabag_core.content_proxy')->updateEntry( $entry, $url, [ @@ -428,7 +428,7 @@ class EntryRestController extends WallabagRestController $this->validateUserAccess($entry->getUser()->getId()); try { - $entry = $this->get('wallabag_core.content_proxy')->updateEntry($entry, $entry->getUrl()); + $this->get('wallabag_core.content_proxy')->updateEntry($entry, $entry->getUrl()); } catch (\Exception $e) { $this->get('logger')->error('Error while saving an entry', [ 'exception' => $e, diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 8d2ac6d4c..6018dfacd 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -53,12 +53,10 @@ class EntryController extends Controller /** * Fetch content and update entry. - * In case it fails, entry will return to avod loosing the data. + * In case it fails, $entry->getContent will return an error message. * * @param Entry $entry * @param string $prefixMessage Should be the translation key: entry_saved or entry_reloaded - * - * @return Entry */ private function updateEntry(Entry $entry, $prefixMessage = 'entry_saved') { @@ -68,7 +66,7 @@ class EntryController extends Controller $message = 'flashes.entry.notice.'.$prefixMessage; try { - $entry = $this->get('wallabag_core.content_proxy')->updateEntry($entry, $entry->getUrl()); + $this->get('wallabag_core.content_proxy')->updateEntry($entry, $entry->getUrl()); } catch (\Exception $e) { $this->get('logger')->error('Error while saving an entry', [ 'exception' => $e, @@ -79,8 +77,6 @@ class EntryController extends Controller } $this->get('session')->getFlashBag()->add('notice', $message); - - return $entry; } /** diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 8ba77ca92..c73b8eafb 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -40,8 +40,6 @@ class ContentProxy * @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 to skip the fetchContent from the url - * - * @return Entry */ public function updateEntry(Entry $entry, $url, array $content = []) { @@ -130,8 +128,6 @@ class ContentProxy 'error_msg' => $e->getMessage(), ]); } - - return $entry; } /** diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index a61388c01..fc462c4cd 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -91,13 +91,11 @@ abstract class AbstractImport implements ImportInterface * @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 */ protected function fetchContent(Entry $entry, $url, array $content = []) { try { - return $this->contentProxy->updateEntry($entry, $url, $content); + $this->contentProxy->updateEntry($entry, $url, $content); } catch (\Exception $e) { return $entry; } diff --git a/src/Wallabag/ImportBundle/Import/BrowserImport.php b/src/Wallabag/ImportBundle/Import/BrowserImport.php index ef0eeb7e1..71e65e591 100644 --- a/src/Wallabag/ImportBundle/Import/BrowserImport.php +++ b/src/Wallabag/ImportBundle/Import/BrowserImport.php @@ -201,7 +201,7 @@ abstract class BrowserImport extends AbstractImport $entry->setTitle($data['title']); // update entry with content (in case fetching failed, the given entry will be return) - $entry = $this->fetchContent($entry, $data['url'], $data); + $this->fetchContent($entry, $data['url'], $data); if (array_key_exists('tags', $data)) { $this->tagsAssigner->assignTagsToEntry( diff --git a/src/Wallabag/ImportBundle/Import/InstapaperImport.php b/src/Wallabag/ImportBundle/Import/InstapaperImport.php index c8e0cd5b2..3aa12f6fb 100644 --- a/src/Wallabag/ImportBundle/Import/InstapaperImport.php +++ b/src/Wallabag/ImportBundle/Import/InstapaperImport.php @@ -125,7 +125,7 @@ class InstapaperImport extends AbstractImport $entry->setTitle($importedEntry['title']); // update entry with content (in case fetching failed, the given entry will be return) - $entry = $this->fetchContent($entry, $importedEntry['url'], $importedEntry); + $this->fetchContent($entry, $importedEntry['url'], $importedEntry); if (!empty($importedEntry['tags'])) { $this->tagsAssigner->assignTagsToEntry( diff --git a/src/Wallabag/ImportBundle/Import/PinboardImport.php b/src/Wallabag/ImportBundle/Import/PinboardImport.php index 489b9257e..110b04642 100644 --- a/src/Wallabag/ImportBundle/Import/PinboardImport.php +++ b/src/Wallabag/ImportBundle/Import/PinboardImport.php @@ -109,7 +109,7 @@ class PinboardImport extends AbstractImport $entry->setTitle($data['title']); // update entry with content (in case fetching failed, the given entry will be return) - $entry = $this->fetchContent($entry, $data['url'], $data); + $this->fetchContent($entry, $data['url'], $data); if (!empty($data['tags'])) { $this->tagsAssigner->assignTagsToEntry( diff --git a/src/Wallabag/ImportBundle/Import/PocketImport.php b/src/Wallabag/ImportBundle/Import/PocketImport.php index 8835161b1..c1d5b6da0 100644 --- a/src/Wallabag/ImportBundle/Import/PocketImport.php +++ b/src/Wallabag/ImportBundle/Import/PocketImport.php @@ -192,7 +192,7 @@ class PocketImport extends AbstractImport $entry->setUrl($url); // update entry with content (in case fetching failed, the given entry will be return) - $entry = $this->fetchContent($entry, $url); + $this->fetchContent($entry, $url); // 0, 1, 2 - 1 if the item is archived - 2 if the item should be deleted $entry->setArchived($importedEntry['status'] == 1 || $this->markAsRead); diff --git a/src/Wallabag/ImportBundle/Import/ReadabilityImport.php b/src/Wallabag/ImportBundle/Import/ReadabilityImport.php index de320d239..002b27f46 100644 --- a/src/Wallabag/ImportBundle/Import/ReadabilityImport.php +++ b/src/Wallabag/ImportBundle/Import/ReadabilityImport.php @@ -109,7 +109,7 @@ class ReadabilityImport extends AbstractImport $entry->setTitle($data['title']); // update entry with content (in case fetching failed, the given entry will be return) - $entry = $this->fetchContent($entry, $data['url'], $data); + $this->fetchContent($entry, $data['url'], $data); $entry->setArchived($data['is_archived']); $entry->setStarred($data['is_starred']); diff --git a/src/Wallabag/ImportBundle/Import/WallabagImport.php b/src/Wallabag/ImportBundle/Import/WallabagImport.php index 0e5382cfe..c64ccd64d 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagImport.php +++ b/src/Wallabag/ImportBundle/Import/WallabagImport.php @@ -108,7 +108,7 @@ abstract class WallabagImport extends AbstractImport $entry->setTitle($data['title']); // update entry with content (in case fetching failed, the given entry will be return) - $entry = $this->fetchContent($entry, $data['url'], $data); + $this->fetchContent($entry, $data['url'], $data); if (array_key_exists('tags', $data)) { $this->tagsAssigner->assignTagsToEntry( diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index 0c715d90a..166439387 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -38,7 +38,8 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ]); $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry(new Entry(new User()), 'http://user@:80'); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://user@:80'); $this->assertEquals('http://user@:80', $entry->getUrl()); $this->assertEmpty($entry->getTitle()); @@ -72,7 +73,8 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ]); $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0'); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); $this->assertEquals('http://0.0.0.0', $entry->getUrl()); $this->assertEmpty($entry->getTitle()); @@ -111,7 +113,8 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ]); $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry(new Entry(new User()), 'http://domain.io'); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://domain.io'); $this->assertEquals('http://domain.io', $entry->getUrl()); $this->assertEquals('my title', $entry->getTitle()); @@ -152,7 +155,8 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ]); $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0'); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); $this->assertEquals('http://1.1.1.1', $entry->getUrl()); $this->assertEquals('this is my title', $entry->getTitle()); @@ -213,8 +217,9 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->method('tag'); $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry( - new Entry(new User()), + $entry = new Entry(new User()); + $proxy->updateEntry( + $entry, 'http://0.0.0.0', [ 'html' => str_repeat('this is my content', 325), @@ -251,8 +256,9 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->method('tag'); $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry( - new Entry(new User()), + $entry = new Entry(new User()); + $proxy->updateEntry( + $entry, 'http://0.0.0.0', [ 'html' => str_repeat('this is my content', 325), @@ -285,8 +291,9 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $logger->pushHandler($handler); $proxy = new ContentProxy((new Graby()), $tagger, $logger, $this->fetchingErrorMessage); - $entry = $proxy->updateEntry( - new Entry(new User()), + $entry = new Entry(new User()); + $proxy->updateEntry( + $entry, 'http://0.0.0.0', [ 'html' => str_repeat('this is my content', 325), @@ -326,7 +333,8 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0', [ + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0', [ 'html' => str_repeat('this is my content', 325), 'title' => 'this is my title', 'url' => 'http://1.1.1.1', From 1c5da417e4ddb14223f9af6e5cea6778e5c0fd08 Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Tue, 6 Dec 2016 22:27:08 -0500 Subject: [PATCH 02/11] Put default fetching error title in global config --- app/config/config.yml | 1 + src/Wallabag/CoreBundle/Controller/EntryController.php | 3 --- src/Wallabag/CoreBundle/DependencyInjection/Configuration.php | 2 ++ .../CoreBundle/DependencyInjection/WallabagCoreExtension.php | 1 + src/Wallabag/CoreBundle/Resources/config/services.yml | 1 + 5 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/config/config.yml b/app/config/config.yml index 9792616ec..04f8547de 100644 --- a/app/config/config.yml +++ b/app/config/config.yml @@ -58,6 +58,7 @@ wallabag_core: cache_lifetime: 10 action_mark_as_read: 1 list_mode: 0 + fetching_error_message_title: 'No title found' fetching_error_message: | wallabag can't retrieve contents for this article. Please troubleshoot this issue. api_limit_mass_actions: 10 diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 6018dfacd..4b2bc7da4 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -60,9 +60,6 @@ class EntryController extends Controller */ private function updateEntry(Entry $entry, $prefixMessage = 'entry_saved') { - // put default title in case of fetching content failed - $entry->setTitle('No title found'); - $message = 'flashes.entry.notice.'.$prefixMessage; try { diff --git a/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php b/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php index 75b37729f..8b5b57442 100644 --- a/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php +++ b/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php @@ -41,6 +41,8 @@ class Configuration implements ConfigurationInterface ->end() ->scalarNode('fetching_error_message') ->end() + ->scalarNode('fetching_error_message_title') + ->end() ->scalarNode('action_mark_as_read') ->defaultValue(1) ->end() diff --git a/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php b/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php index c075c19fd..a2a703cb6 100644 --- a/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php +++ b/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php @@ -26,6 +26,7 @@ class WallabagCoreExtension extends Extension $container->setParameter('wallabag_core.action_mark_as_read', $config['action_mark_as_read']); $container->setParameter('wallabag_core.list_mode', $config['list_mode']); $container->setParameter('wallabag_core.fetching_error_message', $config['fetching_error_message']); + $container->setParameter('wallabag_core.fetching_error_message_title', $config['fetching_error_message_title']); $container->setParameter('wallabag_core.api_limit_mass_actions', $config['api_limit_mass_actions']); $loader = new Loader\YamlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config')); diff --git a/src/Wallabag/CoreBundle/Resources/config/services.yml b/src/Wallabag/CoreBundle/Resources/config/services.yml index a68b2fdcd..a9b0d2d56 100644 --- a/src/Wallabag/CoreBundle/Resources/config/services.yml +++ b/src/Wallabag/CoreBundle/Resources/config/services.yml @@ -41,6 +41,7 @@ services: arguments: - error_message: '%wallabag_core.fetching_error_message%' + error_message_title: '%wallabag_core.fetching_error_message_title%' - "@wallabag_core.guzzle.http_client" - "@wallabag_core.graby.config_builder" calls: From d0e9b3d640acce49068d1a2c5603b92c1bda363e Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Wed, 7 Dec 2016 15:16:49 -0500 Subject: [PATCH 03/11] Add disableContentUpdate import option This commit also decouples the "import" and "update" functions inside ContentProxy. If a content array is available, it must be passed to the new importEntry method. --- .../CoreBundle/Helper/ContentProxy.php | 76 +++++++++++++------ .../ImportBundle/Command/ImportCommand.php | 4 +- .../ImportBundle/Import/AbstractImport.php | 29 ++++++- .../CoreBundle/Helper/ContentProxyTest.php | 9 +-- .../ImportBundle/Import/ChromeImportTest.php | 8 +- .../ImportBundle/Import/FirefoxImportTest.php | 8 +- .../Import/InstapaperImportTest.php | 8 +- .../ImportBundle/Import/PocketImportTest.php | 10 +-- .../Import/ReadabilityImportTest.php | 8 +- .../Import/WallabagV1ImportTest.php | 8 +- .../Import/WallabagV2ImportTest.php | 10 +-- 11 files changed, 118 insertions(+), 60 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index c73b8eafb..88873bd53 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -7,6 +7,7 @@ use Psr\Log\LoggerInterface; use Wallabag\CoreBundle\Entity\Entry; use Wallabag\CoreBundle\Tools\Utils; use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeExtensionGuesser; +use Symfony\Component\Config\Definition\Exception\Exception; /** * This kind of proxy class take care of getting the content from an url @@ -31,34 +32,58 @@ 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. + * Update existing entry by fetching from URL using Graby. * * @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 to skip the fetchContent from the url */ - public function updateEntry(Entry $entry, $url, array $content = []) + public function updateEntry(Entry $entry, $url) { - // ensure content is a bit cleaned up - if (!empty($content['html'])) { - $content['html'] = $this->graby->cleanupHtml($content['html'], $url); - } + $content = $this->graby->fetchContent($url); - // do we have to fetch the content or the provided one is ok? - if (empty($content) || false === $this->validateContent($content)) { - $fetchedContent = $this->graby->fetchContent($url); + $this->stockEntry($entry, $content); + } + + /** + * Import entry using either fetched or provided content. + * + * @param Entry $entry Entry to update + * @param array $content Array with content provided for import with AT LEAST keys title, html, url to skip the fetchContent from the url + * @param bool $disableContentUpdate Whether to skip trying to fetch content using Graby + */ + public function importEntry(Entry $entry, array $content, $disableContentUpdate = false) + { + $this->validateContent($content); + + if (false === $disableContentUpdate) { + try { + $fetchedContent = $this->graby->fetchContent($content['url']); + } catch (\Exception $e) { + $this->logger->error('Error while trying to fetch content from URL.', [ + 'entry_url' => $content['url'], + 'error_msg' => $e->getMessage(), + ]); + } // when content is imported, we have information in $content // in case fetching content goes bad, we'll keep the imported information instead of overriding them - if (empty($content) || $fetchedContent['html'] !== $this->fetchingErrorMessage) { + if ($fetchedContent['html'] !== $this->fetchingErrorMessage) { $content = $fetchedContent; } } + $this->stockEntry($entry, $content); + } + + /** + * Stock entry with fetched or imported content. + * Will fall back to OpenGraph data if available. + * + * @param Entry $entry Entry to stock + * @param array $content Array with at least title and URL + */ + private function stockEntry(Entry $entry, array $content) + { $title = $content['title']; if (!$title && !empty($content['open_graph']['og_title'])) { $title = $content['open_graph']['og_title']; @@ -74,7 +99,7 @@ class ContentProxy } } - $entry->setUrl($content['url'] ?: $url); + $entry->setUrl($content['url']); $entry->setTitle($title); $entry->setContent($html); $entry->setHttpStatus(isset($content['status']) ? $content['status'] : ''); @@ -124,22 +149,29 @@ class ContentProxy $this->tagger->tag($entry); } catch (\Exception $e) { $this->logger->error('Error while trying to automatically tag an entry.', [ - 'entry_url' => $url, + 'entry_url' => $content['url'], 'error_msg' => $e->getMessage(), ]); } } /** - * Validate that the given content as enough value to be used - * instead of fetch the content from the url. + * Validate that the given content has at least a title, an html and a url. * * @param array $content - * - * @return bool true if valid otherwise false */ private function validateContent(array $content) { - return !empty($content['title']) && !empty($content['html']) && !empty($content['url']); + if (!empty($content['title']))) { + throw new Exception('Missing title from imported entry!'); + } + + if (!empty($content['url']))) { + throw new Exception('Missing URL from imported entry!'); + } + + if (!empty($content['html']))) { + throw new Exception('Missing html from imported entry!'); + } } } diff --git a/src/Wallabag/ImportBundle/Command/ImportCommand.php b/src/Wallabag/ImportBundle/Command/ImportCommand.php index ce72837ad..bca800e6c 100644 --- a/src/Wallabag/ImportBundle/Command/ImportCommand.php +++ b/src/Wallabag/ImportBundle/Command/ImportCommand.php @@ -5,6 +5,7 @@ namespace Wallabag\ImportBundle\Command; use Symfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand; use Symfony\Component\Config\Definition\Exception\Exception; use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -19,7 +20,7 @@ class ImportCommand extends ContainerAwareCommand ->addArgument('filepath', InputArgument::REQUIRED, 'Path to the JSON file') ->addOption('importer', null, InputArgument::OPTIONAL, 'The importer to use: v1, v2, instapaper, pinboard, readability, firefox or chrome', 'v1') ->addOption('markAsRead', null, InputArgument::OPTIONAL, 'Mark all entries as read', false) - ->addOption('useUserId', null, InputArgument::OPTIONAL, 'Use user id instead of username to find account', false) + ->addOption('disableContentUpdate', null, InputOption::VALUE_NONE, 'Disable fetching updated content from URL') ; } @@ -69,6 +70,7 @@ class ImportCommand extends ContainerAwareCommand } $import->setMarkAsRead($input->getOption('markAsRead')); + $import->setDisableContentUpdate($input->getOption('disableContentUpdate')); $import->setUser($user); $res = $import diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index fc462c4cd..167853aae 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -24,6 +24,7 @@ abstract class AbstractImport implements ImportInterface protected $producer; protected $user; protected $markAsRead; + protected $disableContentUpdate; protected $skippedEntries = 0; protected $importedEntries = 0; protected $queuedEntries = 0; @@ -84,6 +85,27 @@ abstract class AbstractImport implements ImportInterface return $this->markAsRead; } + /** + * Set whether articles should be fetched for updated content. + * + * @param bool $markAsRead + */ + public function setDisableContentUpdate($disableContentUpdate) + { + $this->disableContentUpdate = $disableContentUpdate; + + return $this; + } + + /** + * Get whether articles should be fetched for updated content. + */ + public function getDisableContentUpdate() + { + return $this->disableContentUpdate; + } + + /** * Fetch content from the ContentProxy (using graby). * If it fails return the given entry to be saved in all case (to avoid user to loose the content). @@ -95,9 +117,12 @@ abstract class AbstractImport implements ImportInterface protected function fetchContent(Entry $entry, $url, array $content = []) { try { - $this->contentProxy->updateEntry($entry, $url, $content); + $this->contentProxy->importEntry($entry, $content, $this->disableContentUpdate); } catch (\Exception $e) { - return $entry; + $this->logger->error('Error trying to import an entry.', [ + 'entry_url' => $content['url'], + 'error_msg' => $e->getMessage(), + ]); } } diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index 166439387..1ad21d147 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -257,9 +257,8 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); $entry = new Entry(new User()); - $proxy->updateEntry( + $proxy->importEntry( $entry, - 'http://0.0.0.0', [ 'html' => str_repeat('this is my content', 325), 'title' => 'this is my title', @@ -294,7 +293,6 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $entry = new Entry(new User()); $proxy->updateEntry( $entry, - 'http://0.0.0.0', [ 'html' => str_repeat('this is my content', 325), 'title' => 'this is my title', @@ -334,13 +332,14 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); $entry = new Entry(new User()); - $proxy->updateEntry($entry, 'http://0.0.0.0', [ + $content = array( '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', - ]); + ); + $proxy->importEntry($entry, $content, true); $this->assertCount(0, $entry->getTags()); } diff --git a/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php b/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php index cec195341..7a15e9187 100644 --- a/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php @@ -89,7 +89,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('updateEntry') + ->method('importEntry') ->willReturn($entry); $res = $chromeImport->import(); @@ -118,7 +118,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('updateEntry') + ->method('importEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php b/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php index c186c8202..09abac57e 100644 --- a/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php @@ -89,7 +89,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('updateEntry') + ->method('importEntry') ->willReturn($entry); $res = $firefoxImport->import(); @@ -118,7 +118,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('updateEntry') + ->method('importEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php b/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php index 9158c8a23..05844490d 100644 --- a/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php @@ -104,7 +104,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(4)) - ->method('updateEntry') + ->method('importEntry') ->willReturn($entry); $res = $instapaperImport->import(); @@ -133,7 +133,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->once()) - ->method('updateEntry') + ->method('importEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -173,7 +173,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -213,7 +213,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php index b81ebe15f..f75e6bea0 100644 --- a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php @@ -282,7 +282,7 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->once()) - ->method('updateEntry') + ->method('importEntry') ->willReturn($entry); $pocketImport->setClient($client); @@ -377,7 +377,7 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('updateEntry') + ->method('importEntry') ->willReturn($entry); $pocketImport->setClient($client); @@ -450,7 +450,7 @@ JSON; $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -536,7 +536,7 @@ JSON; $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('ImportEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); @@ -621,7 +621,7 @@ JSON; $this->contentProxy ->expects($this->once()) - ->method('updateEntry') + ->method('importEntry') ->will($this->throwException(new \Exception())); $pocketImport->setClient($client); diff --git a/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php b/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php index 8f466d383..1b0daa92d 100644 --- a/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php @@ -89,7 +89,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(3)) - ->method('updateEntry') + ->method('importEntry') ->willReturn($entry); $res = $readabilityImport->import(); @@ -118,7 +118,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('updateEntry') + ->method('importEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php index 7cbef6377..f23cb7489 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php @@ -104,7 +104,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('updateEntry') + ->method('importEntry') ->willReturn($entry); $res = $wallabagV1Import->import(); @@ -133,7 +133,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(3)) - ->method('updateEntry') + ->method('importEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -173,7 +173,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -213,7 +213,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php index 5cc04aa59..e1acf5699 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php @@ -100,7 +100,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('updateEntry') + ->method('importEntry') ->willReturn(new Entry($this->user)); $res = $wallabagV2Import->import(); @@ -129,7 +129,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('updateEntry') + ->method('importEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -165,7 +165,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -201,7 +201,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('updateEntry'); + ->method('importEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); @@ -278,7 +278,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('updateEntry') + ->method('importEntry') ->will($this->throwException(new \Exception())); $res = $wallabagV2Import->import(); From 704803e182068923eba16f3dc451e9231f15ecb5 Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Fri, 16 Dec 2016 09:46:21 -0500 Subject: [PATCH 04/11] Replace Wallabag v1 error strings with v2 strings --- .../ImportBundle/Import/WallabagV1Import.php | 18 +++++++++++++++--- .../ImportBundle/Resources/config/services.yml | 2 ++ .../Import/WallabagV1ImportTest.php | 11 ++++++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php index 59e3ce026..872fd642f 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php +++ b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php @@ -4,6 +4,17 @@ namespace Wallabag\ImportBundle\Import; class WallabagV1Import extends WallabagImport { + protected $fetchingErrorMessage; + protected $fetchingErrorMessageTitle; + + public function __construct($em, $contentProxy, $eventDispatcher, $fetchingErrorMessageTitle, $fetchingErrorMessage) + { + $this->fetchingErrorMessageTitle = $fetchingErrorMessageTitle; + $this->fetchingErrorMessage = $fetchingErrorMessage; + + parent::__construct($em, $contentProxy, $eventDispatcher); + } + /** * {@inheritdoc} */ @@ -43,10 +54,11 @@ class WallabagV1Import extends WallabagImport 'created_at' => '', ]; - // force content to be refreshed in case on bad fetch in the v1 installation + // In case of a bad fetch in v1, replace title and content with v2 error strings + // If fetching fails again, they will get this instead of the v1 strings if (in_array($entry['title'], $this->untitled)) { - $data['title'] = ''; - $data['html'] = ''; + $data['title'] = $this->fetchingErrorMessageTitle; + $data['html'] = $this->fetchingErrorMessage; } if (array_key_exists('tags', $entry) && $entry['tags'] != '') { diff --git a/src/Wallabag/ImportBundle/Resources/config/services.yml b/src/Wallabag/ImportBundle/Resources/config/services.yml index 661dc7e1d..b224a6a23 100644 --- a/src/Wallabag/ImportBundle/Resources/config/services.yml +++ b/src/Wallabag/ImportBundle/Resources/config/services.yml @@ -35,6 +35,8 @@ services: - "@wallabag_core.content_proxy" - "@wallabag_core.tags_assigner" - "@event_dispatcher" + - "%wallabag_core.fetching_error_message_title%" + - "%wallabag_core.fetching_error_message%" calls: - [ setLogger, [ "@logger" ]] tags: diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php index f23cb7489..3b2375a19 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php @@ -19,6 +19,8 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase protected $contentProxy; protected $tagsAssigner; protected $uow; + protected $fetchingErrorMessageTitle = 'No title found'; + protected $fetchingErrorMessage = 'wallabag can\'t retrieve contents for this article. Please troubleshoot this issue.'; private function getWallabagV1Import($unsetUser = false, $dispatched = 0) { @@ -58,7 +60,14 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase ->expects($this->exactly($dispatched)) ->method('dispatch'); - $wallabag = new WallabagV1Import($this->em, $this->contentProxy, $this->tagsAssigner, $dispatcher); + $wallabag = new WallabagV1Import( + $this->em, + $this->contentProxy, + $this->tagsAssigner, + $dispatcher, + $this->fetchingErrorMessageTitle, + $this->fetchingErrorMessage + ); $this->logHandler = new TestHandler(); $logger = new Logger('test', [$this->logHandler]); From 432a24f5028446f1bc5c184905f8406bbef8bf05 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 30 May 2017 16:21:25 +0200 Subject: [PATCH 05/11] CS --- src/Wallabag/ImportBundle/Import/AbstractImport.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index 167853aae..1f9042923 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -88,7 +88,7 @@ abstract class AbstractImport implements ImportInterface /** * Set whether articles should be fetched for updated content. * - * @param bool $markAsRead + * @param bool $disableContentUpdate */ public function setDisableContentUpdate($disableContentUpdate) { @@ -105,7 +105,6 @@ abstract class AbstractImport implements ImportInterface return $this->disableContentUpdate; } - /** * Fetch content from the ContentProxy (using graby). * If it fails return the given entry to be saved in all case (to avoid user to loose the content). From d5c2cc54b5490b0bec46f39d7706d5d46869e872 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 30 May 2017 17:48:24 +0200 Subject: [PATCH 06/11] Fix tests --- .../Controller/EntryRestController.php | 45 ++++++++++--------- .../CoreBundle/Helper/ContentProxy.php | 27 +++++++---- .../ImportBundle/Command/ImportCommand.php | 1 + .../ImportBundle/Import/AbstractImport.php | 5 ++- .../ImportBundle/Import/WallabagV1Import.php | 4 +- .../CoreBundle/Helper/ContentProxyTest.php | 10 ++++- 6 files changed, 57 insertions(+), 35 deletions(-) diff --git a/src/Wallabag/ApiBundle/Controller/EntryRestController.php b/src/Wallabag/ApiBundle/Controller/EntryRestController.php index d154c18b3..93c8157e7 100644 --- a/src/Wallabag/ApiBundle/Controller/EntryRestController.php +++ b/src/Wallabag/ApiBundle/Controller/EntryRestController.php @@ -231,7 +231,6 @@ class EntryRestController extends WallabagRestController $this->validateAuthentication(); $urls = json_decode($request->query->get('urls', [])); - $results = []; $limit = $this->container->getParameter('wallabag_core.api_limit_mass_actions'); @@ -239,32 +238,34 @@ class EntryRestController extends WallabagRestController throw new HttpException(400, 'API limit reached'); } + $results = []; + if (empty($urls)) { + return $this->sendResponse($results); + } + // handle multiple urls - if (!empty($urls)) { - foreach ($urls as $key => $url) { - $entry = $this->get('wallabag_core.entry_repository')->findByUrlAndUserId( - $url, - $this->getUser()->getId() - ); + foreach ($urls as $key => $url) { + $entry = $this->get('wallabag_core.entry_repository')->findByUrlAndUserId( + $url, + $this->getUser()->getId() + ); - $results[$key]['url'] = $url; + $results[$key]['url'] = $url; - if (false === $entry) { - $entry = $this->get('wallabag_core.content_proxy')->updateEntry( - new Entry($this->getUser()), - $url - ); - } + if (false === $entry) { + $entry = new Entry($this->getUser()); - $em = $this->getDoctrine()->getManager(); - $em->persist($entry); - $em->flush(); - - $results[$key]['entry'] = $entry instanceof Entry ? $entry->getId() : false; - - // entry saved, dispatch event about it! - $this->get('event_dispatcher')->dispatch(EntrySavedEvent::NAME, new EntrySavedEvent($entry)); + $this->get('wallabag_core.content_proxy')->updateEntry($entry, $url); } + + $em = $this->getDoctrine()->getManager(); + $em->persist($entry); + $em->flush(); + + $results[$key]['entry'] = $entry instanceof Entry ? $entry->getId() : false; + + // entry saved, dispatch event about it! + $this->get('event_dispatcher')->dispatch(EntrySavedEvent::NAME, new EntrySavedEvent($entry)); } return $this->sendResponse($results); diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 88873bd53..cd18c668f 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -34,13 +34,17 @@ class ContentProxy /** * Update existing entry by fetching from URL using Graby. * - * @param Entry $entry Entry to update - * @param string $url Url to grab content for + * @param Entry $entry Entry to update + * @param string $url Url to grab content for */ public function updateEntry(Entry $entry, $url) { $content = $this->graby->fetchContent($url); + // be sure to keep the url in case of error + // so we'll be able to refetch it in the future + $content['url'] = $content['url'] ?: $url; + $this->stockEntry($entry, $content); } @@ -53,7 +57,14 @@ class ContentProxy */ public function importEntry(Entry $entry, array $content, $disableContentUpdate = false) { - $this->validateContent($content); + try { + $this->validateContent($content); + } catch (\Exception $e) { + // validation failed but do we want to disable updating content? + if (true === $disableContentUpdate) { + throw $e; + } + } if (false === $disableContentUpdate) { try { @@ -79,8 +90,8 @@ class ContentProxy * Stock entry with fetched or imported content. * Will fall back to OpenGraph data if available. * - * @param Entry $entry Entry to stock - * @param array $content Array with at least title and URL + * @param Entry $entry Entry to stock + * @param array $content Array with at least title and URL */ private function stockEntry(Entry $entry, array $content) { @@ -162,15 +173,15 @@ class ContentProxy */ private function validateContent(array $content) { - if (!empty($content['title']))) { + if (empty($content['title'])) { throw new Exception('Missing title from imported entry!'); } - if (!empty($content['url']))) { + if (empty($content['url'])) { throw new Exception('Missing URL from imported entry!'); } - if (!empty($content['html']))) { + if (empty($content['html'])) { throw new Exception('Missing html from imported entry!'); } } diff --git a/src/Wallabag/ImportBundle/Command/ImportCommand.php b/src/Wallabag/ImportBundle/Command/ImportCommand.php index bca800e6c..0829a1da3 100644 --- a/src/Wallabag/ImportBundle/Command/ImportCommand.php +++ b/src/Wallabag/ImportBundle/Command/ImportCommand.php @@ -20,6 +20,7 @@ class ImportCommand extends ContainerAwareCommand ->addArgument('filepath', InputArgument::REQUIRED, 'Path to the JSON file') ->addOption('importer', null, InputArgument::OPTIONAL, 'The importer to use: v1, v2, instapaper, pinboard, readability, firefox or chrome', 'v1') ->addOption('markAsRead', null, InputArgument::OPTIONAL, 'Mark all entries as read', false) + ->addOption('useUserId', null, InputArgument::OPTIONAL, 'Use user id instead of username to find account', false) ->addOption('disableContentUpdate', null, InputOption::VALUE_NONE, 'Disable fetching updated content from URL') ; } diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index 1f9042923..bf568a1af 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -24,7 +24,7 @@ abstract class AbstractImport implements ImportInterface protected $producer; protected $user; protected $markAsRead; - protected $disableContentUpdate; + protected $disableContentUpdate = false; protected $skippedEntries = 0; protected $importedEntries = 0; protected $queuedEntries = 0; @@ -115,6 +115,9 @@ abstract class AbstractImport implements ImportInterface */ protected function fetchContent(Entry $entry, $url, array $content = []) { + // be sure to set at least the given url + $content['url'] = isset($content['url']) ? $content['url'] : $url; + try { $this->contentProxy->importEntry($entry, $content, $this->disableContentUpdate); } catch (\Exception $e) { diff --git a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php index 872fd642f..1f0df646b 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php +++ b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php @@ -7,12 +7,12 @@ class WallabagV1Import extends WallabagImport protected $fetchingErrorMessage; protected $fetchingErrorMessageTitle; - public function __construct($em, $contentProxy, $eventDispatcher, $fetchingErrorMessageTitle, $fetchingErrorMessage) + public function __construct($em, $contentProxy, $tagsAssigner, $eventDispatcher, $fetchingErrorMessageTitle, $fetchingErrorMessage) { $this->fetchingErrorMessageTitle = $fetchingErrorMessageTitle; $this->fetchingErrorMessage = $fetchingErrorMessage; - parent::__construct($em, $contentProxy, $eventDispatcher); + parent::__construct($em, $contentProxy, $tagsAssigner, $eventDispatcher); } /** diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index 1ad21d147..be287d847 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -3,6 +3,8 @@ namespace Tests\Wallabag\CoreBundle\Helper; use Psr\Log\NullLogger; +use Monolog\Logger; +use Monolog\Handler\TestHandler; use Wallabag\CoreBundle\Helper\ContentProxy; use Wallabag\CoreBundle\Entity\Entry; use Wallabag\CoreBundle\Entity\Tag; @@ -197,7 +199,8 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ]); $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry(new Entry(new User()), 'http://0.0.0.0'); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); $this->assertEquals('http://1.1.1.1', $entry->getUrl()); $this->assertEquals('this is my title', $entry->getTitle()); @@ -255,7 +258,10 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $tagger->expects($this->once()) ->method('tag'); - $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); + $logHandler = new TestHandler(); + $logger = new Logger('test', array($logHandler)); + + $proxy = new ContentProxy((new Graby()), $tagger, $logger, $this->fetchingErrorMessage); $entry = new Entry(new User()); $proxy->importEntry( $entry, From 843182c7cf428b5f6b8a1ff7057adc703c1e816e Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 09:52:09 +0200 Subject: [PATCH 07/11] CS --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index cd18c668f..e62927443 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -51,9 +51,9 @@ class ContentProxy /** * Import entry using either fetched or provided content. * - * @param Entry $entry Entry to update - * @param array $content Array with content provided for import with AT LEAST keys title, html, url to skip the fetchContent from the url - * @param bool $disableContentUpdate Whether to skip trying to fetch content using Graby + * @param Entry $entry Entry to update + * @param array $content Array with content provided for import with AT LEAST keys title, html, url to skip the fetchContent from the url + * @param bool $disableContentUpdate Whether to skip trying to fetch content using Graby */ public function importEntry(Entry $entry, array $content, $disableContentUpdate = false) { From 6acadf8e98cf6021a9019773df75bdb151865687 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 11:31:45 +0200 Subject: [PATCH 08/11] Rewrote code & fix tests --- .../CoreBundle/Helper/ContentProxy.php | 66 ++++++------------- .../ImportBundle/Import/AbstractImport.php | 7 +- .../Controller/TagControllerTest.php | 2 +- .../CoreBundle/Helper/ContentProxyTest.php | 37 +++++------ .../ImportBundle/Import/ChromeImportTest.php | 8 +-- .../ImportBundle/Import/FirefoxImportTest.php | 8 +-- .../Import/InstapaperImportTest.php | 8 +-- .../ImportBundle/Import/PocketImportTest.php | 10 +-- .../Import/ReadabilityImportTest.php | 8 +-- .../Import/WallabagV1ImportTest.php | 8 +-- .../Import/WallabagV2ImportTest.php | 10 +-- 11 files changed, 71 insertions(+), 101 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index e62927443..3024fdc50 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -7,7 +7,6 @@ use Psr\Log\LoggerInterface; use Wallabag\CoreBundle\Entity\Entry; use Wallabag\CoreBundle\Tools\Utils; use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeExtensionGuesser; -use Symfony\Component\Config\Definition\Exception\Exception; /** * This kind of proxy class take care of getting the content from an url @@ -32,57 +31,40 @@ class ContentProxy } /** - * Update existing entry by fetching from URL using Graby. + * Update entry using either fetched or provided content. * - * @param Entry $entry Entry to update - * @param string $url Url to grab content for + * @param Entry $entry Entry to update + * @param string $url Url of the content + * @param array $content Array with content provided for import with AT LEAST keys title, html, url to skip the fetchContent from the url + * @param bool $disableContentUpdate Whether to skip trying to fetch content using Graby */ - public function updateEntry(Entry $entry, $url) + public function updateEntry(Entry $entry, $url, array $content = [], $disableContentUpdate = false) { - $content = $this->graby->fetchContent($url); - - // be sure to keep the url in case of error - // so we'll be able to refetch it in the future - $content['url'] = $content['url'] ?: $url; - - $this->stockEntry($entry, $content); - } - - /** - * Import entry using either fetched or provided content. - * - * @param Entry $entry Entry to update - * @param array $content Array with content provided for import with AT LEAST keys title, html, url to skip the fetchContent from the url - * @param bool $disableContentUpdate Whether to skip trying to fetch content using Graby - */ - public function importEntry(Entry $entry, array $content, $disableContentUpdate = false) - { - try { - $this->validateContent($content); - } catch (\Exception $e) { - // validation failed but do we want to disable updating content? - if (true === $disableContentUpdate) { - throw $e; - } + if (!empty($content['html'])) { + $content['html'] = $this->graby->cleanupHtml($content['html'], $url); } - if (false === $disableContentUpdate) { + if ((empty($content) || false === $this->validateContent($content)) && false === $disableContentUpdate) { try { - $fetchedContent = $this->graby->fetchContent($content['url']); + $fetchedContent = $this->graby->fetchContent($url); } catch (\Exception $e) { $this->logger->error('Error while trying to fetch content from URL.', [ - 'entry_url' => $content['url'], + 'entry_url' => $url, 'error_msg' => $e->getMessage(), ]); } // when content is imported, we have information in $content // in case fetching content goes bad, we'll keep the imported information instead of overriding them - if ($fetchedContent['html'] !== $this->fetchingErrorMessage) { + if (empty($content) || $fetchedContent['html'] !== $this->fetchingErrorMessage) { $content = $fetchedContent; } } + // be sure to keep the url in case of error + // so we'll be able to refetch it in the future + $content['url'] = !empty($content['url']) ? $content['url'] : $url; + $this->stockEntry($entry, $content); } @@ -126,7 +108,7 @@ class ContentProxy try { $entry->setPublishedAt(new \DateTime($date)); } catch (\Exception $e) { - $this->logger->warning('Error while defining date', ['e' => $e, 'url' => $url, 'date' => $content['date']]); + $this->logger->warning('Error while defining date', ['e' => $e, 'url' => $content['url'], 'date' => $content['date']]); } } @@ -170,19 +152,11 @@ class ContentProxy * Validate that the given content has at least a title, an html and a url. * * @param array $content + * + * @return bool true if valid otherwise false */ private function validateContent(array $content) { - if (empty($content['title'])) { - throw new Exception('Missing title from imported entry!'); - } - - if (empty($content['url'])) { - throw new Exception('Missing URL from imported entry!'); - } - - if (empty($content['html'])) { - throw new Exception('Missing html from imported entry!'); - } + return !empty($content['title']) && !empty($content['html']) && !empty($content['url']); } } diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index bf568a1af..9f3d822ad 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -115,14 +115,11 @@ abstract class AbstractImport implements ImportInterface */ protected function fetchContent(Entry $entry, $url, array $content = []) { - // be sure to set at least the given url - $content['url'] = isset($content['url']) ? $content['url'] : $url; - try { - $this->contentProxy->importEntry($entry, $content, $this->disableContentUpdate); + $this->contentProxy->updateEntry($entry, $url, $content, $this->disableContentUpdate); } catch (\Exception $e) { $this->logger->error('Error trying to import an entry.', [ - 'entry_url' => $content['url'], + 'entry_url' => $url, 'error_msg' => $e->getMessage(), ]); } diff --git a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php index f9bf7b878..af1ad7af1 100644 --- a/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/TagControllerTest.php @@ -123,7 +123,7 @@ class TagControllerTest extends WallabagCoreTestCase $this->assertEquals(302, $client->getResponse()->getStatusCode()); $this->assertEquals($entryUri, $client->getResponse()->getTargetUrl()); - // re-retrieve the entry to be sure to get fresh data from database (mostly for tags) + // re-retrieve the entry to be sure to get fresh data from database (mostly for tags) $entry = $this->getEntityManager()->getRepository(Entry::class)->find($entry->getId()); $this->assertNotContains($this->tagName, $entry->getTags()); diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index be287d847..a3570125e 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -11,8 +11,6 @@ use Wallabag\CoreBundle\Entity\Tag; use Wallabag\UserBundle\Entity\User; use Wallabag\CoreBundle\Helper\RuleBasedTagger; use Graby\Graby; -use Monolog\Handler\TestHandler; -use Monolog\Logger; class ContentProxyTest extends \PHPUnit_Framework_TestCase { @@ -259,12 +257,13 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->method('tag'); $logHandler = new TestHandler(); - $logger = new Logger('test', array($logHandler)); + $logger = new Logger('test', [$logHandler]); $proxy = new ContentProxy((new Graby()), $tagger, $logger, $this->fetchingErrorMessage); $entry = new Entry(new User()); - $proxy->importEntry( + $proxy->updateEntry( $entry, + 'http://1.1.1.1', [ 'html' => str_repeat('this is my content', 325), 'title' => 'this is my title', @@ -299,6 +298,7 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $entry = new Entry(new User()); $proxy->updateEntry( $entry, + 'http://1.1.1.1', [ 'html' => str_repeat('this is my content', 325), 'title' => 'this is my title', @@ -326,26 +326,24 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase public function testTaggerThrowException() { - $graby = $this->getMockBuilder('Graby\Graby') - ->disableOriginalConstructor() - ->getMock(); - $tagger = $this->getTaggerMock(); $tagger->expects($this->once()) ->method('tag') ->will($this->throwException(new \Exception())); - $proxy = new ContentProxy($graby, $tagger, $this->getLogger(), $this->fetchingErrorMessage); - + $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); $entry = new Entry(new User()); - $content = array( - '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', + $proxy->updateEntry( + $entry, + 'http://1.1.1.1', + [ + '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', + ] ); - $proxy->importEntry($entry, $content, true); $this->assertCount(0, $entry->getTags()); } @@ -374,8 +372,9 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->method('tag'); $proxy = new ContentProxy((new Graby()), $tagger, $this->getLogger(), $this->fetchingErrorMessage); - $entry = $proxy->updateEntry( - new Entry(new User()), + $entry = new Entry(new User()); + $proxy->updateEntry( + $entry, 'http://1.1.1.1', [ 'html' => $html, diff --git a/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php b/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php index 7a15e9187..cec195341 100644 --- a/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/ChromeImportTest.php @@ -89,7 +89,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $chromeImport->import(); @@ -118,7 +118,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class ChromeImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php b/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php index 09abac57e..c186c8202 100644 --- a/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/FirefoxImportTest.php @@ -89,7 +89,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $firefoxImport->import(); @@ -118,7 +118,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class FirefoxImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php b/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php index 05844490d..9158c8a23 100644 --- a/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/InstapaperImportTest.php @@ -104,7 +104,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(4)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $instapaperImport->import(); @@ -133,7 +133,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->once()) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -173,7 +173,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -213,7 +213,7 @@ class InstapaperImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php index f75e6bea0..b81ebe15f 100644 --- a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php @@ -282,7 +282,7 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->once()) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $pocketImport->setClient($client); @@ -377,7 +377,7 @@ class PocketImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $pocketImport->setClient($client); @@ -450,7 +450,7 @@ JSON; $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -536,7 +536,7 @@ JSON; $this->contentProxy ->expects($this->never()) - ->method('ImportEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); @@ -621,7 +621,7 @@ JSON; $this->contentProxy ->expects($this->once()) - ->method('importEntry') + ->method('updateEntry') ->will($this->throwException(new \Exception())); $pocketImport->setClient($client); diff --git a/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php b/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php index 1b0daa92d..8f466d383 100644 --- a/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/ReadabilityImportTest.php @@ -89,7 +89,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(3)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $readabilityImport->import(); @@ -118,7 +118,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -158,7 +158,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -198,7 +198,7 @@ class ReadabilityImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php index 3b2375a19..834b7ef57 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV1ImportTest.php @@ -113,7 +113,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(1)) - ->method('importEntry') + ->method('updateEntry') ->willReturn($entry); $res = $wallabagV1Import->import(); @@ -142,7 +142,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(3)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -182,7 +182,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -222,7 +222,7 @@ class WallabagV1ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php index e1acf5699..5cc04aa59 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php @@ -100,7 +100,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); $res = $wallabagV2Import->import(); @@ -129,7 +129,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->willReturn(new Entry($this->user)); // check that every entry persisted are archived @@ -165,7 +165,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $producer = $this->getMockBuilder('OldSound\RabbitMqBundle\RabbitMq\Producer') ->disableOriginalConstructor() @@ -201,7 +201,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->never()) - ->method('importEntry'); + ->method('updateEntry'); $factory = new RedisMockFactory(); $redisMock = $factory->getAdapter('Predis\Client', true); @@ -278,7 +278,7 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $this->contentProxy ->expects($this->exactly(2)) - ->method('importEntry') + ->method('updateEntry') ->will($this->throwException(new \Exception())); $res = $wallabagV2Import->import(); From ec970721525d9a4761a30ee417406cf8ad8bb171 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 11:45:02 +0200 Subject: [PATCH 09/11] No need to catch that Exception --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 3024fdc50..bfaa19769 100644 --- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php +++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php @@ -45,14 +45,7 @@ class ContentProxy } if ((empty($content) || false === $this->validateContent($content)) && false === $disableContentUpdate) { - try { - $fetchedContent = $this->graby->fetchContent($url); - } catch (\Exception $e) { - $this->logger->error('Error while trying to fetch content from URL.', [ - 'entry_url' => $url, - 'error_msg' => $e->getMessage(), - ]); - } + $fetchedContent = $this->graby->fetchContent($url); // when content is imported, we have information in $content // in case fetching content goes bad, we'll keep the imported information instead of overriding them @@ -73,7 +66,7 @@ class ContentProxy * Will fall back to OpenGraph data if available. * * @param Entry $entry Entry to stock - * @param array $content Array with at least title and URL + * @param array $content Array with at least title, url & html */ private function stockEntry(Entry $entry, array $content) { From 701d3066fbac504beb25ab7a13546ad155e65488 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 12:46:07 +0200 Subject: [PATCH 10/11] We don't need that getter --- src/Wallabag/ImportBundle/Import/AbstractImport.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index 9f3d822ad..9b6242967 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -97,14 +97,6 @@ abstract class AbstractImport implements ImportInterface return $this; } - /** - * Get whether articles should be fetched for updated content. - */ - public function getDisableContentUpdate() - { - return $this->disableContentUpdate; - } - /** * Fetch content from the ContentProxy (using graby). * If it fails return the given entry to be saved in all case (to avoid user to loose the content). From f5924e954730efdb7b9fadf23c0b73b3f5a0a434 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 15:44:36 +0200 Subject: [PATCH 11/11] Fix option attributes --- src/Wallabag/ImportBundle/Command/ImportCommand.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Wallabag/ImportBundle/Command/ImportCommand.php b/src/Wallabag/ImportBundle/Command/ImportCommand.php index 0829a1da3..5f1ab0af0 100644 --- a/src/Wallabag/ImportBundle/Command/ImportCommand.php +++ b/src/Wallabag/ImportBundle/Command/ImportCommand.php @@ -18,9 +18,9 @@ class ImportCommand extends ContainerAwareCommand ->setDescription('Import entries from a JSON export') ->addArgument('username', InputArgument::REQUIRED, 'User to populate') ->addArgument('filepath', InputArgument::REQUIRED, 'Path to the JSON file') - ->addOption('importer', null, InputArgument::OPTIONAL, 'The importer to use: v1, v2, instapaper, pinboard, readability, firefox or chrome', 'v1') - ->addOption('markAsRead', null, InputArgument::OPTIONAL, 'Mark all entries as read', false) - ->addOption('useUserId', null, InputArgument::OPTIONAL, 'Use user id instead of username to find account', false) + ->addOption('importer', null, InputOption::VALUE_OPTIONAL, 'The importer to use: v1, v2, instapaper, pinboard, readability, firefox or chrome', 'v1') + ->addOption('markAsRead', null, InputOption::VALUE_OPTIONAL, 'Mark all entries as read', false) + ->addOption('useUserId', null, InputOption::VALUE_NONE, 'Use user id instead of username to find account') ->addOption('disableContentUpdate', null, InputOption::VALUE_NONE, 'Disable fetching updated content from URL') ; }