Uploaders - Refactor and fixes

.gitignore

@@ -7,4 +7,5 @@ composer.lock
 .phpunit.result.cache
 src/public/packages/
 /.phpunit.cache
+coverage/
 

composer.json

@@ -62,9 +62,12 @@
         ]
     },
     "scripts": {
-        "test": "vendor/bin/phpunit --testdox",
+        "test": [
+            "@putenv XDEBUG_MODE=off",
+            "vendor/bin/phpunit"
+        ],
         "test-failing": "vendor/bin/phpunit --order-by=defects --stop-on-failure",
-        "test-coverage": "XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-text"
+        "test-coverage": "XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-html=coverage"
     },
     "extra": {
         "branch-alias": {

phpunit.xml

@@ -23,6 +23,7 @@
     <include>
       <directory suffix=".php">./src/app/Library/CrudPanel/Traits/</directory>
       <directory>./src/app/Library/Validation/</directory>
+      <directory>./src/app/Library/Uploaders/</directory>
       <directory suffix=".php">./src/app/Library/CrudPanel/</directory>
       <directory suffix=".php">./src/app/Models/Traits/</directory>
       <file>./src/app/Library/Widget.php</file>
@@ -35,9 +36,11 @@
   </source>
   <php>
         <env name="APP_ENV" value="testing"/>
+        <env name="APP_KEY" value="AckfSECXIvnK5r28GVIWUAxmbBSjTsmF"/>
         <env name="BCRYPT_ROUNDS" value="12"/>
         <env name="CACHE_DRIVER" value="array"/>
         <env name="DB_CONNECTION" value="sqlite"/>
+        <env name="DB_FOREIGN_KEYS" value="true"/>
         <env name="DB_DATABASE" value=":memory:"/>
         <env name="MAIL_MAILER" value="array"/>
         <env name="QUEUE_CONNECTION" value="sync"/>

src/app/Library/CrudPanel/Traits/FieldsProtectedMethods.php

@@ -281,6 +281,8 @@ protected function makeSureSubfieldsHaveNecessaryAttributes($field)
             $subfield['name'] = Str::replace(' ', '', $subfield['name']);
 
             $subfield['parentFieldName'] = $field['name'];
+            $subfield['baseFieldName'] = is_array($subfield['name']) ? implode(',', $subfield['name']) : $subfield['name'];
+            $subfield['baseFieldName'] = Str::afterLast($subfield['baseFieldName'], '.');
 
             if (! isset($field['model'])) {
                 // we're inside a simple 'repeatable' with no model/relationship, so
@@ -294,8 +296,6 @@ protected function makeSureSubfieldsHaveNecessaryAttributes($field)
                 $currentEntity = $subfield['baseEntity'] ?? $field['entity'];
                 $subfield['baseModel'] = $subfield['baseModel'] ?? $field['model'];
                 $subfield['baseEntity'] = isset($field['baseEntity']) ? $field['baseEntity'].'.'.$currentEntity : $currentEntity;
-                $subfield['baseFieldName'] = is_array($subfield['name']) ? implode(',', $subfield['name']) : $subfield['name'];
-                $subfield['baseFieldName'] = Str::afterLast($subfield['baseFieldName'], '.');
             }
 
             $field['subfields'][$key] = $this->makeSureFieldHasNecessaryAttributes($subfield);

src/app/Library/Uploaders/MultipleFiles.php

@@ -57,9 +57,16 @@ public function uploadFiles(Model $entry, $value = null)
             }
         }
 
-        return isset($entry->getCasts()[$this->getName()]) ? $previousFiles : json_encode($previousFiles);
+        $previousFiles = array_values($previousFiles);
+
+        if (empty($previousFiles)) {
+            return null;
+        }
+
+        return isset($entry->getCasts()[$this->getName()]) || $this->isFake() ? $previousFiles : json_encode($previousFiles);
     }
 
+    /** @codeCoverageIgnore */
     public function uploadRepeatableFiles($files, $previousRepeatableValues, $entry = null)
     {
         $fileOrder = $this->getFileOrderFromRequest();
@@ -73,11 +80,26 @@ public function uploadRepeatableFiles($files, $previousRepeatableValues, $entry
                 }
             }
         }
