Remove old File Upload article + improve the new one #6040
Conversation
|
Btw, why do we use an Entity property to store both the uploaded file and file path? Wouldn't it be better to use an unmapped field? |
|
|
||
| If you are using Doctrine to store the Product entity, you can create a | ||
| :doc:`Doctrine listener </cookbook/doctrine/event_listeners_subscribers>` to | ||
| automatically upload the file when persisting the entity:: |
There was a problem hiding this comment.
I wondering if we should show code + config, or just link to the other article and just mentioning this as a little tip.
There was a problem hiding this comment.
I like how you've handled it here: with code+config (it's really not too long).
|
@wouterj, if I might make an addition as well.... It might be good to include how to handle files when editing them. For example, if I have a simple form like you included in the example, then I would need to either set data_class to null on the brochure field or convert the string to a File object. Otherwise, the form will error out and say - "The form's view data is expected to be an instance of class Symfony\Component\HttpFoundation\File\File, but is a(n) string". Maybe creating the file object is better in a data transformer, not sure. Thoughts? |
I think this was done to always trigger the lifecycle callback methods. If only the file path were mapped to the database, lifecycle callbacks would never be triggered if only the uploaded file was changed (Doctrine would not treat the entity as changed at all). However, I tend to say that we should rather move the whole file uploding logic into a DTO and not pollute the entity with logic that doesn't belong there. |
| # app/config/parameters.yml | ||
| parameters: | ||
| # ... | ||
| brochures_directory: '%kernel.root_dir%/../web/uploads/brochures' |
There was a problem hiding this comment.
This makes more sense in config.yml - it's not a value that would normally need to be changed on a machine-by-machine basis, and devs wouldn't/shouldn't need to worry about configuring it.
There was a problem hiding this comment.
And actually, we're dependent on it later when we make the paths to it in the Twig template.
|
@wouterj I added 2 small comments, but 👍 from me after that. Sorry for the long delay - I'm also excited to remove the old, terrible article. |
|
@weaverryan can we create redirect from old url to the new article? |
|
@Koc That's done with the entry in the |
|
I've updated the article. Should be ready to merge now |
| } | ||
| } | ||
|
|
||
| This class can be registered as a service in the service container: |
There was a problem hiding this comment.
This class can be registered as a service in the service container:
->
Then, define a service for this class: ?
|
While testing this code in an application I found a few bugs which I fixed in f6068c9. Also, I strumbled onto issues creating an edit form as it needs a It's now even more ready to merge than it was this afternoon :) |
|
@wouterj amazing! Thanks for testing everything thoroughly in a real project 🙇 |
|
|
||
| You can now use the following code to link to the PDF brochure of an product: | ||
| You can use the following code to link to the PDF brochure of an product: |
|
👍 I like the changes you made here. However, for the future I think we can improve things even more by introducing a DTO that would be used to build the form and doing the file upload. That way we would not need to have the hacks being present currently. |
|
I left a minor comment, but let's merge this :) 👍 |
|
Updated again. @xabbuh using a DTO is exactly what I thought when improving this article. @weaverryan I can't find your minor comment. |
|
@wouterj you say "updated again" but I can't see any new commits 😕 |
|
@javiereguiluz as it was a very small typo fix, I merged it into the previous commit: 94e21ff#diff-bcae3aaf74848d919ee531cad0d95547R192 |
|
Thank you Wouter. |
…outerJ) This PR was squashed before being merged into the 2.3 branch (closes #6040). Discussion ---------- Remove old File Upload article + improve the new one | Q | A | --- | --- | Doc fix? | yes | New docs? | yes | Applies to | 2.3+ | Fixed tickets | #5375 The old file upload article wasn't good, a new one was written by @javiereguiluz but the old one remained online. This PR removes the old one and documents the missing bits in the new article. Commits ------- 888c61c Remove old File Upload article + improve the new one
The old file upload article wasn't good, a new one was written by @javiereguiluz but the old one remained online. This PR removes the old one and documents the missing bits in the new article.