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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Live][POC] alternate upload system #395

Closed
wants to merge 1 commit into from
Closed

Conversation

kbond
Copy link
Member

@kbond kbond commented Jul 15, 2022

Q A
Bug fix? no
New feature? yes
Tickets n/a
License MIT

Possible replacement/alternative to #299.

The idea here is to create an upload system similar to livewire:

  • Signed upload route/endpoint that takes x files, copies them to the above dir with a unique filename, returns a json list of these filenames (this url should expire)
  • Preview URL to preview temporary images (only images)
  • New LiveFile object that extends HttpFoundation's File (so file validations will work) but also has a url() method that returns /preview-url/unique_id.jpg
  • The above can be added as a property on your live component and hydrated/dehydrated with a LiveFileHydrator
  • For input[type=file] onblur, we can upload the file to upload url above, take the returned filename(s), set it/them on the component, validate, and access the url (so we can show previews like livewire)
  • User's "save" action would do something with these LiveFile's (ie send to s3 or whatever)
  • live:cleanup-temp-files command that deletes temp files older than x hours/days (can be scheduled on a cron)
  • Handle existing files? (ie a user's profile already has an image and they want to modify)
  • Delete existing files? (ie a user's profile image can be removed)

TODO:

  • finish tests
  • changelog
  • docs

@tgalopin
Copy link
Contributor

Would it make sense to connect this behavior to https://glide.thephpleague.com perhaps?

@kbond
Copy link
Member Author

kbond commented Jul 15, 2022

Would it make sense to connect this behavior to https://glide.thephpleague.com/ perhaps?

I suppose you mean for shrinking the preview images? Could the LazyImage component be used somehow? Maybe a ImagePreviewGenerator system: FullImagePreviewGenerator (default), BlurHashImagePreviewGenerator, GlideImagePreviewGenerator, ImagineImagePreviewGenerator?

@Lustmored
Copy link
Contributor

First of all - I really like the direction outlined here. In fact this is in line with my initial ideas for #299 that got somewhere lost between countless implementation details. I would love to see this pushed forward or maybe somehow merged with live controller code from my PR to avoid solving the same problems twice :)

I really like that you've chosen to save files to var instead of publicly available path - that solves quite some security headaches. I think going a little step forwarding and extracting save/preview logic to a service with defined interface (like saveFileToTemp(UploadedFile): string and previewTempFile(string $name): BinaryResponse @throws NotFoundException) would easily benefit users with custom requirements without raising complexity (myself included).

I have already solved implementation challenges to (at least partially) glue File class with Flysystem on one of my projects, therefore I'll be glad to provide Flysystem alternative for this feature upon finalizing interfaces. I think many consumers would appreciate that, especially in flows with many files being uploaded, where var size could be the limiting factor.

@kbond
Copy link
Member Author

kbond commented Jul 22, 2022

I think going a little step forwarding and extracting save/preview logic to a service with defined interface

Agreed, good idea!

I have already solved implementation challenges to (at least partially) glue File class with Flysystem on one of my projects,

I've been hacking on an alternate-flysystem-api library that provides something similar.


Hoping to circle back and iterate on this PR in the next few weeks. Hoping we can merge the work in #299 and I'm going to need help with the js.

Still thinking on how to handle the LiveFile property, some thoughts:

  • needs to be an instance of \SplFileInfo for validation
  • has two states:
    1. pending: user uploaded a file but the component hasn't been saved (in this case, it's url would be the preview url)
    2. existing: component was created with an existing file object. This is where things get murky for me: what's the url? what if the user uses a filesystem abstraction? How do we make it easy to use?

@tgalopin
Copy link
Contributor

I thought about this subject for a while. I have worked both on Glide (https://github.com/thephpleague/glide) and Flysystem (https://github.com/thephpleague/flysystem) in part because I think these two libraries have a role to play for this. I have implemented many variants of an upload system in my projects and I have some ideas to share.

I think this will quickly become larger than a "simple upload" system. We can probably achieve a "simple" version of an upload system at first (which this PR does wonderfully), but I do think we need to set up the proper structure right from the beginning to avoid being blocked when we will inevitably need to make it more generic.

IMO the challenge here is to create a well-thought abstraction for uploading, storing and displaying files: an abstraction that is extensible enough (to allow for manipulation before storage/before display, to allow for storage locally/somewhere remote, ...) while providing all the features we want.

From my experience, such abstraction needs to have several capabilities:

  • the ability to store files in different places (locally, S3, CDN, ...) ;
  • the ability to validate files easily (covered by Validator in this PR, which is great) ;
  • the ability to manipulate files before they are stored (resize images, encode them to JPG, decrease the quality, ... to save space) ;
  • the ability to invalidate HTTP cache (ie. I want each file to have a unique name so that cache is automatically invalidated on upload, and I want to be able to store this name in my database) ;

I'm thinking we may want to create a "UploaderInterface", and maybe have it in a dedicated component that the LiveComponent component could wire automatically when both are installed. Such LiveUpload component (name TBD) could then rely on Flysystem by default for storage, with a default local storage wired (https://github.com/thephpleague/flysystem-bundle), and maybe on Glide for image manipulation (so that it's easy to manipulate the image before storage and before display).

I have been wanting to implement such features in Glide for a while now (thephpleague/glide#288 (comment)) and never had time to do it, because I didn't have the need to. If this PR opens the possibility of creating such component, I'd be glad to start working for real on a Glide version that would benefit such upload system.

PS: I'm 100% fine with merging a simple version of the PR before starting the work on something more generic. However, we should be careful not to block ourselves with things we didn't anticipate.

@Lustmored
Copy link
Contributor

I'm all in for starting with contracts and building up from that. I think initial definition of UploaderInterface contract is the largest task (at least from backend perspective, wiring up JS is other thing). Once we all agree on it the implementation will probably become much easier if not trivial.

I have this topic at the back of my head most of the time and some ideas and conclusions come and go, so I'll take my time this Friday to sit and write a draft of just this interface (without any implementation) and will open a separate PR with just that. We can then discuss the scope and shape of it. Once finalized we can start discussion whether we want a new component with default implementations and what other pieces of the puzzle it could fit in.

I think if we do it right we might even want to promote it to symfony/contracts. Just a food for thought :)

@Lustmored
Copy link
Contributor

I have created a PR #404 with not really innovative interface. IT probably lacks many things, but I think we need to start somewhere :)

Creating another component for LiveUpload sounds more and more as a good idea as a scope of this topic is already becoming rather big. And using Glide/Intervention/Flysystem as a defaults sounds great to me. I am already utilizing all of them and have already worked on them (probably contributed to all), so if there is anything within those libraries that might help us - I would gladly help there too :)

