Merge pull request #3256 from wallabag/fix-patch

Fix PATCH method

3 files changed, 255 insertions, 154 deletions

diff --git a/src/Wallabag/ApiBundle/Controller/EntryRestController.php b/src/Wallabag/ApiBundle/Controller/EntryRestController.php
index ca460c84..8a206124 100644
--- a/src/Wallabag/ApiBundle/Controller/EntryRestController.php
+++ b/src/Wallabag/ApiBundle/Controller/EntryRestController.php
@@ -299,8 +299,8 @@ class EntryRestController extends WallabagRestController
      *          {"name"="url", "dataType"="string", "required"=true, "format"="http://www.test.com/article.html", "description"="Url for the entry."},
      *          {"name"="title", "dataType"="string", "required"=false, "description"="Optional, we'll get the title from the page."},
      *          {"name"="tags", "dataType"="string", "required"=false, "format"="tag1,tag2,tag3", "description"="a comma-separated list of tags."},
-     *          {"name"="starred", "dataType"="integer", "required"=false, "format"="1 or 0", "description"="entry already starred"},
      *          {"name"="archive", "dataType"="integer", "required"=false, "format"="1 or 0", "description"="entry already archived"},
+     *          {"name"="starred", "dataType"="integer", "required"=false, "format"="1 or 0", "description"="entry already starred"},
      *          {"name"="content", "dataType"="string", "required"=false, "description"="Content of the entry"},
      *          {"name"="language", "dataType"="string", "required"=false, "description"="Language of the entry"},
      *          {"name"="preview_picture", "dataType"="string", "required"=false, "description"="Preview picture of the entry"},
@@ -328,7 +328,58 @@ class EntryRestController extends WallabagRestController
             $entry->setUrl($url);
         }
 
-        $this->upsertEntry($entry, $request);
+        $data = $this->retrieveValueFromRequest($request);
+
+        try {
+            $this->get('wallabag_core.content_proxy')->updateEntry(
+                $entry,
+                $entry->getUrl(),
+                [
+                    'title' => !empty($data['title']) ? $data['title'] : $entry->getTitle(),
+                    'html' => !empty($data['content']) ? $data['content'] : $entry->getContent(),
+                    'url' => $entry->getUrl(),
+                    'language' => !empty($data['language']) ? $data['language'] : $entry->getLanguage(),
+                    'date' => !empty($data['publishedAt']) ? $data['publishedAt'] : $entry->getPublishedAt(),
+                    // faking the open graph preview picture
+                    'open_graph' => [
+                        'og_image' => !empty($data['picture']) ? $data['picture'] : $entry->getPreviewPicture(),
+                    ],
+                    'authors' => is_string($data['authors']) ? explode(',', $data['authors']) : $entry->getPublishedBy(),
+                ]
+            );
+        } catch (\Exception $e) {
+            $this->get('logger')->error('Error while saving an entry', [
+                'exception' => $e,
+                'entry' => $entry,
+            ]);
+        }
+
+        if (null !== $data['isArchived']) {
+            $entry->setArchived((bool) $data['isArchived']);
+        }
+
+        if (null !== $data['isStarred']) {
+            $entry->setStarred((bool) $data['isStarred']);
+        }
+
+        if (!empty($data['tags'])) {
+            $this->get('wallabag_core.tags_assigner')->assignTagsToEntry($entry, $data['tags']);
+        }
+
+        if (null !== $data['isPublic']) {
+            if (true === (bool) $data['isPublic'] && null === $entry->getUid()) {
+                $entry->generateUid();
+            } elseif (false === (bool) $data['isPublic']) {
+                $entry->cleanUid();
+            }
+        }
+
+        $em = $this->getDoctrine()->getManager();
+        $em->persist($entry);
+        $em->flush();
+
+        // entry saved, dispatch event about it!
+        $this->get('event_dispatcher')->dispatch(EntrySavedEvent::NAME, new EntrySavedEvent($entry));
 
         return $this->sendResponse($entry);
     }
@@ -361,7 +412,78 @@ class EntryRestController extends WallabagRestController
         $this->validateAuthentication();
         $this->validateUserAccess($entry->getUser()->getId());
 
