From 59b97fae996d8307b9d957d210d46200f6d206bf Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sat, 17 Sep 2016 07:40:56 +0200 Subject: [PATCH] Avoid losing entry when fetching fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of just say “Failed to save entry” we’ll save the entry at all cost and try to fetch content. If fetching content failed, the entry will still be saved at least, but without content. --- .../CoreBundle/Controller/EntryController.php | 50 +++++++++++-------- .../Resources/translations/messages.da.yml | 4 +- .../Resources/translations/messages.de.yml | 4 +- .../Resources/translations/messages.en.yml | 4 +- .../Resources/translations/messages.es.yml | 4 +- .../Resources/translations/messages.fa.yml | 4 +- .../Resources/translations/messages.fr.yml | 4 +- .../Resources/translations/messages.it.yml | 4 +- .../Resources/translations/messages.oc.yml | 4 +- .../Resources/translations/messages.pl.yml | 4 +- .../Resources/translations/messages.ro.yml | 4 +- .../Resources/translations/messages.tr.yml | 4 +- .../ImportBundle/Import/AbstractImport.php | 6 +-- .../ImportBundle/Import/PocketImport.php | 19 ++----- .../ImportBundle/Import/ReadabilityImport.php | 16 ++---- .../ImportBundle/Import/WallabagImport.php | 16 ++---- .../ImportBundle/Import/PocketImportTest.php | 4 +- .../Import/WallabagV2ImportTest.php | 2 +- 18 files changed, 73 insertions(+), 84 deletions(-) diff --git a/src/Wallabag/CoreBundle/Controller/EntryController.php b/src/Wallabag/CoreBundle/Controller/EntryController.php index 624576b5b..40111af0b 100644 --- a/src/Wallabag/CoreBundle/Controller/EntryController.php +++ b/src/Wallabag/CoreBundle/Controller/EntryController.php @@ -17,26 +17,35 @@ use Sensio\Bundle\FrameworkExtraBundle\Configuration\Cache; class EntryController extends Controller { /** - * @param Entry $entry + * Fetch content and update entry. + * In case it fails, entry will return to avod loosing the data. + * + * @param Entry $entry + * @param string $prefixMessage Should be the translation key: entry_saved or entry_reloaded + * + * @return Entry */ - private function updateEntry(Entry $entry) + 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 { $entry = $this->get('wallabag_core.content_proxy')->updateEntry($entry, $entry->getUrl()); - - $em = $this->getDoctrine()->getManager(); - $em->persist($entry); - $em->flush(); } catch (\Exception $e) { $this->get('logger')->error('Error while saving an entry', [ 'exception' => $e, 'entry' => $entry, ]); - return false; + $message = 'flashes.entry.notice.'.$prefixMessage.'_failed'; } - return true; + $this->get('session')->getFlashBag()->add('notice', $message); + + return $entry; } /** @@ -66,12 +75,11 @@ class EntryController extends Controller return $this->redirect($this->generateUrl('view', ['id' => $existingEntry->getId()])); } - $message = 'flashes.entry.notice.entry_saved'; - if (false === $this->updateEntry($entry)) { - $message = 'flashes.entry.notice.entry_saved_failed'; - } + $this->updateEntry($entry); - $this->get('session')->getFlashBag()->add('notice', $message); + $em = $this->getDoctrine()->getManager(); + $em->persist($entry); + $em->flush(); return $this->redirect($this->generateUrl('homepage')); } @@ -95,6 +103,10 @@ class EntryController extends Controller if (false === $this->checkIfEntryAlreadyExists($entry)) { $this->updateEntry($entry); + + $em = $this->getDoctrine()->getManager(); + $em->persist($entry); + $em->flush(); } return $this->redirect($this->generateUrl('homepage')); @@ -316,15 +328,11 @@ class EntryController extends Controller { $this->checkUserAction($entry); - $message = 'flashes.entry.notice.entry_reloaded'; - if (false === $this->updateEntry($entry)) { - $message = 'flashes.entry.notice.entry_reload_failed'; - } + $this->updateEntry($entry, 'entry_reloaded'); - $this->get('session')->getFlashBag()->add( - 'notice', - $message - ); + $em = $this->getDoctrine()->getManager(); + $em->persist($entry); + $em->flush(); return $this->redirect($this->generateUrl('view', ['id' => $entry->getId()])); } diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.da.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.da.yml index 0a7c6e8cf..9f051edbd 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.da.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.da.yml @@ -414,10 +414,10 @@ flashes: notice: # entry_already_saved: 'Entry already saved on %date%' # entry_saved: 'Entry saved' - # entry_saved_failed: 'Failed to save entry' + # entry_saved_failed: 'Entry saved but fetching content failed' # entry_updated: 'Entry updated' # entry_reloaded: 'Entry reloaded' - # entry_reload_failed: 'Failed to reload entry' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Artikel arkiveret' entry_unarchived: 'Artikel ikke længere arkiveret' entry_starred: 'Artikel markeret som favorit' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.de.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.de.yml index a400686ea..cbfacd553 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.de.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.de.yml @@ -414,10 +414,10 @@ flashes: notice: entry_already_saved: 'Eintrag bereits am %date% gespeichert' entry_saved: 'Eintrag gespeichert' - # entry_saved_failed: 'Failed to save entry' + # entry_saved_failed: 'Entry saved but fetching content failed' entry_updated: 'Eintrag aktualisiert' entry_reloaded: 'Eintrag neugeladen' - entry_reload_failed: 'Neuladen des Eintrags fehlgeschlagen' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Artikel archiviert' entry_unarchived: 'Artikel dearchiviert' entry_starred: 'Artikel favorisiert' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.en.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.en.yml index 2d097cafd..21e2405ca 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.en.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.en.yml @@ -416,10 +416,10 @@ flashes: notice: entry_already_saved: 'Entry already saved on %date%' entry_saved: 'Entry saved' - entry_saved_failed: 'Failed to save entry' + entry_saved_failed: 'Entry saved but fetching content failed' entry_updated: 'Entry updated' entry_reloaded: 'Entry reloaded' - entry_reload_failed: 'Failed to reload entry' + entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Entry archived' entry_unarchived: 'Entry unarchived' entry_starred: 'Entry starred' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.es.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.es.yml index 1dbff0225..43f376d4b 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.es.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.es.yml @@ -414,10 +414,10 @@ flashes: notice: entry_already_saved: 'Entrada ya guardada por %fecha%' entry_saved: 'Entrada guardada' - # entry_saved_failed: 'Failed to save entry' + # entry_saved_failed: 'Entry saved but fetching content failed' entry_updated: 'Entrada actualizada' entry_reloaded: 'Entrada recargada' - entry_reload_failed: 'Entrada recargada reprobada' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Artículo archivado' entry_unarchived: 'Artículo desarchivado' entry_starred: 'Artículo guardado en los favoritos' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.fa.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.fa.yml index ad13c9404..56418ef9e 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.fa.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.fa.yml @@ -414,10 +414,10 @@ flashes: notice: entry_already_saved: 'این مقاله در تاریخ %date% ذخیره شده بود' entry_saved: 'مقاله ذخیره شد' - # entry_saved_failed: 'Failed to save entry' + # entry_saved_failed: 'Entry saved but fetching content failed' entry_updated: 'مقاله به‌روز شد' entry_reloaded: 'مقاله به‌روز شد' - entry_reload_failed: 'به‌روزرسانی مقاله شکست خورد' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'مقاله بایگانی شد' entry_unarchived: 'مقاله از بایگانی درآمد' entry_starred: 'مقاله برگزیده شد' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml index 35e5c9d07..bde21866e 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml @@ -416,10 +416,10 @@ flashes: notice: entry_already_saved: 'Article déjà sauvergardé le %date%' entry_saved: 'Article enregistré' - entry_saved_failed: "L'enregistrement a échoué" + entry_saved_failed: 'Article enregistré mais impossible de récupérer le contenu' entry_updated: 'Article mis à jour' entry_reloaded: 'Article rechargé' - entry_reload_failed: "Le rechargement de l'article a échoué" + entry_reload_failed: "Article mis à jour mais impossible de récupérer le contenu" entry_archived: 'Article marqué comme lu' entry_unarchived: 'Article marqué comme non lu' entry_starred: 'Article ajouté dans les favoris' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.it.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.it.yml index 5bc896c3d..26bb31ba7 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.it.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.it.yml @@ -413,10 +413,10 @@ flashes: notice: entry_already_saved: 'Contenuto già salvato in data %date%' entry_saved: 'Contenuto salvato' - # entry_saved_failed: 'Failed to save entry' + # entry_saved_failed: 'Entry saved but fetching content failed' entry_updated: 'Contenuto aggiornato' entry_reloaded: 'Contenuto ricaricato' - entry_reload_failed: 'Errore nel ricaricamento del contenuto' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Contenuto archiviato' entry_unarchived: 'Contenuto dis-archiviato' entry_starred: 'Contenuto segnato come preferito' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.oc.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.oc.yml index b0194c594..2c4df8670 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.oc.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.oc.yml @@ -414,10 +414,10 @@ flashes: notice: entry_already_saved: 'Article ja salvargardat lo %date%' entry_saved: 'Article enregistrat' - entry_saved_failed: "Fracàs de l'enregistrament de l'entrada" + # entry_saved_failed: 'Entry saved but fetching content failed' entry_updated: 'Article mes a jorn' entry_reloaded: 'Article recargat' - entry_reload_failed: "Fracàs de l'actualizacion de l'article" + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Article marcat coma legit' entry_unarchived: 'Article marcat coma pas legit' entry_starred: 'Article apondut dins los favorits' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.pl.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.pl.yml index 6412ad169..fb8219666 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.pl.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.pl.yml @@ -414,10 +414,10 @@ flashes: notice: entry_already_saved: 'Wpis już został dodany %date%' entry_saved: 'Wpis zapisany' - entry_saved_failed: 'Zapis artykułu się nie powiódł' + # entry_saved_failed: 'Entry saved but fetching content failed' entry_updated: 'Wpis zaktualizowany' entry_reloaded: 'Wpis ponownie załadowany' - entry_reload_failed: 'Błąd ponownego załadowania' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Wpis dodany do archiwum' entry_unarchived: 'Wpis usunięty z archiwum' entry_starred: 'Wpis oznaczony gwiazdką' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.ro.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.ro.yml index 11b744c76..3d22e29d3 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.ro.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.ro.yml @@ -414,10 +414,10 @@ flashes: notice: # entry_already_saved: 'Entry already saved on %date%' # entry_saved: 'Entry saved' - # entry_saved_failed: 'Failed to save entry' + # entry_saved_failed: 'Entry saved but fetching content failed' # entry_updated: 'Entry updated' # entry_reloaded: 'Entry reloaded' - # entry_reload_failed: 'Failed to reload entry' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Articol arhivat' entry_unarchived: 'Articol dezarhivat' entry_starred: 'Articol adăugat la favorite' diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.tr.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.tr.yml index d6aaacfec..5099b002c 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.tr.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.tr.yml @@ -414,10 +414,10 @@ flashes: notice: entry_already_saved: 'Entry already saved on %date%' entry_saved: 'Makale kaydedildi' - # entry_saved_failed: 'Failed to save entry' + # entry_saved_failed: 'Entry saved but fetching content failed' # entry_updated: 'Entry updated' entry_reloaded: 'Makale içeriği yenilendi' - # entry_reload_failed: 'Failed to reload entry' + # entry_reload_failed: 'Entry reloaded but fetching content failed' entry_archived: 'Makale arşivlendi' entry_unarchived: 'Makale arşivden çıkartıldı' entry_starred: 'Makale favorilere eklendi' diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index 2af0e69b4..a1a14576f 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -79,20 +79,20 @@ abstract class AbstractImport implements ImportInterface /** * Fetch content from the ContentProxy (using graby). - * If it fails return false instead of the updated entry. + * If it fails return the given entry to be saved in all case (to avoid user to loose the content). * * @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|false + * @return Entry */ protected function fetchContent(Entry $entry, $url, array $content = []) { try { return $this->contentProxy->updateEntry($entry, $url, $content); } catch (\Exception $e) { - return false; + return $entry; } } diff --git a/src/Wallabag/ImportBundle/Import/PocketImport.php b/src/Wallabag/ImportBundle/Import/PocketImport.php index 1bf22d687..e00eb44b3 100644 --- a/src/Wallabag/ImportBundle/Import/PocketImport.php +++ b/src/Wallabag/ImportBundle/Import/PocketImport.php @@ -199,24 +199,16 @@ class PocketImport extends AbstractImport } $entry = new Entry($this->user); + $entry->setUrl($url); + + // update entry with content (in case fetching failed, the given entry will be return) $entry = $this->fetchContent($entry, $url); - // jump to next entry in case of problem while getting content - if (false === $entry) { - ++$this->skippedEntries; - - return; - } - // 0, 1, 2 - 1 if the item is archived - 2 if the item should be deleted - if ($importedEntry['status'] == 1 || $this->markAsRead) { - $entry->setArchived(true); - } + $entry->setArchived($importedEntry['status'] == 1 || $this->markAsRead); // 0 or 1 - 1 If the item is starred - if ($importedEntry['favorite'] == 1) { - $entry->setStarred(true); - } + $entry->setStarred($importedEntry['favorite'] == 1); $title = 'Untitled'; if (isset($importedEntry['resolved_title']) && $importedEntry['resolved_title'] != '') { @@ -226,7 +218,6 @@ class PocketImport extends AbstractImport } $entry->setTitle($title); - $entry->setUrl($url); // 0, 1, or 2 - 1 if the item has images in it - 2 if the item is an image if (isset($importedEntry['has_image']) && $importedEntry['has_image'] > 0 && isset($importedEntry['images'][1])) { diff --git a/src/Wallabag/ImportBundle/Import/ReadabilityImport.php b/src/Wallabag/ImportBundle/Import/ReadabilityImport.php index b852f8f01..fa2b70539 100644 --- a/src/Wallabag/ImportBundle/Import/ReadabilityImport.php +++ b/src/Wallabag/ImportBundle/Import/ReadabilityImport.php @@ -103,18 +103,12 @@ class ReadabilityImport extends AbstractImport 'created_at' => $importedEntry['date_added'], ]; - $entry = $this->fetchContent( - new Entry($this->user), - $data['url'], - $data - ); + $entry = new Entry($this->user); + $entry->setUrl($data['url']); + $entry->setTitle($data['title']); - // jump to next entry in case of problem while getting content - if (false === $entry) { - ++$this->skippedEntries; - - return; - } + // update entry with content (in case fetching failed, the given entry will be return) + $entry = $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 969a6a049..043bb0a23 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagImport.php +++ b/src/Wallabag/ImportBundle/Import/WallabagImport.php @@ -101,18 +101,12 @@ abstract class WallabagImport extends AbstractImport $data = $this->prepareEntry($importedEntry); - $entry = $this->fetchContent( - new Entry($this->user), - $importedEntry['url'], - $data - ); + $entry = new Entry($this->user); + $entry->setUrl($data['url']); + $entry->setTitle($data['title']); - // jump to next entry in case of problem while getting content - if (false === $entry) { - ++$this->skippedEntries; - - return; - } + // update entry with content (in case fetching failed, the given entry will be return) + $entry = $this->fetchContent($entry, $data['url'], $data); if (array_key_exists('tags', $data)) { $this->contentProxy->assignTagsToEntry( diff --git a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php index 48fbbfb61..952521a2a 100644 --- a/tests/Wallabag/ImportBundle/Import/PocketImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/PocketImportTest.php @@ -566,6 +566,8 @@ JSON; "status": 1, "list": { "229279689": { + "status": "1", + "favorite": "1", "resolved_url": "http://www.grantland.com/blog/the-triangle/post/_/id/38347/ryder-cup-preview" } } @@ -603,6 +605,6 @@ JSON; $res = $pocketImport->import(); $this->assertTrue($res); - $this->assertEquals(['skipped' => 1, 'imported' => 0, 'queued' => 0], $pocketImport->getSummary()); + $this->assertEquals(['skipped' => 0, 'imported' => 1, 'queued' => 0], $pocketImport->getSummary()); } } diff --git a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php index b4017f720..12bd6bdd0 100644 --- a/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php +++ b/tests/Wallabag/ImportBundle/Import/WallabagV2ImportTest.php @@ -256,6 +256,6 @@ class WallabagV2ImportTest extends \PHPUnit_Framework_TestCase $res = $wallabagV2Import->import(); $this->assertTrue($res); - $this->assertEquals(['skipped' => 24, 'imported' => 0, 'queued' => 0], $wallabagV2Import->getSummary()); + $this->assertEquals(['skipped' => 22, 'imported' => 2, 'queued' => 0], $wallabagV2Import->getSummary()); } }