From 3784688a88230d9c3aec4ca518be52ea1c70aeb9 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Sat, 12 Jan 2019 13:09:36 +0100 Subject: [PATCH 1/3] Replace continue; with break; to avoid PHP 7.3 warnings Signed-off-by: Thomas Citharel --- .../GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php | 2 +- src/Wallabag/CoreBundle/Helper/DownloadImages.php | 2 +- src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php | 2 +- src/Wallabag/CoreBundle/Helper/TagsAssigner.php | 2 +- src/Wallabag/ImportBundle/Import/AbstractImport.php | 2 +- src/Wallabag/ImportBundle/Import/BrowserImport.php | 6 +++--- src/Wallabag/ImportBundle/Import/InstapaperImport.php | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php index 90e00c62d..2e57aac8a 100644 --- a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php +++ b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php @@ -114,7 +114,7 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder $extraFields = []; foreach ($extraFieldsStrings as $extraField) { if (false === strpos($extraField, '=')) { - continue; + break; } list($fieldName, $fieldValue) = explode('=', $extraField, 2); diff --git a/src/Wallabag/CoreBundle/Helper/DownloadImages.php b/src/Wallabag/CoreBundle/Helper/DownloadImages.php index cc3dcfceb..8c1c208f5 100644 --- a/src/Wallabag/CoreBundle/Helper/DownloadImages.php +++ b/src/Wallabag/CoreBundle/Helper/DownloadImages.php @@ -56,7 +56,7 @@ class DownloadImages $imagePath = $this->processSingleImage($entryId, $image, $url, $relativePath); if (false === $imagePath) { - continue; + break; } // if image contains "&" and we can't find it in the html it might be because it's encoded as & diff --git a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php index fbdf2ac7a..d156df84e 100644 --- a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php +++ b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php @@ -37,7 +37,7 @@ class RuleBasedTagger foreach ($rules as $rule) { if (!$this->rulerz->satisfies($entry, $rule->getRule())) { - continue; + break; } $this->logger->info('Matching rule.', [ diff --git a/src/Wallabag/CoreBundle/Helper/TagsAssigner.php b/src/Wallabag/CoreBundle/Helper/TagsAssigner.php index e6b4989f8..519150f53 100644 --- a/src/Wallabag/CoreBundle/Helper/TagsAssigner.php +++ b/src/Wallabag/CoreBundle/Helper/TagsAssigner.php @@ -49,7 +49,7 @@ class TagsAssigner // avoid empty tag if (0 === \strlen($label)) { - continue; + break; } if (isset($tagsNotYetFlushed[$label])) { diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index d39d71b6c..5ae4aa8d6 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -169,7 +169,7 @@ abstract class AbstractImport implements ImportInterface $entry = $this->parseEntry($importedEntry); if (null === $entry) { - continue; + break; } // store each entry to be flushed so we can trigger the entry.saved event for each of them diff --git a/src/Wallabag/ImportBundle/Import/BrowserImport.php b/src/Wallabag/ImportBundle/Import/BrowserImport.php index 804bc6cd0..178ebe8e5 100644 --- a/src/Wallabag/ImportBundle/Import/BrowserImport.php +++ b/src/Wallabag/ImportBundle/Import/BrowserImport.php @@ -158,13 +158,13 @@ abstract class BrowserImport extends AbstractImport foreach ($entries as $importedEntry) { if ((array) $importedEntry !== $importedEntry) { - continue; + break; } $entry = $this->parseEntry($importedEntry); if (null === $entry) { - continue; + break; } // @see AbstractImport @@ -206,7 +206,7 @@ abstract class BrowserImport extends AbstractImport { foreach ($entries as $importedEntry) { if ((array) $importedEntry !== $importedEntry) { - continue; + break; } // set userId for the producer (it won't know which user is connected) diff --git a/src/Wallabag/ImportBundle/Import/InstapaperImport.php b/src/Wallabag/ImportBundle/Import/InstapaperImport.php index 439c978c7..6b6b35afa 100644 --- a/src/Wallabag/ImportBundle/Import/InstapaperImport.php +++ b/src/Wallabag/ImportBundle/Import/InstapaperImport.php @@ -65,7 +65,7 @@ class InstapaperImport extends AbstractImport $handle = fopen($this->filepath, 'r'); while (false !== ($data = fgetcsv($handle, 10240))) { if ('URL' === $data[0]) { - continue; + break; } // last element in the csv is the folder where the content belong From ea925bb112ab99efbb29d8e7113e80357a70bd18 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Wed, 27 Feb 2019 14:33:26 +0100 Subject: [PATCH 2/3] CS --- src/Wallabag/CoreBundle/Form/Type/EntryFilterType.php | 2 +- src/Wallabag/ImportBundle/Import/BrowserImport.php | 10 +++++----- src/Wallabag/ImportBundle/Import/ChromeImport.php | 2 +- src/Wallabag/ImportBundle/Import/FirefoxImport.php | 2 +- src/Wallabag/ImportBundle/Import/WallabagImport.php | 2 +- src/Wallabag/ImportBundle/Import/WallabagV1Import.php | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Wallabag/CoreBundle/Form/Type/EntryFilterType.php b/src/Wallabag/CoreBundle/Form/Type/EntryFilterType.php index 702c7f7aa..37d0640a6 100644 --- a/src/Wallabag/CoreBundle/Form/Type/EntryFilterType.php +++ b/src/Wallabag/CoreBundle/Form/Type/EntryFilterType.php @@ -108,7 +108,7 @@ class EntryFilterType extends AbstractType ->add('httpStatus', TextFilterType::class, [ 'apply_filter' => function (QueryInterface $filterQuery, $field, $values) { $value = $values['value']; - if (false === array_key_exists($value, Response::$statusTexts)) { + if (false === \array_key_exists($value, Response::$statusTexts)) { return; } diff --git a/src/Wallabag/ImportBundle/Import/BrowserImport.php b/src/Wallabag/ImportBundle/Import/BrowserImport.php index 178ebe8e5..99717beb8 100644 --- a/src/Wallabag/ImportBundle/Import/BrowserImport.php +++ b/src/Wallabag/ImportBundle/Import/BrowserImport.php @@ -77,7 +77,7 @@ abstract class BrowserImport extends AbstractImport */ public function parseEntry(array $importedEntry) { - if ((!array_key_exists('guid', $importedEntry) || (!array_key_exists('id', $importedEntry))) && \is_array(reset($importedEntry))) { + if ((!\array_key_exists('guid', $importedEntry) || (!\array_key_exists('id', $importedEntry))) && \is_array(reset($importedEntry))) { if ($this->producer) { $this->parseEntriesForProducer($importedEntry); @@ -89,7 +89,7 @@ abstract class BrowserImport extends AbstractImport return; } - if (array_key_exists('children', $importedEntry)) { + if (\array_key_exists('children', $importedEntry)) { if ($this->producer) { $this->parseEntriesForProducer($importedEntry['children']); @@ -101,11 +101,11 @@ abstract class BrowserImport extends AbstractImport return; } - if (!array_key_exists('uri', $importedEntry) && !array_key_exists('url', $importedEntry)) { + if (!\array_key_exists('uri', $importedEntry) && !\array_key_exists('url', $importedEntry)) { return; } - $url = array_key_exists('uri', $importedEntry) ? $importedEntry['uri'] : $importedEntry['url']; + $url = \array_key_exists('uri', $importedEntry) ? $importedEntry['uri'] : $importedEntry['url']; $existingEntry = $this->em ->getRepository('WallabagCoreBundle:Entry') @@ -126,7 +126,7 @@ abstract class BrowserImport extends AbstractImport // update entry with content (in case fetching failed, the given entry will be return) $this->fetchContent($entry, $data['url'], $data); - if (array_key_exists('tags', $data)) { + if (\array_key_exists('tags', $data)) { $this->tagsAssigner->assignTagsToEntry( $entry, $data['tags'] diff --git a/src/Wallabag/ImportBundle/Import/ChromeImport.php b/src/Wallabag/ImportBundle/Import/ChromeImport.php index eccee6986..4ae82ade8 100644 --- a/src/Wallabag/ImportBundle/Import/ChromeImport.php +++ b/src/Wallabag/ImportBundle/Import/ChromeImport.php @@ -57,7 +57,7 @@ class ChromeImport extends BrowserImport 'created_at' => substr($entry['date_added'], 0, 10), ]; - if (array_key_exists('tags', $entry) && '' !== $entry['tags']) { + if (\array_key_exists('tags', $entry) && '' !== $entry['tags']) { $data['tags'] = $entry['tags']; } diff --git a/src/Wallabag/ImportBundle/Import/FirefoxImport.php b/src/Wallabag/ImportBundle/Import/FirefoxImport.php index 8999e3f39..b3558f21e 100644 --- a/src/Wallabag/ImportBundle/Import/FirefoxImport.php +++ b/src/Wallabag/ImportBundle/Import/FirefoxImport.php @@ -57,7 +57,7 @@ class FirefoxImport extends BrowserImport 'created_at' => substr($entry['dateAdded'], 0, 10), ]; - if (array_key_exists('tags', $entry) && '' !== $entry['tags']) { + if (\array_key_exists('tags', $entry) && '' !== $entry['tags']) { $data['tags'] = $entry['tags']; } diff --git a/src/Wallabag/ImportBundle/Import/WallabagImport.php b/src/Wallabag/ImportBundle/Import/WallabagImport.php index c3a142b91..75a28fbf5 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagImport.php +++ b/src/Wallabag/ImportBundle/Import/WallabagImport.php @@ -122,7 +122,7 @@ abstract class WallabagImport extends AbstractImport // update entry with content (in case fetching failed, the given entry will be return) $this->fetchContent($entry, $data['url'], $data); - if (array_key_exists('tags', $data)) { + if (\array_key_exists('tags', $data)) { $this->tagsAssigner->assignTagsToEntry( $entry, $data['tags'], diff --git a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php index b9bb525ab..e05626117 100644 --- a/src/Wallabag/ImportBundle/Import/WallabagV1Import.php +++ b/src/Wallabag/ImportBundle/Import/WallabagV1Import.php @@ -61,7 +61,7 @@ class WallabagV1Import extends WallabagImport $data['html'] = $this->fetchingErrorMessage; } - if (array_key_exists('tags', $entry) && '' !== $entry['tags']) { + if (\array_key_exists('tags', $entry) && '' !== $entry['tags']) { $data['tags'] = $entry['tags']; } From 8c0ba953070dca22e9a06999cfe355ea01847c64 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Wed, 27 Feb 2019 14:59:50 +0100 Subject: [PATCH 3/3] Adding more tests --- .../GrabySiteConfigBuilder.php | 2 +- .../CoreBundle/Helper/DownloadImages.php | 2 +- .../CoreBundle/Helper/RuleBasedTagger.php | 2 +- .../CoreBundle/Helper/TagsAssigner.php | 2 +- .../ImportBundle/Import/AbstractImport.php | 2 +- .../ImportBundle/Import/BrowserImport.php | 6 +- .../ImportBundle/Import/InstapaperImport.php | 2 +- .../GrabySiteConfigBuilderTest.php | 68 +++++++++++++++++++ 8 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php index 2e57aac8a..90e00c62d 100644 --- a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php +++ b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php @@ -114,7 +114,7 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder $extraFields = []; foreach ($extraFieldsStrings as $extraField) { if (false === strpos($extraField, '=')) { - break; + continue; } list($fieldName, $fieldValue) = explode('=', $extraField, 2); diff --git a/src/Wallabag/CoreBundle/Helper/DownloadImages.php b/src/Wallabag/CoreBundle/Helper/DownloadImages.php index 8c1c208f5..cc3dcfceb 100644 --- a/src/Wallabag/CoreBundle/Helper/DownloadImages.php +++ b/src/Wallabag/CoreBundle/Helper/DownloadImages.php @@ -56,7 +56,7 @@ class DownloadImages $imagePath = $this->processSingleImage($entryId, $image, $url, $relativePath); if (false === $imagePath) { - break; + continue; } // if image contains "&" and we can't find it in the html it might be because it's encoded as & diff --git a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php index d156df84e..fbdf2ac7a 100644 --- a/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php +++ b/src/Wallabag/CoreBundle/Helper/RuleBasedTagger.php @@ -37,7 +37,7 @@ class RuleBasedTagger foreach ($rules as $rule) { if (!$this->rulerz->satisfies($entry, $rule->getRule())) { - break; + continue; } $this->logger->info('Matching rule.', [ diff --git a/src/Wallabag/CoreBundle/Helper/TagsAssigner.php b/src/Wallabag/CoreBundle/Helper/TagsAssigner.php index 519150f53..e6b4989f8 100644 --- a/src/Wallabag/CoreBundle/Helper/TagsAssigner.php +++ b/src/Wallabag/CoreBundle/Helper/TagsAssigner.php @@ -49,7 +49,7 @@ class TagsAssigner // avoid empty tag if (0 === \strlen($label)) { - break; + continue; } if (isset($tagsNotYetFlushed[$label])) { diff --git a/src/Wallabag/ImportBundle/Import/AbstractImport.php b/src/Wallabag/ImportBundle/Import/AbstractImport.php index 5ae4aa8d6..d39d71b6c 100644 --- a/src/Wallabag/ImportBundle/Import/AbstractImport.php +++ b/src/Wallabag/ImportBundle/Import/AbstractImport.php @@ -169,7 +169,7 @@ abstract class AbstractImport implements ImportInterface $entry = $this->parseEntry($importedEntry); if (null === $entry) { - break; + continue; } // store each entry to be flushed so we can trigger the entry.saved event for each of them diff --git a/src/Wallabag/ImportBundle/Import/BrowserImport.php b/src/Wallabag/ImportBundle/Import/BrowserImport.php index 99717beb8..3987e80f0 100644 --- a/src/Wallabag/ImportBundle/Import/BrowserImport.php +++ b/src/Wallabag/ImportBundle/Import/BrowserImport.php @@ -158,13 +158,13 @@ abstract class BrowserImport extends AbstractImport foreach ($entries as $importedEntry) { if ((array) $importedEntry !== $importedEntry) { - break; + continue; } $entry = $this->parseEntry($importedEntry); if (null === $entry) { - break; + continue; } // @see AbstractImport @@ -206,7 +206,7 @@ abstract class BrowserImport extends AbstractImport { foreach ($entries as $importedEntry) { if ((array) $importedEntry !== $importedEntry) { - break; + continue; } // set userId for the producer (it won't know which user is connected) diff --git a/src/Wallabag/ImportBundle/Import/InstapaperImport.php b/src/Wallabag/ImportBundle/Import/InstapaperImport.php index 6b6b35afa..439c978c7 100644 --- a/src/Wallabag/ImportBundle/Import/InstapaperImport.php +++ b/src/Wallabag/ImportBundle/Import/InstapaperImport.php @@ -65,7 +65,7 @@ class InstapaperImport extends AbstractImport $handle = fopen($this->filepath, 'r'); while (false !== ($data = fgetcsv($handle, 10240))) { if ('URL' === $data[0]) { - break; + continue; } // last element in the csv is the folder where the content belong diff --git a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php index 1173fc3de..7beccd309 100644 --- a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php +++ b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php @@ -134,4 +134,72 @@ class GrabySiteConfigBuilderTest extends TestCase $this->assertCount(1, $records, 'One log was recorded'); } + + public function testBuildConfigWithBadExtraFields() + { + /* @var \Graby\SiteConfig\ConfigBuilder|\PHPUnit_Framework_MockObject_MockObject */ + $grabyConfigBuilderMock = $this->getMockBuilder('Graby\SiteConfig\ConfigBuilder') + ->disableOriginalConstructor() + ->getMock(); + + $grabySiteConfig = new GrabySiteConfig(); + $grabySiteConfig->requires_login = true; + $grabySiteConfig->login_uri = 'http://www.example.com/login'; + $grabySiteConfig->login_username_field = 'login'; + $grabySiteConfig->login_password_field = 'password'; + $grabySiteConfig->login_extra_fields = ['field']; + $grabySiteConfig->not_logged_in_xpath = '//div[@class="need-login"]'; + + $grabyConfigBuilderMock + ->method('buildForHost') + ->with('example.com') + ->will($this->returnValue($grabySiteConfig)); + + $logger = new Logger('foo'); + $handler = new TestHandler(); + $logger->pushHandler($handler); + + $siteCrentialRepo = $this->getMockBuilder('Wallabag\CoreBundle\Repository\SiteCredentialRepository') + ->disableOriginalConstructor() + ->getMock(); + $siteCrentialRepo->expects($this->once()) + ->method('findOneByHostAndUser') + ->with('example.com', 1) + ->willReturn(['username' => 'foo', 'password' => 'bar']); + + $user = $this->getMockBuilder('Wallabag\UserBundle\Entity\User') + ->disableOriginalConstructor() + ->getMock(); + $user->expects($this->once()) + ->method('getId') + ->willReturn(1); + + $token = new UsernamePasswordToken($user, 'pass', 'provider'); + + $tokenStorage = new TokenStorage(); + $tokenStorage->setToken($token); + + $this->builder = new GrabySiteConfigBuilder( + $grabyConfigBuilderMock, + $tokenStorage, + $siteCrentialRepo, + $logger + ); + + $config = $this->builder->buildForHost('www.example.com'); + + $this->assertSame('example.com', $config->getHost()); + $this->assertTrue($config->requiresLogin()); + $this->assertSame('http://www.example.com/login', $config->getLoginUri()); + $this->assertSame('login', $config->getUsernameField()); + $this->assertSame('password', $config->getPasswordField()); + $this->assertSame([], $config->getExtraFields()); + $this->assertSame('//div[@class="need-login"]', $config->getNotLoggedInXpath()); + $this->assertSame('foo', $config->getUsername()); + $this->assertSame('bar', $config->getPassword()); + + $records = $handler->getRecords(); + + $this->assertCount(1, $records, 'One log was recorded'); + } }