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

Refactor the Uploading Document #2346

Closed
1 task
weaverryan opened this issue Mar 23, 2013 · 9 comments
Closed
1 task

Refactor the Uploading Document #2346

weaverryan opened this issue Mar 23, 2013 · 9 comments
Labels
actionable Clear and specific issues ready for anyone to take them. Doctrine DX HttpFoundation

Comments

@weaverryan
Copy link
Member

The document in question: http://symfony.com/doc/current/cookbook/doctrine/file_uploads.html

There are a number of open issues about this document:

Here's a summary of issues:

a) I really dislike the use of paths inside the entity itself

b) Complication of the callbacks not triggering if only the file object property is changed (since it's not mapped). This may not be avoidable, but should be solved as simply as possible).

c) I agree that the VichUploaderBundle should be suggested to help with much of this

Suggestions: One big theme is building-up in complexity from little to more.

  1. We should first show how to upload files. It's very easy and includes implementing the UploadedFile object which you can get off of the Request. This should live as a little "cookbook" in the HttpFoundation component

  2. Have a cookbook article that talks about uploading in Symfony, but nothing about Doctrine. This would include using a Form, validation, and moving the uploaded file in the controller.

  3. Update the Doctrine file uploads doc to build off of the new, short doc in (2). So, it would start with pointing you to (2), then continue by building an entity, persisting, etc.

Questions

  • What is a good (but still somewhat simple - I don't want to introduce services with lots of big logic to do this) way to keep track of where different "types" of files are uploaded. For example, suppose a site allows you to upload avatars and product images - the first is stored in /uploads/avatars and the second in /uploads/products. Where each type is stored should be stored in configuration somewhere, then accessed in the controller and in the view. I want to show a nice, but still simple way you might handle this.
@mahmouds
Copy link
Contributor

@weaverryan I totally agree that we need an article for the upload without doctrine 👍
btw i have updated my PR #2120 and it should have fixed all of the callbacks triggering issues.

@javiereguiluz
Copy link
Member

Could someone please label this issue with the DX label? Thanks!

@weaverryan weaverryan added the DX label Jul 5, 2014
@weaverryan
Copy link
Member Author

Done! This is a huge issue.

saro0h added a commit to saro0h/symfony-docs that referenced this issue Jul 11, 2014
saro0h added a commit to saro0h/symfony-docs that referenced this issue Jul 11, 2014
saro0h added a commit to saro0h/symfony-docs that referenced this issue Jul 11, 2014
xabbuh pushed a commit to xabbuh/symfony-docs that referenced this issue Aug 24, 2014
@gondo
Copy link
Contributor

gondo commented Sep 27, 2014

what about handling file upload in POST_SUBMIT event of the root (the one without a parent) form?

i really don't understand why everyone (s2 cookbook, VichUploaderBundle, all the blog posts/tutorials) is putting the logic into entity class. in my understanding, it doesn't belong there at all. it belongs to form processing, not to data storing.
or am i missing something?

edit 1:
i've spent more time on this and man its complex.
using form post hooks doesn't work as expected because of form validation (constraints).
i thought that my requirement is simple:

  • i have entity with field image where i define my constraints Image and NotBlank
  • i build a form where field image is type of file
  • i want to handle the upload after form validation (to start with: in a controller) but i simply can't do this.

what symfony is forcing me to do:

  • separate upload and data mapping. by having different properties of Entity class (image_path with constraint NotBlank and file with constraint Image), or by using unmapped file form field and define Image constraint inside form builder.
  • handle upload inside Entity class (as mentioned above, i don't consider this correct), or handle this in controller what can be messy for a for with multiple files

why this has to be this complicated?

@weaverryan
Copy link
Member Author

Hey @gondo!

First, the cookbook is just wrong, and I think a lot of blogs have probably followed that :). About VichUploaderBundle, it only has the configuration in the entity (other than the extra property, you have a point with this), which is no different than having the Doctrine or Validation metadata there. So, you're not missing anything - you are correct imo!

You bring up some great points about how complex this can be, but that's more of an issue for the core than the docs.

Cheers!

weaverryan added a commit that referenced this issue Jun 19, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Added a new cookbook about file uploading

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | all
| Fixed tickets | #2346

This PR is completely based on the work made by @saro0h in #4018. All the credit for the original work goes to her. I just picked her work and applied the suggestions made by @weaverryan @xabbuh and @wouterj to make it mergeable.

If you agree with this PR, please merge it as soon as possible because this is a very important topic and we're a bit late on this doc. Thanks!

Commits
-------

4a7709b Fixed all the issues spotted by Ryan
20ae21b Added a new cookbook about file uploading
@javiereguiluz
Copy link
Member

I think we can close this issue now that #5375 is merged.

@weaverryan
Copy link
Member Author

@javiereguiluz what do you think about removing http://symfony.com/doc/current/cookbook/doctrine/file_uploads.html entirely now?

@javiereguiluz
Copy link
Member

I agree ... but only when I implement the changes that you suggested here #5375 (comment) I plan to do that soon.

@weaverryan
Copy link
Member Author

@javiereguiluz if that's on your list (which would be awesome), then I'll close this :). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable Clear and specific issues ready for anyone to take them. Doctrine DX HttpFoundation
Projects
None yet
Development

No branches or pull requests

4 participants