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=utf8 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 624576b5..40111af0 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 0a7c6e8c..9f051edb 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 a400686e..cbfacd55 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 2d097caf..21e2405c 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 1dbff022..43f376d4 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 ad13c940..56418ef9 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 35e5c9d0..bde21866 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 5bc896c3..26bb31ba 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 b0194c59..2c4df867 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 6412ad16..fb821966 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 11b744c7..3d22e29d 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 d6aaacfe..5099b002 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 2af0e69b..a1a14576 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 1bf22d68..e00eb44b 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 = $this->fetchContent($entry, $url); - - // jump to next entry in case of problem while getting content - if (false === $entry) { - ++$this->skippedEntries; + $entry->setUrl($url); - return; - } + // update entry with content (in case fetching failed, the given entry will be return) + $entry = $this->fetchContent($entry, $url); // 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 b852f8f0..fa2b7053 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 - ); - - // jump to next entry in case of problem while getting content - if (false === $entry) { - ++$this->skippedEntries; + $entry = new Entry($this->user); + $entry->setUrl($data['url']); + $entry->setTitle($data['title']); - 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 969a6a04..043bb0a2 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 - ); - - // jump to next entry in case of problem while getting content - if (false === $entry) { - ++$this->skippedEntries; + $entry = new Entry($this->user); + $entry->setUrl($data['url']); + $entry->setTitle($data['title']); - 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 48fbbfb6..952521a2 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 b4017f72..12bd6bdd 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()); } } -- 2.41.0