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

Clarify documentation on serving files #6613

Closed
wants to merge 1 commit into from

Conversation

raphaelm
Copy link
Contributor

For a beginner, the "Serving Files" documentation was a little bit ambiguous on whether the makeDisposition method only adds the header or deals with sending the content as well. This change tries to make that explicit.

@wouterj
Copy link
Member

wouterj commented Jul 2, 2016

👍

status: reviewed

@@ -483,6 +483,9 @@ abstracts the hard work behind a simple API::

$response->headers->set('Content-Disposition', $d);

You can add the file's content as usual, for example by using
Copy link
Member

Choose a reason for hiding this comment

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

Instead of You can add the file's content as usual... maybe we should say Then you must include the file's content as usual...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being a non-native speaker, I'm really not sure which one is better here. :)

Copy link
Member

Choose a reason for hiding this comment

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

@raphaelm setting aside the grammar/wording issues ... the problem I see with the original text is that it says: "well, if you want, include the file contents". But it should say something like: "don't forget that it's mandatory to include the content files and you must do that manually; otherwise, the file is not really sent".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I adjusted that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually wondering why we don't set the content as the constructor argument, as done most of the time when setting response content?

@javiereguiluz
Copy link
Member

👍

For a beginner, the "Serving Files" documentation was a little bit ambiguous on whether the makeDisposition method only adds the header or deals with sending the content as well. This change tries to make that explicit.
wouterj added a commit that referenced this pull request Jul 8, 2016
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #6613).

Discussion
----------

Clarify documentation on serving files

For a beginner, the "Serving Files" documentation was a little bit ambiguous on whether the makeDisposition method only adds the header or deals with sending the content as well. This change tries to make that explicit.

Commits
-------

7746e07 Clarify documentation on serving files
@wouterj
Copy link
Member

wouterj commented Jul 8, 2016

Thanks @raphaelm! I've merged this into the 2.7 version of the docs with 16bf6db.

Afterwards, I decided to remove the sentence you've added and instead add the missing code lines in the example. I think it's easier to read and understand. You can see these changes in 4fe59bb. Feel free to comment if you don't agree.

Thanks again for pointing us at this confusing spot in the docs. I hope to see more of these clearifying improvements! :)

@wouterj wouterj closed this Jul 8, 2016
@raphaelm raphaelm deleted the patch-1 branch July 8, 2016 09:08
@raphaelm
Copy link
Contributor Author

raphaelm commented Jul 8, 2016

Hi @wouterj, I think that your version is even better. Thank you as well! :)

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.

4 participants