From: Jérémy Benoist Date: Wed, 5 Jul 2017 13:00:24 +0000 (+0200) Subject: Merge pull request #3256 from wallabag/fix-patch X-Git-Tag: 2.3.0~31^2~43 X-Git-Url: https://git.immae.eu/?a=commitdiff_plain;h=b5d7eb148c4cd62ff187b08765f0c13c7d330fcf;hp=896f981ff522fe7d594e386a7112b23e593d6240;p=github%2Fwallabag%2Fwallabag.git Merge pull request #3256 from wallabag/fix-patch Fix PATCH method --- 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)) { @@ -603,6 +608,17 @@ class Entry $tag->removeEntry($this); } + /** + * Remove all assigned tags from the entry. + */ + public function removeAllTags() + { + foreach ($this->tags as $tag) { + $this->tags->removeElement($tag); + $tag->removeEntry($this); + } + } + /** * Set 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 @@ -66,6 +66,76 @@ class ContentProxy $this->stockEntry($entry, $content); } + /** + * 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); - } } diff --git a/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php b/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php index ae4af4cd..c76be13d 100644 --- a/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php +++ b/tests/Wallabag/ApiBundle/Controller/EntryRestControllerTest.php @@ -519,9 +519,6 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->markTestSkipped('No content found in db.'); } - // hydrate the tags relations - $nbTags = count($entry->getTags()); - $this->client->request('PATCH', '/api/entries/' . $entry->getId() . '.json', [ 'title' => 'New awesome title', 'tags' => 'new tag ' . uniqid(), @@ -532,6 +529,7 @@ class EntryRestControllerTest extends WallabagApiTestCase 'authors' => 'bob,sponge', 'content' => 'awesome', 'public' => 0, + 'published_at' => 1488833381, ]); $this->assertSame(200, $this->client->getResponse()->getStatusCode()); @@ -541,7 +539,7 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertSame($entry->getId(), $content['id']); $this->assertSame($entry->getUrl(), $content['url']); $this->assertSame('New awesome title', $content['title']); - $this->assertGreaterThan($nbTags, count($content['tags'])); + $this->assertGreaterThanOrEqual(1, count($content['tags']), 'We force only one tag'); $this->assertSame(1, $content['user_id']); $this->assertSame('de_AT', $content['language']); $this->assertSame('http://preview.io/picture.jpg', $content['preview_picture']); @@ -549,6 +547,7 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertContains('bob', $content['published_by']); $this->assertSame('awesome', $content['content']); $this->assertFalse($content['is_public'], 'Entry is no more shared'); + $this->assertContains('2017-03-06', $content['published_at']); } public function testPatchEntryWithoutQuotes() @@ -562,8 +561,8 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->markTestSkipped('No content found in db.'); } - // hydrate the tags relations - $nbTags = count($entry->getTags()); + $previousContent = $entry->getContent(); + $previousLanguage = $entry->getLanguage(); $this->client->request('PATCH', '/api/entries/' . $entry->getId() . '.json', [ 'title' => 'New awesome title', @@ -579,9 +578,10 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertSame($entry->getId(), $content['id']); $this->assertSame($entry->getUrl(), $content['url']); - $this->assertSame('New awesome title', $content['title']); - $this->assertGreaterThan($nbTags, count($content['tags'])); + $this->assertGreaterThanOrEqual(1, count($content['tags']), 'We force only one tag'); $this->assertEmpty($content['published_by'], 'Authors were not saved because of an array instead of a string'); + $this->assertSame($previousContent, $content['content'], 'Ensure content has not moved'); + $this->assertSame($previousLanguage, $content['language'], 'Ensure language has not moved'); } public function testGetTagsEntry() @@ -727,6 +727,8 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->markTestSkipped('No content found in db.'); } + $previousTitle = $entry->getTitle(); + $this->client->request('PATCH', '/api/entries/' . $entry->getId() . '.json', [ 'title' => $entry->getTitle() . '++', ]); @@ -736,6 +738,7 @@ class EntryRestControllerTest extends WallabagApiTestCase $content = json_decode($this->client->getResponse()->getContent(), true); $this->assertSame(1, $content['is_archived']); + $this->assertSame($previousTitle . '++', $content['title']); } public function testSaveIsStarredAfterPatch() @@ -907,6 +910,17 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertCount(4, $tags); } + public function testPostEntriesTagsListActionNoList() + { + $this->client->request('POST', '/api/entries/tags/lists?list=' . json_encode([])); + + $this->assertSame(200, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertEmpty($content); + } + public function testDeleteEntriesTagsListAction() { $em = $this->client->getContainer()->get('doctrine.orm.entity_manager'); @@ -933,6 +947,17 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertCount(0, $entry->getTags()); } + public function testDeleteEntriesTagsListActionNoList() + { + $this->client->request('DELETE', '/api/entries/tags/list?list=' . json_encode([])); + + $this->assertSame(200, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertEmpty($content); + } + public function testPostEntriesListAction() { $list = [ @@ -953,6 +978,17 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertSame('http://0.0.0.0/entry2', $content[1]['url']); } + public function testPostEntriesListActionWithNoUrls() + { + $this->client->request('POST', '/api/entries/lists?urls=' . json_encode([])); + + $this->assertSame(200, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertEmpty($content); + } + public function testDeleteEntriesListAction() { $em = $this->client->getContainer()->get('doctrine.orm.entity_manager'); @@ -978,6 +1014,17 @@ class EntryRestControllerTest extends WallabagApiTestCase $this->assertSame('http://0.0.0.0/test-entry-not-exist', $content[1]['url']); } + public function testDeleteEntriesListActionWithNoUrls() + { + $this->client->request('DELETE', '/api/entries/list?urls=' . json_encode([])); + + $this->assertSame(200, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertEmpty($content); + } + public function testLimitBulkAction() { $list = [ diff --git a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php index c63671c4..f94c2137 100644 --- a/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php +++ b/tests/Wallabag/CoreBundle/Helper/ContentProxyTest.php @@ -221,12 +221,9 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase ->method('tag'); $validator = $this->getValidator(); - $validator->expects($this->exactly(2)) + $validator->expects($this->once()) ->method('validate') - ->will($this->onConsecutiveCalls( - new ConstraintViolationList([new ConstraintViolation('oops', 'oops', [], 'oops', 'language', 'dontexist')]), - new ConstraintViolationList() - )); + ->willReturn(new ConstraintViolationList([new ConstraintViolation('oops', 'oops', [], 'oops', 'language', 'dontexist')])); $graby = $this->getMockBuilder('Graby\Graby') ->setMethods(['fetchContent']) @@ -498,6 +495,41 @@ class ContentProxyTest extends \PHPUnit_Framework_TestCase $this->assertSame('1.1.1.1', $entry->getDomainName()); } + public function testWithImageAsContent() + { + $tagger = $this->getTaggerMock(); + $tagger->expects($this->once()) + ->method('tag'); + + $graby = $this->getMockBuilder('Graby\Graby') + ->setMethods(['fetchContent']) + ->disableOriginalConstructor() + ->getMock(); + + $graby->expects($this->any()) + ->method('fetchContent') + ->willReturn([ + 'html' => '

', + 'title' => 'this is my title', + 'url' => 'http://1.1.1.1/image.jpg', + 'content_type' => 'image/jpeg', + 'status' => '200', + 'open_graph' => [], + ]); + + $proxy = new ContentProxy($graby, $tagger, $this->getValidator(), $this->getLogger(), $this->fetchingErrorMessage); + $entry = new Entry(new User()); + $proxy->updateEntry($entry, 'http://0.0.0.0'); + + $this->assertSame('http://1.1.1.1/image.jpg', $entry->getUrl()); + $this->assertSame('this is my title', $entry->getTitle()); + $this->assertContains('http://1.1.1.1/image.jpg', $entry->getContent()); + $this->assertSame('http://1.1.1.1/image.jpg', $entry->getPreviewPicture()); + $this->assertSame('image/jpeg', $entry->getMimetype()); + $this->assertSame('200', $entry->getHttpStatus()); + $this->assertSame('1.1.1.1', $entry->getDomainName()); + } + private function getTaggerMock() { return $this->getMockBuilder(RuleBasedTagger::class)