From e9b2b180938a604fa0ac756a1c6e3f5b858d09e0 Mon Sep 17 00:00:00 2001 From: Hugo Sales Date: Thu, 29 Apr 2021 18:12:32 +0000 Subject: [PATCH] [Avatar] Implement avatar deletion --- components/Avatar/Controller/Avatar.php | 41 +++++++++++++++++-------- src/Entity/Attachment.php | 33 +++++++++++--------- src/Entity/AttachmentThumbnail.php | 16 +++++++--- src/Entity/Avatar.php | 33 +++++++++++--------- 4 files changed, 78 insertions(+), 45 deletions(-) diff --git a/components/Avatar/Controller/Avatar.php b/components/Avatar/Controller/Avatar.php index 4ac333ce55..8c89a7f8e6 100644 --- a/components/Avatar/Controller/Avatar.php +++ b/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')]], ]); @@ -64,8 +72,17 @@ class Avatar extends Controller $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { - $data = $form->getData(); + $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,26 +106,24 @@ class Avatar extends Controller } else { throw new ClientException('Invalid form'); } - $user = Common::user(); - $gsactor_id = $user->getId(); - $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]); + $attachment = GSFile::validateAndStoreAttachment($sfile, Common::config('avatar', 'dir'), $title = null, $is_local = true, $use_unique = $gsactor_id); + $old_attachment = 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) { - @unlink($old_file); + if ($old_attachment != null) { + @unlink($old_attachment); } + Event::handle('DeleteCachedAvatar', [$user->getId()]); } - Event::handle('DeleteCachedAvatar', [$user->getId()]); } return ['_template' => 'settings/avatar.html.twig', 'avatar' => $form->createView()]; diff --git a/src/Entity/Attachment.php b/src/Entity/Attachment.php index e08359f31f..10520e6d7c 100644 --- a/src/Entity/Attachment.php +++ b/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(); - $a->delete($flush, $delete_files_now, $cascading = true); + $files[] = $a->getPath(); + $a->delete(cascade: false, flush: false); } - foreach (DB::findBy('attachment_thumbnail', ['attachment_id' => $this->id]) as $ft) { - $files[] = $ft->delete($flush, $delete_files_now, $cascading); + foreach ($this->getThumbnails() as $at) { + $files[] = $at->getPath(); + $at->delete(flush: false); } } + $files[] = $this->getPath(); DB::remove($this); if ($flush) { DB::flush(); } - if ($delete_files_now) { - self::deleteFiles($files); - return []; + foreach ($files as $f) { + if (file_exists($f)) { + 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) { - @unlink($f); - } + return DB::findBy('attachment_thumbnail', ['attachment_id' => $this->id]); } public function getPath() diff --git a/src/Entity/AttachmentThumbnail.php b/src/Entity/AttachmentThumbnail.php index bd98dcf227..1b25b03e73 100644 --- a/src/Entity/AttachmentThumbnail.php +++ b/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 - return ''; + $filepath = $this->getPath(); + 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 diff --git a/src/Entity/Avatar.php b/src/Entity/Avatar.php index 3c4e8bf3de..df6f35eb86 100644 --- a/src/Entity/Avatar.php +++ b/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 (!$cascading) { - $files = $this->getAttachment()->delete($cascade = true, $file_flush = false, $delete_files_now); - } else { - DB::remove(DB::getReference('avatar', ['gsactor_id' => $this->gsactor_id])); - $file_path = $this->getFilePath(); - $files[] = $file_path; - if ($flush) { - DB::flush(); + if ($cascade) { + // Avatar doesn't own the file, but it's stored in a different place than Attachment + // would think, so we need to handle it ourselves. Since the attachment could be shared, + // can only delete if cascading + $filepath = $this->getPath(); + if (file_exists($filepath)) { + if (@unlink($filepath) === false) { + Log::warning("Failed deleting attachment for avatar with id={$id} at {$filepath}"); + } } - return $delete_files_now ? [] : $files; + $this->attachment->delete(cascade: true, flush: false); + } + DB::remove($this); + if ($flush) { + DB::flush(); } - return []; } public static function schemaDef(): array