I have recently found out about Intervention Image v3 being in alpha. This library is rather slow in development, but it might be worth to take a look at the changes for Glide 3 :)

@liliancarneau
Copy link

Hello guys,
What is the news about this feature ?
Thank you in advance

@kbond
Copy link
Member Author

kbond commented Aug 30, 2022

Still on our minds. @Lustmored, @weaverryan and I have been stepping back to take a discuss how best to provide filesystem abstraction in a way that would help here.

@veewee
Copy link
Contributor

veewee commented Apr 7, 2023

@kbond
Since we needed this functionallity in our app, I started from the provided controller and built (parts) of your task list in our application. It can currently:

  • upload files to the server (currently just an UploadedFile - no LiveFile)
  • link those files directly to a live-form
  • run validation on top of these files when submitting the form
  • cleanup old files through a cron
  • (It can also work together with the dropzone component if you overwrite the default form theme for that widget)

There were some nasty Symfony-forms related things to overcome if you want to use this in combination with the form-trait.
Therefore, you might be interested in this gist where you might continue from.

https://gist.github.com/veewee/471e35798761f1295482343972a468a7

@kbond
Copy link
Member Author

kbond commented Apr 17, 2023

Awesome, thanks @veewee. Will check out your gist.

For a status update, @Lustmored and I are (slowly) working on an experiment to use zenstruck/filesystem (filesystem abstraction system based on flysystem) as the storage abstraction for this feature.

@Lustmored recently added controller value resolvers for "files/images". We're hoping to try and reuse the attributes for a LiveFile/LiveImage system.

@kbond
Copy link
Member Author

kbond commented May 10, 2023

Closing in favor of #834 which is a better first step to achieve.

@kbond kbond closed this May 10, 2023
@kbond kbond deleted the live-uploads branch May 10, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants