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

Added cookbook to show how to make a simple upload #4018

Closed
wants to merge 2 commits into from

Conversation

saro0h
Copy link
Contributor

@saro0h saro0h commented Jul 11, 2014

Q A
Doc fix? No
New docs? yes
Applies to all
Fixed tickets #2346

This documentation refers to the ticket #2346, the second point saying:

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.

Not sure if it was wise to put it in the cookbook/controller section. Feel free to tell me if I should put it elsewhere.

@javiereguiluz
Copy link
Member

@saro0h thanks for writing this article!! This is something that Symfony Cookbook definitely needed.

My only concern about the article is that it includes some topics that may be considered "not strictly necessary" when dealing with file uploads. To be honest, I don't know if we must remove them because they overcomplicate the article ... or we should keep them because they complement the article. In any case, those topics are the following:

  • Defining the form as service
  • Creating the form theme
  • Displaying the flash message

@mtrojanowski
Copy link
Contributor

I think it's nice to have some information on this topics (that you can, for example, change the form theme if you'd like to) but it's probably better to add them as references to the appropriate documentation entries. Thus we won't have to change anything in this article if the way you alter the form theme should ever change.

@saro0h
Copy link
Contributor Author

saro0h commented Jul 11, 2014

@mtrojanowski Can you tell me which one I need to change, so I can make proper corrections :)

@mtrojanowski
Copy link
Contributor

It's what @javiereguiluz suggested:

  • Defining the form as service
  • Creating the form theme
  • Displaying the flash message

@hhamon
Copy link
Contributor

hhamon commented Jul 12, 2014

Good one! 👍

}
}

Create the corresponding template as following::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to remove one of the colons so that the following lines are not treated as a PHP code block.

</ul>
{% endfor %}
{% endfor %}
{# … #}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also include a PHP example

@wouterj
Copy link
Member

wouterj commented Jul 13, 2014

Thank you @saro0h !

I've added a bunch of commits about same minor things you have to change if you agree.

I'm -1 for adding the form theming stuff to this article. I think it's too comlex and it doesn't add anything valuable.

At last, there is a Travis build failure which you should fix and a link to this document needs to be added to cookbook/controller/index.rst and cookbook/map.rst.inc.

{% endblock form_row %}

Some sugar has been added by adapting our form with a form theme (take a look at
the :doc:`form themes </cookbook/form/form_customization#what-are-form-themes>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work (you cannot use the doc role to link to a certain section). Instead use something like this:

:ref:`form themes <cookbook-form-customization-form-themes>`

@weaverryan
Copy link
Member

Great job @saro0h! I just think we need much less now - simplify everything and focus on the mechanics of only setting up the form and handling the file upload.

I'd also be interested in showing them how to create a link to the file once it's uploaded.

@wouterj
Copy link
Member

wouterj commented Aug 15, 2014

ping @saro0h, can you please take a look at the comments made by @weaverryan ?

@wouterj
Copy link
Member

wouterj commented Sep 16, 2014

ping @saro0h If you don't have time, please say so, then somebody else can take it over.

@wouterj
Copy link
Member

wouterj commented May 15, 2015

Hi @saro0h,

On May 23rd, there will be a doc sprint day. Do you maybe have time to update your PR before/during that day? If you don't, we can add it to a list so other people can work on it during that day (you can of course also join us on that day to have some fun & finish this PR 😄)

Thanks for starting this one!

@javiereguiluz
Copy link
Member

I took the original work made by @saro0h and finished it in a new PR #5375 which supersedes this one.

@xabbuh
Copy link
Member

xabbuh commented Jun 10, 2015

closing in favor of #5375

@xabbuh xabbuh closed this Jun 10, 2015
weaverryan added a commit that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants