fix(media): copy and delete temp file when saving instead of moving it for FS FileManager

+ throw exception instead silently failing file save

closes #338

modules/Media/Entities/BaseMedia.php

@@ -13,6 +13,7 @@ namespace Modules\Media\Entities;
 use CodeIgniter\Entity\Entity;
 use CodeIgniter\Files\File;
 use Modules\Media\Models\MediaModel;
+use RuntimeException;
 
 /**
  * @property int $id
@@ -97,15 +98,13 @@ class BaseMedia extends Entity
         return $this;
     }
 
-    public function saveFile(): bool
+    public function saveFile(): void
     {
         if (! $this->attributes['file'] || ! $this->file_key) {
-            return false;
+            throw new RuntimeException("'file' and 'file_key' attributes must be set before saving a file.");
         }
 
         $this->attributes['file_key'] = service('file_manager')->save($this->attributes['file'], $this->file_key);
-
-        return true;
     }
 
     public function deleteFile(): bool
@@ -128,6 +127,7 @@ class BaseMedia extends Entity
 
         if (! service('file_manager')->rename($this->file_key, $newFileKey)) {
             $db->transRollback();
+
             return false;
         }
 

modules/Media/Entities/Image.php

@@ -94,14 +94,14 @@ class Image extends BaseMedia
         return $this;
     }
 
-    public function saveFile(): bool
+    public function saveFile(): void
     {
         if ($this->attributes['sizes'] !== []) {
             $this->initFileProperties();
             $this->saveSizes();
         }
 
-        return parent::saveFile();
+        parent::saveFile();
     }
 
     public function deleteFile(): bool

modules/Media/Entities/Transcript.php

@@ -11,6 +11,7 @@ declare(strict_types=1);
 namespace Modules\Media\Entities;
 
 use CodeIgniter\Files\File;
+use Exception;
 use Modules\Media\TranscriptParser;
 
 class Transcript extends BaseMedia
@@ -53,11 +54,11 @@ class Transcript extends BaseMedia
         return $this;
     }
 
-    public function saveFile(): bool
+    public function saveFile(): void
     {
         $this->saveJsonTranscript();
 
-        return parent::saveFile();
+        parent::saveFile();
     }
 
     public function deleteFile(): bool
@@ -73,19 +74,18 @@ class Transcript extends BaseMedia
         return true;
     }
 
-    private function saveJsonTranscript(): bool
+    private function saveJsonTranscript(): void
     {
         $srtContent = file_get_contents($this->file->getRealPath());
 
         $transcriptParser = new TranscriptParser();
 
         if ($srtContent === false) {
-            return false;
+            throw new Exception('Could not read transcript file at ' . $this->file->getRealPath());
         }
 
-        if (! $transcriptJson = $transcriptParser->loadString($srtContent)->parseSrt()) {
+        $transcriptJson = $transcriptParser->loadString($srtContent)
-            return false;
+            ->parseSrt();
-        }
 
         $tempFilePath = WRITEPATH . 'uploads/' . $this->file->getRandomName();
         file_put_contents($tempFilePath, $transcriptJson);
@@ -94,7 +94,5 @@ class Transcript extends BaseMedia
 
         service('file_manager')
             ->save($newTranscriptJson, $this->json_key);
-
-        return true;
     }
 }

modules/Media/FileManagers/FS.php

@@ -20,32 +20,32 @@ class FS implements FileManagerInterface
     /**
      * Saves a file to the corresponding folder in `public/media`
      */