-        $this->upsertEntry($entry, $request, true);
+        $contentProxy = $this->get('wallabag_core.content_proxy');
+
+        $data = $this->retrieveValueFromRequest($request);
+
+        // this is a special case where user want to manually update the entry content
+        // the ContentProxy will only cleanup the html
+        // and also we force to not re-fetch the content in case of error
+        if (!empty($data['content'])) {
+            try {
+                $contentProxy->updateEntry(
+                    $entry,
+                    $entry->getUrl(),
+                    [
+                        'html' => $data['content'],
+                    ],
+                    true
+                );
+            } catch (\Exception $e) {
+                $this->get('logger')->error('Error while saving an entry', [
+                    'exception' => $e,
+                    'entry' => $entry,
+                ]);
+            }
+        }
+
+        if (!empty($data['title'])) {
+            $entry->setTitle($data['title']);
+        }
+
+        if (!empty($data['language'])) {
+            $contentProxy->updateLanguage($entry, $data['language']);
+        }
+
+        if (!empty($data['authors']) && is_string($data['authors'])) {
+            $entry->setPublishedBy(explode(',', $data['authors']));
+        }
+
+        if (!empty($data['picture'])) {
+            $contentProxy->updatePreviewPicture($entry, $data['picture']);
+        }
+
+        if (!empty($data['publishedAt'])) {
+            $contentProxy->updatePublishedAt($entry, $data['publishedAt']);
+        }
+
+        if (null !== $data['isArchived']) {
+            $entry->setArchived((bool) $data['isArchived']);
+        }
+
+        if (null !== $data['isStarred']) {
+            $entry->setStarred((bool) $data['isStarred']);
+        }
+
+        if (!empty($data['tags'])) {
+            $entry->removeAllTags();
+            $this->get('wallabag_core.tags_assigner')->assignTagsToEntry($entry, $data['tags']);
+        }
+
+        if (null !== $data['isPublic']) {
+            if (true === (bool) $data['isPublic'] && null === $entry->getUid()) {
+                $entry->generateUid();
+            } elseif (false === (bool) $data['isPublic']) {
+                $entry->cleanUid();
+            }
+        }
+
+        $em = $this->getDoctrine()->getManager();
+        $em->persist($entry);
+        $em->flush();
+
+        // entry saved, dispatch event about it!
+        $this->get('event_dispatcher')->dispatch(EntrySavedEvent::NAME, new EntrySavedEvent($entry));
 
         return $this->sendResponse($entry);
     }
@@ -634,76 +756,27 @@ class EntryRestController extends WallabagRestController
     }
 
     /**
-     * Update or Insert a new entry.
+     * Retrieve value from the request.
+     * Used for POST & PATCH on a an entry.
      *
-     * @param Entry   $entry
      * @param Request $request
-     * @param bool    $disableContentUpdate If we don't want the content to be update by fetching the url (used when patching instead of posting)
+     *
+     * @return array
      */
