filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
17.6k stars 2.75k forks source link

FileUpload callback behaviour #14025

Open dododedodonl opened 3 weeks ago

dododedodonl commented 3 weeks ago

Package

filament/forms

Package Version

v3.2.106

Laravel Version

v10.48.20

Livewire Version

v3.5.6

PHP Version

PHP 8.3.10

Problem description

When using the FileUpload field, there is inconsistent behaviour in when certain callbacks are triggered in regards to removing a file.

  1. when a file is uploaded by the user, afterStateUpdated() is called (on a create and edit form)
  2. when an uploaded file is removed by the user before the form is submitted, no callback is called, as removeUploadedFile() is called directly by the page
  3. when an already uploaded and stored file (eg. on an edit form) is removed by the user, deleteUploadedFile() is called, which calls removeUploadedFile() and a callback deleteUploadedFileUsing() afterwards

Expected behavior

A way to consistently trigger code after a user decided to remove an uploaded file which is not persisted by submitting the form.

Steps to reproduce

                Forms\Components\FileUpload::make('files')
                        ->multiple()
                        ->afterStateUpdated(static function (array $state): void {
                            // triggered after every upload
                        })
                        ->deleteUploadedFileUsing(static function (array $state, $file): void {
                            // triggered only after a file that is persisted (eg. on edit form) is removed
                        }),

Reproduction repository

https://github.com/dododedodonl/file-upload-behaviour

Donate 💰 to fund this issue

Fund with Polar

github-actions[bot] commented 3 weeks ago

Hey @dododedodonl! We're sorry to hear that you've hit this issue. 💛

However, it looks like you forgot to fill in the reproduction repository URL. Can you edit your original post and then we'll look at your issue?

We need a public GitHub repository which contains a Laravel app with the minimal amount of Filament code to reproduce the problem. Please do not link to your actual project, what we need instead is a minimal reproduction in a fresh project without any unnecessary code. This means it doesn't matter if your real project is private / confidential, since we want a link to a separate, isolated reproduction. That would allow us to download it and review your bug much easier, so it can be fixed quicker. Please make sure to include a database seeder with everything we need to set the app up quickly.

dododedodonl commented 1 week ago

@danharrin I want to create a PR for this; but I am not sure which approach to take for V3. I am leaning towards adding another callback (like deleteUploadedFileUsing()) that is called in removeUploadedFile (eg. removeUploadedFileUsing()), what do you think?

danharrin commented 1 week ago

if the bug is that deleteUploadedFileUsing is not triggered when it should be, can you not add the extra trigger instead of introducing the new callback?

dododedodonl commented 3 days ago

@danharrin I think the bug is that a user changes something in a form and the fields state is updated, but afterStateUpdated() is not called. However, I also think this is a big of a change to implement in V3 as it would probably break quite some logic already out there.

danharrin commented 3 days ago

I don't think deleteUploadedFileUsing should be called (theres no file to delete from the disk unless its persisted), but afterStateUpdated should be and it sounds like a bug