[Avatar] Implement avatar deletion

components/Avatar/Controller/Avatar.php

@@ -22,15 +22,23 @@
 namespace Component\Avatar\Controller;
 
 use App\Core\Controller;
+use App\Core\DB\DB;
+use App\Core\Event;
 use App\Core\Form;
+use App\Core\GSFile;
 use App\Core\GSFile as M;
 use function App\Core\I18n\_m;
+use App\Entity\Avatar as AvatarEntity;
+use App\Util\Common;
+use App\Util\Exception\NotFoundException;
 use App\Util\TemporaryFile;
 use Exception;
 use Symfony\Component\Form\Extension\Core\Type\CheckboxType;
 use Symfony\Component\Form\Extension\Core\Type\FileType;
 use Symfony\Component\Form\Extension\Core\Type\HiddenType;
 use Symfony\Component\Form\Extension\Core\Type\SubmitType;
+use Symfony\Component\Form\FormError;
+use Symfony\Component\HttpFoundation\File\File as SymfonyFile;
 use Symfony\Component\HttpFoundation\Request;
 
 class Avatar extends Controller
@@ -56,7 +64,7 @@ class Avatar extends Controller
     {
         $form = Form::create([
             ['avatar', FileType::class,     ['label' => _m('Avatar'), 'help' => _m('You can upload your personal avatar. The maximum file size is 2MB.'), 'multiple' => false, 'required' => false]],
-            ['remove', CheckboxType::class, ['label' => _m('Remove avatar'), 'help' => _m('Remove your avatar and use the default one')]],
+            ['remove', CheckboxType::class, ['label' => _m('Remove avatar'), 'help' => _m('Remove your avatar and use the default one'), 'required' => false, 'value' => false]],
             ['hidden', HiddenType::class,   []],
             ['save',   SubmitType::class,   ['label' => _m('Submit')]],
         ]);
@@ -65,7 +73,16 @@ class Avatar extends Controller
 
         if ($form->isSubmitted() && $form->isValid()) {
             $data       = $form->getData();
+            $user       = Common::user();
+            $gsactor_id = $user->getId();
             if ($data['remove'] == true) {
+                try {
+                    $avatar = DB::findOneBy('avatar', ['gsactor_id' => $gsactor_id]);
+                    $avatar->delete();
+                    Event::handle('DeleteCachedAvatar', [$user->getId()]);
+                } catch (NotFoundException) {
+                    $form->addError(new FormError(_m('No avatar set, so cannot delete')));
+                }
             } else {
                 $sfile = null;
                 if (isset($data['hidden'])) {
@@ -89,27 +106,25 @@ class Avatar extends Controller
                 } else {
                     throw new ClientException('Invalid form');
                 }
-                $user       = Common::user();
+                $attachment     = GSFile::validateAndStoreAttachment($sfile, Common::config('avatar', 'dir'), $title = null, $is_local = true, $use_unique = $gsactor_id);
-                $gsactor_id = $user->getId();
+                $old_attachment = null;
-                $file       = GSFile::validateAndStoreAttachment($sfile, Common::config('avatar', 'dir'), $title = null, $is_local = true, $use_unique = $gsactor_id);
-                $old_file   = null;
                 $avatar         = DB::find('avatar', ['gsactor_id' => $gsactor_id]);
                 // Must get old id before inserting another one
                 if ($avatar != null) {
-                    $old_file = $avatar->delete();
+                    $old_attachment = $avatar->delete();
                 }
-                DB::persist($file);
+                DB::persist($attachment);
                 // Can only get new id after inserting
                 DB::flush();
-                DB::persist(self::create(['gsactor_id' => $gsactor_id, 'attachment_id' => $file->getId()]));
+                DB::persist(AvatarEntity::create(['gsactor_id' => $gsactor_id, 'attachment_id' => $attachment->getId()]));
                 DB::flush();
                 // Only delete files if the commit went through
-                if ($old_file != null) {
+                if ($old_attachment != null) {
-                    @unlink($old_file);
+                    @unlink($old_attachment);
-                }
                 }
                 Event::handle('DeleteCachedAvatar', [$user->getId()]);
             }
+        }
 
         return ['_template' => 'settings/avatar.html.twig', 'avatar' => $form->createView()];
     }

src/Entity/Attachment.php

@@ -194,38 +194,43 @@ class Attachment extends Entity
     const FILEHASH_ALGO = 'sha256';
 
     /**
-     * Delete this file and by default all the associated entities (avatar and/or thumbnails, which this owns)
+     * Delete this attachment and optianlly all the associated entities (avatar and/or thumbnails, which this owns)
      */
-    public function delete(bool $cascade = true, bool $flush = false, bool $delete_files_now = false): array
+    public function delete(bool $cascade = true, bool $flush = true): void
     {
         $files = [];
         if ($cascade) {
             // An avatar can own a file, and it becomes invalid if the file is deleted
             $avatar = DB::findBy('avatar', ['attachment_id' => $this->id]);
             foreach ($avatar as $a) {
-                $files[] = $a->getFilePath();
+                $files[] = $a->getPath();
-                $a->delete($flush, $delete_files_now, $cascading = true);
+                $a->delete(cascade: false, flush: false);
             }
-            foreach (DB::findBy('attachment_thumbnail', ['attachment_id' => $this->id]) as $ft) {
+            foreach ($this->getThumbnails() as $at) {
-                $files[] = $ft->delete($flush, $delete_files_now, $cascading);
+                $files[] = $at->getPath();
+                $at->delete(flush: false);
             }
         }
+        $files[] = $this->getPath();
         DB::remove($this);
         if ($flush) {
             DB::flush();
         }
-        if ($delete_files_now) {
+        foreach ($files as $f) {
-            self::deleteFiles($files);
+            if (file_exists($f)) {
-            return [];
+                if (@unlink($f) === false) {
+                    Log::warning("Failed deleting file for attachment with id={$this->id} at {$f}");
+                }
+            }
         }
-        return $files;
     }
 
-    public static function deleteFiles(array $files)
+    /**
+     * Find all thumbnails associated with this attachment. Don't bother caching as this is not supposed to be a common operation
+     */
+    public function getThumbnails()
     {
-        foreach ($files as $f) {
+        return DB::findBy('attachment_thumbnail', ['attachment_id' => $this->id]);
-            @unlink($f);
-        }
     }
 
     public function getPath()

src/Entity/AttachmentThumbnail.php

@@ -171,12 +171,20 @@ class AttachmentThumbnail extends Entity
     }
 
     /**
-     * Delete a attachment thumbnail. This table doesn't own all the attachments, only itself
+     * Delete an attachment thumbnail
      */
-    public function delete(bool $flush = false, bool $delete_attachments_now = false, bool $cascading = false): string
+    public function delete(bool $flush = true): void
     {
-        // TODO Implement deleting attachment thumbnails
+        $filepath = $this->getPath();
-        return '';
+        if (file_exists($filepath)) {
+            if (@unlink($filepath) === false) {
+                Log::warning("Failed deleting file for attachment thumbnail with id={$this->attachment_id}, width={$this->width}, height={$this->height} at {$filepath}");
+            }
+        }
+        DB::remove($this);
+        if ($flush) {
+            DB::flush();
+        }
     }
 
     public static function schemaDef(): array

src/Entity/Avatar.php

@@ -104,7 +104,7 @@ class Avatar extends Entity
 
     public function getAttachment(): Attachment
     {
-        $this->attachment = $this->attachment ?: DB::find('attachment', ['id' => $this->attachment_id]);
+        $this->attachment = $this->attachment ?: DB::findOneBy('attachment', ['id' => $this->attachment_id]);
         return $this->attachment;
     }
 
@@ -113,29 +113,34 @@ class Avatar extends Entity
         return Common::config('avatar', 'dir') . $filename;
     }
 
-    public function getFilePath(): string
+    public function getPath(): string
     {
         return Common::config('avatar', 'dir') . $this->getAttachment()->getFileName();
     }
 
     /**
      * Delete this avatar and the corresponding file and thumbnails, which this owns
+     *
+     * Inefficient implementation, but there are plenty of edge cases and this is supposed to be a rare operation
      */
-    public function delete(bool $flush = false, bool $delete_files_now = false, bool $cascading = false): array
+    public function delete(bool $cascade = true, bool $flush = true): void
     {
-        // Don't go into a loop if we're deleting from File
+        if ($cascade) {
-        if (!$cascading) {
+            // Avatar doesn't own the file, but it's stored in a different place than Attachment
-            $files = $this->getAttachment()->delete($cascade = true, $file_flush = false, $delete_files_now);
+            // would think, so we need to handle it ourselves. Since the attachment could be shared,
-        } else {
+            // can only delete if cascading
-            DB::remove(DB::getReference('avatar', ['gsactor_id' => $this->gsactor_id]));
+            $filepath = $this->getPath();
-            $file_path = $this->getFilePath();
+            if (file_exists($filepath)) {
-            $files[]   = $file_path;
+                if (@unlink($filepath) === false) {
+                    Log::warning("Failed deleting attachment for avatar with id={$id} at {$filepath}");
+                }
+            }
+            $this->attachment->delete(cascade: true, flush: false);
+        }
+        DB::remove($this);
         if ($flush) {
             DB::flush();
         }
-            return $delete_files_now ? [] : $files;
-        }
-        return [];
     }
 
     public static function schemaDef(): array