Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple serializable temporary filesystem #91

Open
wants to merge 7 commits into
base: 1.x
Choose a base branch
from

Conversation

Lustmored
Copy link
Contributor

@Lustmored Lustmored commented Feb 24, 2023

In this PR I have collected 2 different topics, but both are pretty straight forward.

First of them is bringing ValueResolver from document-library and extended to support PendingImage. I think it needs no comment 馃樃

Second one is a temporary file decorated filesystem. It's purpose is to store uploaded files with original filenames so that they can be serialized and moved to the final filesystem (with all the naming etc.) automatically via doctrine listeners. The implementation is simpler than I initially thought, but hopefully complete.

I am not really happy with TemporaryFilesystem name, but couldn't find any better. The idea is that upon validation of PendingFile/PendingImage we can call $file->saveToTemporary($filesystem) and store the result in session (or elsewhere, serialized) only to move to persistent filesystem when required - this part is exactly what's the most complicated in handling files in live components without this library.

It is a proof of concept, I should add tests and a bit of docs, but for now I prefer to show you something and polish it when we have agreed direction for those.

This library already provides a purge filesystem command that could be ran via CRON to clean the temporary filesystem from old files.

@kbond
Copy link
Member

kbond commented Feb 24, 2023

First of them is bringing ValueResolver from document-library and extended to support PendingImage. I think it needs no comment smile_cat

Thanks, I'd forgotten about this! Could you split this out into it's own PR?


Regarding temporary files, this is sort of what I was envisioning:

  1. In config, user defines a temp filesystem (we'd add by default with a recipe in public/files/temp with a url_prefix as /files/temp)
  2. The UploadedFile attribute (or maybe a new TempFile attribute) could accept the temp filesystem to save there in a unique way. So in your controller (and in the future, live components), you could mark controller arguments as #[UploadedFile(filesystem: 'temp')] File $file (note the File type instead of PendingFile - this would change the behaviour of the resolver).
  3. For purging, we'd instruct users to run bin/console zenstruck:filesystem:purge temp --older-than="24 hours" as a cron job.

I think we could have the same functionality in a new form type (#87)

@Lustmored
Copy link
Contributor Author

Thanks, I'd forgotten about this! Could you split this out into it's own PR?

Sure, will do

2. The UploadedFile attribute (or maybe a new TempFile attribute) could accept the temp filesystem to save there in a unique way. So in your controller (and in the future, live components), you could mark controller arguments as #[UploadedFile(filesystem: 'temp')] File $file (note the File type instead of PendingFile - this would change the behaviour of the resolver).

I though about it and decided against it because we'd need to at least add validation logic into the value resolver (we don't want to store files we can easily discard by validation) and possibly some manipulators (although I think transformInPlace in controller should be enough for this one). But maybe a new attribute with signature like:
TemporaryFile(?string $path = null, array $constraints = [])
would be enough. Value resolver would then validate the file and, if valid, store it to the temporary filesystem (with original filename for later Namer usage). I'm not sure what to do when the validation fails - yet another File offspring (InvalidFile?) with list of constraint violations? Or maybe original PendingFile with violations added somewhere...?

I have specifically omitted the filesystem from declaration. If we want to configure a single filesystem for temporary storage (I think it makes sense), we should avoid allowing breaking this by configuring another filesystem on the attribute level.

@kbond
Copy link
Member

kbond commented Feb 24, 2023

(we don't want to store files we can easily discard by validation)

True. I like the idea of adding constraints. Maybe we should auto-add the ImageConstraint if expecting an Image? Instead of returning null if not an image, we'd fail.

For what to do when the constraints fail? I think throwing a 422 (customizable) would be best. There are some PR's in Symfony core that are looking to add this feature to request parameters. ie: #[Assert(errorCode: 422, [new Length(123), ...])] string $value so this would be consistent.

If you want to do something else like collect the validation errors and return. Thinking this would need to be done manually by either accepting a PendingFile and validating yourself or using the form system.

What I'm trying to avoid is a 1st party concept of a temporary file/filesystem. I'd like this to be just another filesystem.

What I'm thinking for the attribute:

public function __construct(
    ?string $path = null,
    bool $image = false,
    list<Constraint> $constraints = [],
    ?string $filesystem = null, // required if type hint is File/Image
    Namer|string|null $namer = new Expression('{checksum}/{name}{ext}'), // used if type hint is File/Image
) {}

The above is probably too much for one attribute so maybe we could split into PendingUploadedFile and UploadedFile?

@kbond
Copy link
Member

kbond commented Feb 24, 2023

Another thought I had about this: while I understand it's a waste of resources to save a file that's going to be discarded because it fails validation, it would make the validation of files consistent with other validators. Consider a text field that has a Length constraint. When there is a validation error, the wrong value isn't discarded, it's returned to the user.

Also, consider a future enhancement, that allows users to upload files directly to an s3 bucket (on submit, the form sends the link to the file instead of the file itself). The file exists and will eventually be discarded after validation fails. This would be consistent with that.

@Lustmored Lustmored changed the title Value resolver and simply serializable temporary filesystem Simple serializable temporary filesystem Mar 2, 2023
@Lustmored Lustmored mentioned this pull request Mar 2, 2023
@Lustmored
Copy link
Contributor Author

I have split this PR into two, for now without any changes. I have noticed that due to decoration of nodes my logic regarding temporary files is completely wrong here. I will work on it more tomorrow 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants