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

Guides fixes #13937

Merged
merged 1 commit into from Feb 16, 2014
Merged

Guides fixes #13937

merged 1 commit into from Feb 16, 2014

Conversation

ktaragorn
Copy link
Contributor

Minor improvements to Getting started and Assets Pipeline Guide

Once enabled, this can be used on any request which requires a file to be sent
back to the user. More details about this header and the motivation can be
found in the [Yii Framework wiki](http://www.yiiframework.com/wiki/129/x-sendfile-serve-large-static-files-efficiently-from-web-applications/).
This header can be used by calling [send_file](http://apidock.com/rails/ActionController/DataStreaming/send_file) on any file.
Copy link
Member

Choose a reason for hiding this comment

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

I like the previous text better. Also I don't think we should link to a wiki of a PHP framework. The only thing I'd add is that send_file makes use of that header:

When enabled the header is set automatically by `send_file`. See the [API docs](http://apidock.com/rails/ActionController/DataStreaming/send_file) for more info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I understand the weirdness of linking to a competing framework, I disagree that the previous text is better. Both my colleague and myself were unable to understand the meaning and purpose of it, which is why I went hunting, and the yii page explains it in great detail, hence me linking to them.

Copy link
Member

Choose a reason for hiding this comment

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

You can link to the relevant pages from Apache and Nginx:

Again the previous explanation is a good bird's view about the feature. More details are available at the links above. The new text is confusing. It uses defined terms like redirect in the wrong context: (e.g. allows a web application to redirect the request for a file to the web server). And again, I don't think we need to link to that framework page. This is an advanced feature, if you want to use it, you should be able to get into the apache or nginx docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we get a third opinion about the original text? If this guide is targetting newbs, then I can say with authority that it made no sense to me. My colleague, who is a lot more familiar with rails (and is a contributor as well) could not explain it to me either, which is what sent me hunting for better explanations.

As to your issue with the word "redirect", I copy here the first line of the nginx link that you gave me.
X-accel allows for internal redirection to a location determined.....

Personally I believe the page I linked is superior in terms of content. It explains in greater detail about the motivation itself, while the apache/nginx links are more about how to use it.

I can try to rewrite it again i guess, without the link to the Yii page, if that is a strict no

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take a look at my rewrite, it explains more in detail and does not use the text or the link from the YII page.

@senny senny added the docs label Feb 4, 2014
@@ -1103,9 +1103,14 @@ but will be very soon.

The `method: :patch` option tells Rails that we want this form to be submitted
via the `PATCH` HTTP method which is the HTTP method you're expected to use to
**update** resources according to the REST protocol.
**update** resources according to the REST protocol. If this option is not specified
the default method used is `POST`.
Copy link
Member

Choose a reason for hiding this comment

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

The default is already explained earlier in the guide: https://github.com/ktaragorn/rails/blame/guides_fixes/guides/source/getting_started.md#L554-L556

No need to repeat it over again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didnt really add or repeat that.. I moved it from the tip. I can remove both the tip and the new line if you want..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ive removed it as you requested

@vijaydev
Copy link
Member

vijaydev commented Feb 9, 2014

Big 👎 on the changes to the X-sendfile section, except may be adding the links to Apache and nginx pages. The existing text is clear and to the point.

@ktaragorn
Copy link
Contributor Author

Ive reverted the sendfile changes, except for the ones recommended by @senny

@@ -1018,7 +1018,7 @@ The X-Sendfile header is a directive to the web server to ignore the response
from the application, and instead serve a specified file from disk. This option
is off by default, but can be enabled if your server supports it. When enabled,
this passes responsibility for serving the file to the web server, which is
faster.
faster. This header can be used by calling [send_file](http://apidock.com/rails/ActionController/DataStreaming/send_file) on any file.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is still weird. We should reword it to:

Have a look at send_file on how to use this feature.

@vijaydev
Copy link
Member

@ktaragorn Please ensure that finally there is only one commit to be merged.

…h links

Explain how form_for :article is able to pull in the properties of @Article
Make it clear that article_id is generated due to the association set up

Add link to the rails function that uses X-Sendfile.
Add links to apache and nginx docs for the header
@ktaragorn
Copy link
Contributor Author

Hi @vijaydev, URL source has been changed and the commits squashed

vijaydev added a commit that referenced this pull request Feb 16, 2014
@vijaydev vijaydev merged commit 3e3ed1e into rails:master Feb 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants