Improve code & readability

Propaganistas · May 29, 2021 · 0e1a13f · 0e1a13f
1 parent 0f23e48
commit 0e1a13f
Show file tree

Hide file tree

Showing 9 changed files with 100 additions and 128 deletions.
diff --git a/README.md b/README.md
@@ -63,4 +63,5 @@ Use the `indisposable` validator to ensure a given field doesn't hold a disposab
 
 ### Custom fetches
 
-By default the package retrieves a new list by using `file_get_contents()`. If your application has different needs (e.g. when behind a proxy) please review the `disposable-email.fetcher` configuration value.
+By default the package retrieves a new list by using `file_get_contents()`. 
+If your application has different needs (e.g. when behind a proxy) please review the `disposable-email.fetcher` configuration value.
diff --git a/config/disposable-email.php b/config/disposable-email.php
@@ -24,10 +24,11 @@
     |
     | The class responsible for fetching the contents of the source url.
     | The default implementation makes use of file_get_contents and
-    | will suffice for most applications.
+    | json_decode and will probably suffice for most applications.
     |
     | If your application has different needs (e.g. behind a proxy) then you
     | can define a custom fetch class here that carries out the fetching.
+    | Your custom class should implement the Fetcher contract.
     |
     */
 

diff --git a/src/Console/UpdateDisposableDomainsCommand.php b/src/Console/UpdateDisposableDomainsCommand.php
@@ -4,6 +4,7 @@
 
 use Illuminate\Contracts\Config\Repository as Config;
 use Illuminate\Console\Command;
+use Propaganistas\LaravelDisposableEmail\Contracts\Fetcher;
 use Propaganistas\LaravelDisposableEmail\DisposableDomains;
 
 class UpdateDisposableDomainsCommand extends Command
@@ -34,9 +35,14 @@ public function handle(Config $config, DisposableDomains $disposable)
         $this->line('Fetching from source...');
 
         $fetcher = $this->laravel->make(
-            $config->get('disposable-email.fetcher')
+            $fetcherClass = $config->get('disposable-email.fetcher')
         );
 
+        if (! $fetcher instanceof Fetcher) {
+            $this->error($fetcherClass . ' should implement ' . Fetcher::class);
+            return 1;
+        }
+
         $data = $this->laravel->call([$fetcher, 'handle'], [
             'url' => $config->get('disposable-email.source'),
         ]);
@@ -46,8 +52,10 @@ public function handle(Config $config, DisposableDomains $disposable)
         if ($disposable->saveToStorage($data)) {
             $this->info('Disposable domains list updated successfully.');
             $disposable->bootstrap();
+            return 0;
         } else {
             $this->error('Could not write to storage ('.$disposable->getStoragePath().')!');
+            return 1;
         }
     }
 }
diff --git a/src/Contracts/Fetcher.php b/src/Contracts/Fetcher.php
@@ -0,0 +1,8 @@
+<?php
+
+namespace Propaganistas\LaravelDisposableEmail\Contracts;
+
+interface Fetcher
+{
+    public function handle($url): array;
+}
diff --git a/src/DisposableDomains.php b/src/DisposableDomains.php
@@ -2,11 +2,9 @@
 
 namespace Propaganistas\LaravelDisposableEmail;
 
-use ErrorException;
 use Illuminate\Contracts\Cache\Repository as Cache;
 use Illuminate\Support\Arr;
 use Illuminate\Support\Str;