-    private function upsertEntry(Entry $entry, Request $request, $disableContentUpdate = false)
+    private function retrieveValueFromRequest(Request $request)
     {
-        $title = $request->request->get('title');
-        $tags = $request->request->get('tags', []);
-        $isArchived = $request->request->get('archive');
-        $isStarred = $request->request->get('starred');
-        $isPublic = $request->request->get('public');
-        $content = $request->request->get('content');
-        $language = $request->request->get('language');
-        $picture = $request->request->get('preview_picture');
-        $publishedAt = $request->request->get('published_at');
-        $authors = $request->request->get('authors', '');
-
-        try {
-            $this->get('wallabag_core.content_proxy')->updateEntry(
-                $entry,
-                $entry->getUrl(),
-                [
-                    'title' => !empty($title) ? $title : $entry->getTitle(),
-                    'html' => !empty($content) ? $content : $entry->getContent(),
-                    'url' => $entry->getUrl(),
-                    'language' => !empty($language) ? $language : $entry->getLanguage(),
-                    'date' => !empty($publishedAt) ? $publishedAt : $entry->getPublishedAt(),
-                    // faking the open graph preview picture
-                    'open_graph' => [
-                        'og_image' => !empty($picture) ? $picture : $entry->getPreviewPicture(),
-                    ],
-                    'authors' => is_string($authors) ? explode(',', $authors) : $entry->getPublishedBy(),
-                ],
-                $disableContentUpdate
-            );
-        } catch (\Exception $e) {
-            $this->get('logger')->error('Error while saving an entry', [
-                'exception' => $e,
-                'entry' => $entry,
-            ]);
-        }
-
-        if (null !== $isArchived) {
-            $entry->setArchived((bool) $isArchived);
-        }
-
-        if (null !== $isStarred) {
-            $entry->setStarred((bool) $isStarred);
-        }
-
-        if (!empty($tags)) {
-            $this->get('wallabag_core.tags_assigner')->assignTagsToEntry($entry, $tags);
-        }
-
-        if (null !== $isPublic) {
-            if (true === (bool) $isPublic && null === $entry->getUid()) {
-                $entry->generateUid();
-            } elseif (false === (bool) $isPublic) {
-                $entry->cleanUid();
-            }
-        }
-
-        $em = $this->getDoctrine()->getManager();
-        $em->persist($entry);
-        $em->flush();
-
-        // entry saved, dispatch event about it!
-        $this->get('event_dispatcher')->dispatch(EntrySavedEvent::NAME, new EntrySavedEvent($entry));
+        return [
+            'title' => $request->request->get('title'),
+            'tags' => $request->request->get('tags', []),
+            'isArchived' => $request->request->get('archive'),
+            'isStarred' => $request->request->get('starred'),
+            'isPublic' => $request->request->get('public'),
+            'content' => $request->request->get('content'),
+            'language' => $request->request->get('language'),
+            'picture' => $request->request->get('preview_picture'),
+            'publishedAt' => $request->request->get('published_at'),
+            'authors' => $request->request->get('authors', ''),
+        ];
     }
 
     /**
diff --git a/src/Wallabag/CoreBundle/Entity/Entry.php b/src/Wallabag/CoreBundle/Entity/Entry.php
index 581e8906..cba72d31 100644
--- a/src/Wallabag/CoreBundle/Entity/Entry.php
+++ b/src/Wallabag/CoreBundle/Entity/Entry.php
@@ -593,6 +593,11 @@ class Entry
         $tag->addEntry($this);
     }
 
+    /**
+     * Remove the given tag from the entry (if the tag is associated).
+     *
+     * @param Tag $tag
+     */
     public function removeTag(Tag $tag)
     {
         if (!$this->tags->contains($tag)) {
@@ -604,6 +609,17 @@ class Entry
     }
 
     /**
+     * Remove all assigned tags from the entry.
+     */
+    public function removeAllTags()
+    {
+        foreach ($this->tags as $tag) {
+            $this->tags->removeElement($tag);
+            $tag->removeEntry($this);
+        }
+    }
+
+    /**
      * Set previewPicture.
      *
      * @param string $previewPicture
diff --git a/src/Wallabag/CoreBundle/Helper/ContentProxy.php b/src/Wallabag/CoreBundle/Helper/ContentProxy.php
index ddecd6f4..656ac6ee 100644
--- a/src/Wallabag/CoreBundle/Helper/ContentProxy.php
+++ b/src/Wallabag/CoreBundle/Helper/ContentProxy.php
@@ -67,6 +67,76 @@ class ContentProxy
     }
 
     /**
+     * Use a Symfony validator to ensure the language is well formatted.
+     *
+     * @param Entry  $entry
+     * @param string $value Language to validate and save
+     */
+    public function updateLanguage(Entry $entry, $value)
+    {
+        // some lang are defined as fr-FR, es-ES.
+        // replacing - by _ might increase language support
+        $value = str_replace('-', '_', $value);
+
+        $errors = $this->validator->validate(
+            $value,
+            (new LocaleConstraint())
+        );
+
+        if (0 === count($errors)) {
+            $entry->setLanguage($value);
+
+            return;
+        }
+
+        $this->logger->warning('Language validation failed. ' . (string) $errors);
+    }
+
+    /**
+     * Use a Symfony validator to ensure the preview picture is a real url.
+     *
+     * @param Entry  $entry
+     * @param string $value URL to validate and save
+     */
+    public function updatePreviewPicture(Entry $entry, $value)
+    {
+        $errors = $this->validator->validate(
+            $value,
+            (new UrlConstraint())
+        );
+
+        if (0 === count($errors)) {
+            $entry->setPreviewPicture($value);
+
+            return;
+        }
+
+        $this->logger->warning('PreviewPicture validation failed. ' . (string) $errors);
+    }
+
+    /**
+     * Update date.
+     *
+     * @param Entry  $entry
+     * @param string $value Date to validate and save
+     */
+    public function updatePublishedAt(Entry $entry, $value)
+    {
+        $date = $value;
+
+        // is it a timestamp?
+        if (filter_var($date, FILTER_VALIDATE_INT) !== false) {
+            $date = '@' . $value;
+        }
+
+        try {
+            $entry->setPublishedAt(new \DateTime($date));
+        } catch (\Exception $e) {
+            $this->logger->warning('Error while defining date', ['e' => $e, 'url' => $entry->getUrl(), 'date' => $value]);
+        }
+    }
+
+    /**
      * Stock entry with fetched or imported content.
      * Will fall back to OpenGraph data if available.
      *
@@ -75,9 +145,17 @@ class ContentProxy
      */
     private function stockEntry(Entry $entry, array $content)
     {
-        $title = $content['title'];
-        if (!$title && !empty($content['open_graph']['og_title'])) {
-            $title = $content['open_graph']['og_title'];
+        $entry->setUrl($content['url']);
+
+        $domainName = parse_url($entry->getUrl(), PHP_URL_HOST);
+        if (false !== $domainName) {
+            $entry->setDomainName($domainName);
+        }
+
+        if (!empty($content['title'])) {
+            $entry->setTitle($content['title']);
+        } elseif (!empty($content['open_graph']['og_title'])) {
+            $entry->setTitle($content['open_graph']['og_title']);
         }
 
         $html = $content['html'];
@@ -90,24 +168,11 @@ class ContentProxy
             }
         }
 
-        $entry->setUrl($content['url']);
-        $entry->setTitle($title);
         $entry->setContent($html);
-        $entry->setHttpStatus(isset($content['status']) ? $content['status'] : '');
-
-        if (!empty($content['date'])) {
-            $date = $content['date'];
-
-            // is it a timestamp?
-            if (filter_var($date, FILTER_VALIDATE_INT) !== false) {
-                $date = '@' . $content['date'];
-            }
+        $entry->setReadingTime(Utils::getReadingTime($html));
 
-            try {
-                $entry->setPublishedAt(new \DateTime($date));
-            } catch (\Exception $e) {
-                $this->logger->warning('Error while defining date', ['e' => $e, 'url' => $content['url'], 'date' => $content['date']]);
-            }
+        if (!empty($content['status'])) {
+            $entry->setHttpStatus($content['status']);
         }
 
         if (!empty($content['authors']) && is_array($content['authors'])) {
@@ -118,30 +183,25 @@ class ContentProxy
             $entry->setHeaders($content['all_headers']);
         }
 
-        $this->validateAndSetLanguage(
-            $entry,
-            isset($content['language']) ? $content['language'] : null
-        );
+        if (!empty($content['date'])) {
+            $this->updatePublishedAt($entry, $content['date']);
+        }
 
-        $this->validateAndSetPreviewPicture(
-            $entry,
-            isset($content['open_graph']['og_image']) ? $content['open_graph']['og_image'] : null
-        );
+        if (!empty($content['language'])) {
+            $this->updateLanguage($entry, $content['language']);
+        }
+
+        if (!empty($content['open_graph']['og_image'])) {
+            $this->updatePreviewPicture($entry, $content['open_graph']['og_image']);
+        }
 
         // if content is an image, define it as a preview too
         if (!empty($content['content_type']) && in_array($this->mimeGuesser->guess($content['content_type']), ['jpeg', 'jpg', 'gif', 'png'], true)) {
-            $this->validateAndSetPreviewPicture(
-                $entry,
-                $content['url']
-            );
+            $this->updatePreviewPicture($entry, $content['url']);
         }
 
-        $entry->setMimetype(isset($content['content_type']) ? $content['content_type'] : '');
-        $entry->setReadingTime(Utils::getReadingTime($html));
-
-        $domainName = parse_url($entry->getUrl(), PHP_URL_HOST);
-        if (false !== $domainName) {
-            $entry->setDomainName($domainName);
+        if (!empty($content['content_type'])) {
+            $entry->setMimetype($content['content_type']);
         }
 
         try {
@@ -165,52 +225,4 @@ class ContentProxy
     {
         return !empty($content['title']) && !empty($content['html']) && !empty($content['url']);
     }
-
-    /**
-     * Use a Symfony validator to ensure the language is well formatted.
-     *
-     * @param Entry  $entry
-     * @param string $value Language to validate
-     */
-    private function validateAndSetLanguage($entry, $value)
-    {
-        // some lang are defined as fr-FR, es-ES.
-        // replacing - by _ might increase language support
-        $value = str_replace('-', '_', $value);
-
-        $errors = $this->validator->validate(
-            $value,
-            (new LocaleConstraint())
-        );
-
-        if (0 === count($errors)) {
-            $entry->setLanguage($value);
-
-            return;
-        }
-
-        $this->logger->warning('Language validation failed. ' . (string) $errors);
-    }
-
-    /**
-     * Use a Symfony validator to ensure the preview picture is a real url.
-     *
-     * @param Entry  $entry
-     * @param string $value URL to validate
-     */
-    private function validateAndSetPreviewPicture($entry, $value)
-    {
-        $errors = $this->validator->validate(
-            $value,
-            (new UrlConstraint())
-        );
-
-        if (0 === count($errors)) {
-            $entry->setPreviewPicture($value);
-
-            return;
-        }
-
-        $this->logger->warning('PreviewPicture validation failed. ' . (string) $errors);
-    }
 }