+        // create a temporary variable that we can unset keys
+        // everytime one is found. That way we avoid iterating
+        // already handled keys (notice we do a deep array copy)
+        $tempFileOrder = array_map(function ($item) {
+            return $item;
+        }, $fileOrder);
 
         foreach ($previousRepeatableValues as $previousRow => $previousFiles) {
             foreach ($previousFiles ?? [] as $key => $file) {
-                $key = array_search($file, $fileOrder, true);
-                if ($key === false) {
+                $previousFileInArray = array_filter($tempFileOrder, function ($items, $key) use ($file, $tempFileOrder) {
+                    $found = array_search($file, $items ?? [], true);
+                    if ($found !== false) {
+                        Arr::forget($tempFileOrder, $key.'.'.$found);
+
+                        return true;
+                    }
+
+                    return false;
+                }, ARRAY_FILTER_USE_BOTH);
+                if ($file && ! $previousFileInArray) {
                     Storage::disk($this->getDisk())->delete($file);
                 }
             }

src/app/Library/Uploaders/SingleBase64Image.php

@@ -7,6 +7,7 @@
 use Illuminate\Support\Facades\Storage;
 use Illuminate\Support\Str;
 
+/** @codeCoverageIgnore */
 class SingleBase64Image extends Uploader
 {
     public function uploadFiles(Model $entry, $value = null)
@@ -51,7 +52,7 @@ public function uploadRepeatableFiles($values, $previousRepeatableValues, $entry
             }
         }
 
-        $imagesToDelete = array_diff($previousRepeatableValues, $values);
+        $imagesToDelete = array_diff(array_filter($previousRepeatableValues), $values);
 
         foreach ($imagesToDelete as $image) {
             Storage::disk($this->getDisk())->delete($image);
@@ -65,7 +66,7 @@ protected function shouldUploadFiles($value): bool
         return $value && is_string($value) && Str::startsWith($value, 'data:image');
     }
 
-    protected function shouldKeepPreviousValueUnchanged(Model $entry, $entryValue): bool
+    public function shouldKeepPreviousValueUnchanged(Model $entry, $entryValue): bool
     {
         return $entry->exists && is_string($entryValue) && ! Str::startsWith($entryValue, 'data:image');
     }

src/app/Library/Uploaders/SingleFile.php

@@ -38,6 +38,7 @@ public function uploadFiles(Model $entry, $value = null)
         return $previousFile;
     }
 
+    /** @codeCoverageIgnore */
     public function uploadRepeatableFiles($values, $previousRepeatableValues, $entry = null)
     {
         $orderedFiles = $this->getFileOrderFromRequest();
@@ -53,9 +54,13 @@ public function uploadRepeatableFiles($values, $previousRepeatableValues, $entry
         }
 
         foreach ($previousRepeatableValues as $row => $file) {
-            if ($file && ! isset($orderedFiles[$row])) {
-                $orderedFiles[$row] = null;
-                Storage::disk($this->getDisk())->delete($file);
+            if ($file) {
+                if (! isset($orderedFiles[$row])) {
+                    $orderedFiles[$row] = null;
+                }
+                if (! in_array($file, $orderedFiles)) {
+                    Storage::disk($this->getDisk())->delete($file);
+                }
             }
         }
 
@@ -65,7 +70,7 @@ public function uploadRepeatableFiles($values, $previousRepeatableValues, $entry
     /**
      * Single file uploaders send no value when they are not dirty.
      */
-    protected function shouldKeepPreviousValueUnchanged(Model $entry, $entryValue): bool
+    public function shouldKeepPreviousValueUnchanged(Model $entry, $entryValue): bool
     {
         return is_string($entryValue);
     }

src/app/Library/Uploaders/Support/Interfaces/UploaderInterface.php

@@ -30,6 +30,8 @@ public function repeats(string $repeatableContainerName): self;
 
     public function relationship(bool $isRelation): self;
 
+    public function fake(bool|string $isFake): self;
+
     /**
      * Getters.
      */
@@ -60,4 +62,10 @@ public function canHandleMultipleFiles(): bool;
     public function isRelationship(): bool;
 
     public function getPreviousFiles(Model $entry): mixed;
+
+    public function getValueWithoutPath(?string $value = null): ?string;
+
+    public function isFake(): bool;
+
+    public function getFakeAttribute(): bool|string;
 }

src/app/Library/Uploaders/Support/RegisterUploadEvents.php

@@ -60,6 +60,7 @@ private function registerSubfieldEvent(array $subfield, bool $registerModelEvent
         $uploader = $this->getUploader($subfield, $this->uploaderConfiguration);
         $crudObject = $this->crudObject->getAttributes();
         $uploader = $uploader->repeats($crudObject['name']);
+        $uploader = $uploader->fake((isset($crudObject['fake']) && $crudObject['fake']) ? ($crudObject['store_in'] ?? 'extras') : false);
 
         // If this uploader is already registered bail out. We may endup here multiple times when doing modifications to the crud object.
         // Changing `subfields` properties will call the macros again. We prevent duplicate entries by checking
@@ -139,6 +140,14 @@ private function setupModelEvents(string $model, UploaderInterface $uploader): v
             $uploader->deleteUploadedFiles($entry);
         });
 
+        // if the uploader is a relationship and handles repeatable files, we will also register the deleting event on the
+        // parent model. that way we can control the deletion of the files when the parent model is deleted.
+        if ($uploader->isRelationship() && $uploader->handleRepeatableFiles) {
+            app('crud')->model::deleting(function ($entry) use ($uploader) {
+                $uploader->deleteUploadedFiles($entry);
+            });
+        }
+
         app('UploadersRepository')->markAsHandled($uploader->getIdentifier());
     }
 
@@ -154,9 +163,13 @@ private function setupModelEvents(string $model, UploaderInterface $uploader): v
      */
     private function getUploader(array $crudObject, array $uploaderConfiguration): UploaderInterface
     {
-        $customUploader = isset($uploaderConfiguration['uploader']) && class_exists($uploaderConfiguration['uploader']);
+        $hasCustomUploader = isset($uploaderConfiguration['uploader']);
+
+        if ($hasCustomUploader && ! is_a($uploaderConfiguration['uploader'], UploaderInterface::class, true)) {
+            throw new Exception('Invalid uploader class provided for '.$this->crudObjectType.' type: '.$crudObject['type']);
+        }
 
-        if ($customUploader) {
+        if ($hasCustomUploader) {
             return $uploaderConfiguration['uploader']::for($crudObject, $uploaderConfiguration);
         }