-    public function save(File $file, string $path): string | false
+    public function save(File $file, string $key): string
     {
         helper('media');
 
-        if ((pathinfo($path, PATHINFO_EXTENSION) === '') && (($extension = $file->getExtension()) !== '')) {
-            $path = $path . '.' . $extension;
-        }
-
         $mediaRoot = $this->media_path_absolute();
+        $path = $mediaRoot . '/' . $key;
 
-        if (! file_exists(dirname($mediaRoot . '/' . $path))) {
+        if (! file_exists(dirname($path))) {
-            mkdir(dirname($mediaRoot . '/' . $path), 0777, true);
+            mkdir(dirname($path), 0777, true);
         }
 
-        if (! file_exists(dirname($mediaRoot . '/' . $path) . '/index.html')) {
+        if (! file_exists(dirname($path) . '/index.html')) {
-            touch(dirname($mediaRoot . '/' . $path) . '/index.html');
+            touch(dirname($path) . '/index.html');
         }
 
-        try {
+        // copy to media folder, overwrite file if already existing
-            // move to media folder, overwrite file if already existing
+        $isCopySuccessful = copy($file->getRealPath(), $path);
-            $file->move($mediaRoot . '/', $path, true);
+
-        } catch (Exception) {
+        if (! $isCopySuccessful) {
-            return false;
+            throw new Exception("Could not save file {$key} to {$path}");
         }
 
-        return $path;
+        // delete temporary file after copy
+        unlink($file->getRealPath());
+
+        return $key;
     }
 
     public function delete(string $key): bool

modules/Media/FileManagers/FileManagerInterface.php

@@ -9,7 +9,7 @@ use CodeIgniter\HTTP\Response;
 
 interface FileManagerInterface
 {
-    public function save(File $file, string $key): string | false;
+    public function save(File $file, string $key): string;
 
     public function delete(string $key): bool;
 

modules/Media/FileManagers/S3.php

@@ -30,19 +30,15 @@ class S3 implements FileManagerInterface
         ]);
     }
 
-    public function save(File $file, string $key): string|false
+    public function save(File $file, string $key): string
     {
-        try {
+        $this->s3->putObject([
-            $this->s3->putObject([
+            'Bucket'       => $this->config->s3['bucket'],
-                'Bucket'       => $this->config->s3['bucket'],
+            'Key'          => $this->prefixKey($key),
-                'Key'          => $this->prefixKey($key),
+            'SourceFile'   => $file,
-                'SourceFile'   => $file,
+            'ContentType'  => $file->getMimeType(),
-                'ContentType'  => $file->getMimeType(),
+            'CacheControl' => 'max-age=' . YEAR,
-                'CacheControl' => 'max-age=' . YEAR,
+        ]);
-            ]);
-        } catch (Exception) {
-            return false;
-        }
 
         // delete file after storage in s3
         unlink($file->getRealPath());

modules/Media/Models/MediaModel.php

@@ -117,12 +117,13 @@ class MediaModel extends Model
     public function saveMedia(object $media): int | false
     {
         // save file first
-        if (! $media->saveFile()) {
+        $media->saveFile();
-            return false;
-        }
 
         // insert record in database
-        if (! $mediaId = $this->insert($media, true)) {
+        /** @var int|false $mediaId */
+        $mediaId = $this->insert($media, true);
+
+        if (! $mediaId) {
             $this->db->transRollback();
 
             return false;
@@ -137,10 +138,10 @@ class MediaModel extends Model
     public function updateMedia(object $media): bool
     {
         // save file first
-        if (! $media->saveFile()) {
+        // FIXME: what if file is not set?
-            return false;
+        $media->saveFile();
-        }
 
+        // update record in database
         return $this->update($media->id, $media);
     }
 

modules/Media/TranscriptParser.php

@@ -12,6 +12,7 @@ declare(strict_types=1);
 
 namespace Modules\Media;
 
+use Exception;
 use stdClass;
 
 class TranscriptParser
@@ -27,8 +28,10 @@ class TranscriptParser
 
     /**
      * Adapted from: https://stackoverflow.com/a/11659306
+     *
+     * @return string Returns the json encoded string
      */
-    public function parseSrt(): string | false
+    public function parseSrt(): string
     {
         if (! defined('SRT_STATE_SUBNUMBER')) {
             define('SRT_STATE_SUBNUMBER', 0);
@@ -98,7 +101,13 @@ class TranscriptParser
             $subs[] = $sub;
         }
 
-        return json_encode($subs, JSON_PRETTY_PRINT);
+        $jsonString = json_encode($subs, JSON_PRETTY_PRINT);
+
+        if (! $jsonString) {
+            throw new Exception('Failed to parse SRT to JSON.');
+        }
+
+        return $jsonString;
     }
 
     private function getSecondsFromTimeString(string $timeString): float

modules/PodcastImport/Commands/PodcastImport.php

@@ -46,8 +46,6 @@ class PodcastImport extends BaseCommand
 
     public function init(): void
     {
-        CLI::clearScreen();
-
         helper('podcast_import');
 
         $importQueue = get_import_tasks();
@@ -97,9 +95,9 @@ class PodcastImport extends BaseCommand
 
     public function run(array $params): void
     {
-        try {
+        $this->init();
-            $this->init();
 
+        try {
             CLI::write('All good! Feed was parsed successfully!');
 
             CLI::write(

modules/WebSub/Commands/Publish.php

@@ -69,7 +69,7 @@ class Publish extends BaseCommand
                     $request->post($hub, $requestOptions);
                 } catch (Exception $exception) {
                     log_message(
-                        'critical',
+                        'warning',
                         "COULD NOT PUBLISH @{$podcast->handle} ON {$hub}" . PHP_EOL . $exception->getMessage()
                     );
                 }