-use UnexpectedValueException;
 
 class DisposableDomains
 {
@@ -58,8 +56,9 @@ public function bootstrap()
         $domains = $this->getFromCache();
 
         if (! $domains) {
-            $domains = $this->getFromStorage();
-            $this->saveToCache($domains);
+            $this->saveToCache(
+                $domains = $this->getFromStorage()
+            );
         }
 
         $this->domains = $domains;
@@ -76,8 +75,14 @@ protected function getFromCache()
     {
         if ($this->cache) {
             $domains = $this->cache->get($this->getCacheKey());
-
-            return is_string($domains) ? json_decode($domains) : $domains;
+
+            // @TODO: Legacy code for bugfix. Remove me.
+            if (is_string($domains) || empty($domains)) {
+                $this->flushCache();
+                return null;
+            }
+
+            return $domains;
         }
 
         return null;
@@ -86,12 +91,12 @@ protected function getFromCache()
     /**
      * Save the domains in cache.
      *
-     * @param  mixed  $data
+     * @param  array|null  $domains
      */
-    public function saveToCache($data)
+    public function saveToCache(array $domains = null)
     {
-        if ($this->cache) {
-            $this->cache->forever($this->getCacheKey(), $data);
+        if ($this->cache && ! empty($domains)) {
+            $this->cache->forever($this->getCacheKey(), $domains);
         }
     }
 
@@ -112,34 +117,21 @@ public function flushCache()
      */
     protected function getFromStorage()
     {
-        if (is_file($this->getStoragePath())) {
-            $domains = $this->parseJson(
-                file_get_contents($this->getStoragePath())
-            );
+        $domains = is_file($this->getStoragePath())
+            ? file_get_contents($this->getStoragePath())
+            : file_get_contents(__DIR__.'/../domains.json');
 
-            if ($domains) {
-                return Arr::wrap($domains);
-            }
-        }
-
-        // Fall back to the list provided by the package.
-        return $this->parseJson(file_get_contents(__DIR__.'/../domains.json'));
+        return json_decode($domains, true);
     }
 
     /**
      * Save the domains in storage.
      *
-     * @param  mixed  $data
+     * @param  array  $domains
      */
-    public function saveToStorage($data)
+    public function saveToStorage(array $domains)
     {
-        if (is_string($data) && ! $this->parseJson($data)) {
-            throw new UnexpectedValueException('Provided data could not be parsed as a JSON list');
-        }
-
-        $saved = file_put_contents($this->getStoragePath(),
-            is_string($data) ? $data : json_encode($data)
-        );
+        $saved = file_put_contents($this->getStoragePath(), json_encode($domains));
 
         if ($saved) {
             $this->flushCache();
@@ -251,21 +243,4 @@ public function setCacheKey($key)
 
         return $this;
     }
-
-    /**
-     * Parses the given JSON into a native array. Returns false on errors.
-     *
-     * @param string $data
-     * @return array|bool
-     */
-    protected function parseJson($data)
-    {
-        $data = json_decode($data, true);
-
-        if (json_last_error() !== JSON_ERROR_NONE || empty($data)) {
-            return false;
-        }
-
-        return $data;
-    }
 }
diff --git a/src/Fetcher/DefaultFetcher.php b/src/Fetcher/DefaultFetcher.php
@@ -3,11 +3,12 @@
 namespace Propaganistas\LaravelDisposableEmail\Fetcher;
 
 use InvalidArgumentException;
+use Propaganistas\LaravelDisposableEmail\Contracts\Fetcher;
 use UnexpectedValueException;
 
-class DefaultFetcher
+class DefaultFetcher implements Fetcher
 {
-    public function handle($url)
+    public function handle($url): array
     {
         if (! $url) {
             throw new InvalidArgumentException('Source URL is null');
@@ -19,6 +20,17 @@ public function handle($url)
             throw new UnexpectedValueException('Failed to interpret the source URL ('.$url.')');
         }
 
-        return $content;
+        if (! $this->isValidJson($content)) {
+            throw new UnexpectedValueException('Provided data could not be parsed as JSON');
+        }
+
+        return json_decode($content);
+    }
+
+    protected function isValidJson($data): bool
+    {
+        $data = json_decode($data, true);
+
+        return json_last_error() === JSON_ERROR_NONE && ! empty($data);
     }
 }
diff --git a/tests/Console/UpdateDisposableDomainsCommandTest.php b/tests/Console/UpdateDisposableDomainsCommandTest.php
@@ -3,8 +3,8 @@
 namespace Propaganistas\LaravelDisposableEmail\Tests\Console;
 
 use InvalidArgumentException;
+use Propaganistas\LaravelDisposableEmail\Contracts\Fetcher;
 use Propaganistas\LaravelDisposableEmail\Tests\TestCase;
-use UnexpectedValueException;
 
 class UpdateDisposableDomainsCommandTest extends TestCase
 {
@@ -13,7 +13,8 @@ public function it_creates_the_file()
     {
         $this->assertFileNotExists($this->storagePath);
 
-        $this->artisan('disposable:update');
+        $this->artisan('disposable:update')
+            ->assertExitCode(0);
 
         $this->assertFileExists($this->storagePath);
 
@@ -26,9 +27,10 @@ public function it_creates_the_file()
     /** @test */
     public function it_overwrites_the_file()
     {
-        file_put_contents($this->storagePath, 'foo');
+        file_put_contents($this->storagePath, json_encode(['foo']));
 
-        $this->artisan('disposable:update');
+        $this->artisan('disposable:update')
+            ->assertExitCode(0);
 
         $this->assertFileExists($this->storagePath);
 
@@ -45,32 +47,12 @@ public function it_doesnt_overwrite_on_fetch_failure()
         $this->expectException(InvalidArgumentException::class);
         $this->expectExceptionMessage('Source URL is null');
 
-        file_put_contents($this->storagePath, 'foo');
+        file_put_contents($this->storagePath, json_encode(['foo']));
 
         $this->app['config']['disposable-email.source'] = null;
 
-        $this->artisan('disposable:update');
-
-        $this->assertFileExists($this->storagePath);
-
-        $domains = $this->disposable()->getDomains();
-
-        $this->assertIsArray($domains);
-        $this->assertEquals(['foo'], $domains);
-    }
-
-    /** @test */
-    public function it_cannot_use_a_custom_fetcher_with_string_result()
-    {
-        $this->expectException(UnexpectedValueException::class);
-        $this->expectExceptionMessage('Provided data could not be parsed as a JSON list');
-
-        file_put_contents($this->storagePath, 'foo');
-
-        $this->app['config']['disposable-email.source'] = 'bar';
-        $this->app['config']['disposable-email.fetcher'] = CustomFetcherWithStringResult::class;
-
-        $this->artisan('disposable:update');
+        $this->artisan('disposable:update')
+            ->assertExitCode(1);
 
         $this->assertFileExists($this->storagePath);
 
@@ -79,64 +61,52 @@ public function it_cannot_use_a_custom_fetcher_with_string_result()
     }
 
     /** @test */
-    public function it_can_use_a_custom_fetcher_with_json_result()
+    public function custom_fetchers_need_fetcher_contract()
     {
-        file_put_contents($this->storagePath, 'foo');
+        file_put_contents($this->storagePath, json_encode(['foo']));
 
         $this->app['config']['disposable-email.source'] = 'bar';
-        $this->app['config']['disposable-email.fetcher'] = CustomFetcherWithJSONResult::class;
+        $this->app['config']['disposable-email.fetcher'] = InvalidFetcher::class;
 
-        $this->artisan('disposable:update');
+        $this->artisan('disposable:update')
+            ->assertExitCode(1);
 
         $this->assertFileExists($this->storagePath);
 
         $domains = $this->disposable()->getDomains();
-
-        $this->assertIsArray($domains);
-        $this->assertEquals(['bar'], $domains);
         $this->assertNotEquals(['foo'], $domains);
     }
 
     /** @test */
-    public function it_can_use_a_custom_fetcher_with_array_result()
+    public function it_can_use_a_custom_fetcher()
     {
-        file_put_contents($this->storagePath, 'foo');
+        file_put_contents($this->storagePath, json_encode(['foo']));
 
         $this->app['config']['disposable-email.source'] = 'bar';
-        $this->app['config']['disposable-email.fetcher'] = CustomFetcherWithArrayResult::class;
+        $this->app['config']['disposable-email.fetcher'] = CustomFetcher::class;
 
-        $this->artisan('disposable:update');
+        $this->artisan('disposable:update')
+            ->assertExitCode(0);
 
         $this->assertFileExists($this->storagePath);
 
         $domains = $this->disposable()->getDomains();
-
-        $this->assertIsArray($domains);
         $this->assertEquals(['bar'], $domains);
-        $this->assertNotEquals(['foo'], $domains);
-    }
-}
-
-class CustomFetcherWithStringResult
-{
-    public function handle($url)
-    {
-        return $url;
     }
 }
 
-class CustomFetcherWithJSONResult
+class CustomFetcher implements Fetcher
 {
-    public function handle($url)
+    public function handle($url): array
     {
-        return json_encode($url);
+        return [$url];
     }
 }
 
-class CustomFetcherWithArrayResult
+class InvalidFetcher
 {
     public function handle($url)
     {
-        return [$url];
+        return $url;
     }
 }