From 7aba665e484c5c36ee029219a999a427d864ff22 Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Tue, 6 Dec 2016 22:17:44 -0500 Subject: 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. --- src/Wallabag/ApiBundle/Controller/EntryRestController.php | 4 ++-- src/Wallabag/CoreBundle/Controller/EntryController.php | 8 ++------ src/Wallabag/CoreBundle/Helper/ContentProxy.php | 4 ---- src/Wallabag/ImportBundle/Import/AbstractImport.php | 4 +--- src/Wallabag/ImportBundle/Import/BrowserImport.php | 2 +- src/Wallabag/ImportBundle/Import/InstapaperImport.php | 2 +- src/Wallabag/ImportBundle/Import/PinboardImport.php | 2 +- src/Wallabag/ImportBundle/Import/PocketImport.php | 2 +- src/Wallabag/ImportBundle/Import/ReadabilityImport.php | 2 +- src/Wallabag/ImportBundle/Import/WallabagImport.php | 2 +- 10 files changed, 11 insertions(+), 21 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/ApiBundle/Controller/EntryRestController.php b/src/Wallabag/ApiBundle/Controller/EntryRestController.php index c3ba1858..d154c18b 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 8d2ac6d4..6018dfac 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 8ba77ca9..c73b8eaf 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 a61388c0..fc462c4c 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 ef0eeb7e..71e65e59 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 c8e0cd5b..3aa12f6f 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 489b9257..110b0464 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 8835161b..c1d5b6da 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 de320d23..002b27f4 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 0e5382cf..c64ccd64 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( -- cgit v1.2.3 From 1c5da417e4ddb14223f9af6e5cea6778e5c0fd08 Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Tue, 6 Dec 2016 22:27:08 -0500 Subject: Put default fetching error title in global config --- src/Wallabag/CoreBundle/Controller/EntryController.php | 3 --- src/Wallabag/CoreBundle/DependencyInjection/Configuration.php | 2 ++ src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php | 1 + src/Wallabag/CoreBundle/Resources/config/services.yml | 1 + 4 files changed, 4 insertions(+), 3 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 6018dfac..4b2bc7da 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 75b37729..8b5b5744 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 c075c19f..a2a703cb 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 a68b2fdc..a9b0d2d5 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: -- cgit v1.2.3 From d0e9b3d640acce49068d1a2c5603b92c1bda363e Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Wed, 7 Dec 2016 15:16:49 -0500 Subject: 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. --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 76 +++++++++++++++------- .../ImportBundle/Command/ImportCommand.php | 4 +- .../ImportBundle/Import/AbstractImport.php | 29 ++++++++- 3 files changed, 84 insertions(+), 25 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index c73b8eaf..88873bd5 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); + + $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); - // 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); + 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 ce72837a..bca800e6 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 fc462c4c..167853aa 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(), + ]); } } -- cgit v1.2.3 From 704803e182068923eba16f3dc451e9231f15ecb5 Mon Sep 17 00:00:00 2001 From: Jerome Charaoui Date: Fri, 16 Dec 2016 09:46:21 -0500 Subject: Replace Wallabag v1 error strings with v2 strings --- src/Wallabag/ImportBundle/Import/WallabagV1Import.php | 18 +++++++++++++++--- .../ImportBundle/Resources/config/services.yml | 2 ++ 2 files changed, 17 insertions(+), 3 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php index 59e3ce02..872fd642 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 661dc7e1..b224a6a2 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: -- cgit v1.2.3 From 432a24f5028446f1bc5c184905f8406bbef8bf05 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 30 May 2017 16:21:25 +0200 Subject: CS --- src/Wallabag/ImportBundle/Import/AbstractImport.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index 167853aa..1f904292 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). -- cgit v1.2.3 From d5c2cc54b5490b0bec46f39d7706d5d46869e872 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 30 May 2017 17:48:24 +0200 Subject: Fix tests --- .../ApiBundle/Controller/EntryRestController.php | 45 +++++++++++----------- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 27 +++++++++---- .../ImportBundle/Command/ImportCommand.php | 1 + .../ImportBundle/Import/AbstractImport.php | 5 ++- .../ImportBundle/Import/WallabagV1Import.php | 4 +- 5 files changed, 49 insertions(+), 33 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/ApiBundle/Controller/EntryRestController.php b/src/Wallabag/ApiBundle/Controller/EntryRestController.php index d154c18b..93c8157e 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() - ); - - $results[$key]['url'] = $url; - - if (false === $entry) { - $entry = $this->get('wallabag_core.content_proxy')->updateEntry( - new Entry($this->getUser()), - $url - ); - } + foreach ($urls as $key => $url) { + $entry = $this->get('wallabag_core.entry_repository')->findByUrlAndUserId( + $url, + $this->getUser()->getId() + ); - $em = $this->getDoctrine()->getManager(); - $em->persist($entry); - $em->flush(); + $results[$key]['url'] = $url; - $results[$key]['entry'] = $entry instanceof Entry ? $entry->getId() : false; + if (false === $entry) { + $entry = new Entry($this->getUser()); - // 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 88873bd5..cd18c668 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 bca800e6..0829a1da 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 1f904292..bf568a1a 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 872fd642..1f0df646 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); } /** -- cgit v1.2.3 From 843182c7cf428b5f6b8a1ff7057adc703c1e816e Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 09:52:09 +0200 Subject: CS --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index cd18c668..e6292744 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) { -- cgit v1.2.3 From 6acadf8e98cf6021a9019773df75bdb151865687 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 11:31:45 +0200 Subject: Rewrote code & fix tests --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 66 +++++++--------------- .../ImportBundle/Import/AbstractImport.php | 7 +-- 2 files changed, 22 insertions(+), 51 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index e6292744..3024fdc5 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 bf568a1a..9f3d822a 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(), ]); } -- cgit v1.2.3 From ec970721525d9a4761a30ee417406cf8ad8bb171 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 11:45:02 +0200 Subject: No need to catch that Exception --- src/Wallabag/CoreBundle/Helper/ContentProxy.php | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php index 3024fdc5..bfaa1976 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) { -- cgit v1.2.3 From 701d3066fbac504beb25ab7a13546ad155e65488 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 12:46:07 +0200 Subject: We don't need that getter --- src/Wallabag/ImportBundle/Import/AbstractImport.php | 8 -------- 1 file changed, 8 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index 9f3d822a..9b624296 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). -- cgit v1.2.3 From f5924e954730efdb7b9fadf23c0b73b3f5a0a434 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Thu, 1 Jun 2017 15:44:36 +0200 Subject: Fix option attributes --- src/Wallabag/ImportBundle/Command/ImportCommand.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/Wallabag') diff --git a/src/Wallabag/ImportBundle/Command/ImportCommand.php b/src/Wallabag/ImportBundle/Command/ImportCommand.php index 0829a1da..5f1ab0af 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') ; } -- cgit v1.2.3