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.

Sign up for GitHub

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

Jump to bottom

Document how web paths are built #829

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Document how web paths are built #829

merged 1 commit into from
Dec 5, 2016

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Dec 3, 2016

There are common pitfalls that can be easily avoided with some
configuration.
Closes #828

@greg0ire greg0ire mentioned this pull request Dec 3, 2016
Closed
@greg0ire greg0ire force-pushed the document_web_path_resolver_url_construction branch from b8ba579 to 4b35b5e Compare December 3, 2016 15:10
alexwilson
alexwilson approved these changes Dec 3, 2016
View reviewed changes
Copy link
Collaborator

@alexwilson alexwilson left a comment

Choose a reason for hiding this comment

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

I'm pretty happy with this, it's simplistic explains the required configuration without going into too much depth.

@robfrawley robfrawley added Type: Documentation This item pertains to documentation of this project. State: Reviewing This item is being reviewed to determine if it should be accepted. labels Dec 3, 2016
@robfrawley
Copy link
Collaborator

@greg0ire @antoligy I reformatted the majority of new information into a tip box, and moved the links to the bottoms of the page (our global convention), as well as cleaned up the language a bit. Also, I went ahead and did a local test build of the RST docs to ensure proper formatting. Here's how the new docs look:

liipimaginebundle-rst-docs-pr829

Thoughts on this PR at this point? Looks good to me; will merge shortly pending any comments otherwise.

@greg0ire
Copy link
Contributor Author

greg0ire commented Dec 3, 2016

Good job! Shall I squash the commits or will you?

greg0ire
greg0ire commented Dec 3, 2016
View reviewed changes
Resources/doc/cache-resolver/web_path.rst

The request context is most notably used to determine the HTTP scheme
prepended to the final URL. If you use a proxy to offload TLS traffic
decryption and need the resolver to generate HTTPS URLS, you will need
Copy link
Contributor Author

Choose a reason for hiding this comment

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

URLs, maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh; already noticed that myself in my follow-up commit. though do please take a look at my final changes and advise if you notice anything else needing cleanup.

@robfrawley
Copy link
Collaborator

@greg0ire Just pushed an additional commit with some cleanup of the first few paragraphs of the web path resolver docs, as well as some changes to the my initial language with regard to the original intent of this PR. Feel free to squash the commits yourself, but there is no longer a need to do so yourself (GitHub PRs now allow for squashing and merging in a single click).

@robfrawley
Copy link
Collaborator

Also, here is the final rendering with all the prior changes:

liipimaginebundle-rst-docs-pr829-2

@greg0ire
Copy link
Contributor Author

greg0ire commented Dec 3, 2016

but there is no longer a need to do so yourself (GitHub PRs now allow for squashing and merging in a single click).

I'm aware of that, it's just to make sure we don't forget it by mistake ;)

@greg0ire greg0ire force-pushed the document_web_path_resolver_url_construction branch from 5728609 to d8f69ef Compare December 3, 2016 20:12
@greg0ire greg0ire changed the title Document how web paths are build Document how web paths are built Dec 3, 2016
robfrawley
robfrawley approved these changes Dec 3, 2016
View reviewed changes
Copy link
Collaborator

@robfrawley robfrawley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. Thanks @greg0ire. @antoligy / @lsmith77: Good to go?

greg0ire
greg0ire commented Dec 3, 2016
View reviewed changes
Resources/doc/cache-resolver/web_path.rst
traffic decryption and need the resolver to generate secure URLs,
you will need to appropriately configure Symfony's `trusted proxies`_.
If you utilize `embedded controllers`_ in your templates, you must
add ``localhost`` to your trusted proxy configuration.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not so sure about that one… trusted proxies configuration maybe? or "add localhost as a trusted proxy"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer "trusted proxies" to rewording the sentence. Pushed a commit reflective of such. Anything else we need to update/change?

@greg0ire
Document how web paths are built
dcc364e 
There are common pitfalls that can be easily avoided with some
configuration.
Closes liip#828
@greg0ire greg0ire force-pushed the document_web_path_resolver_url_construction branch from 95c7fff to dcc364e Compare December 3, 2016 20:22
@greg0ire
Copy link
Contributor Author

greg0ire commented Dec 3, 2016

LGTM (PR authors can't approve their own PRs)

@robfrawley robfrawley changed the base branch from 1.0 to 2.0 December 3, 2016 20:25
@robfrawley robfrawley changed the base branch from 2.0 to 1.0 December 3, 2016 20:25
@robfrawley
Copy link
Collaborator

robfrawley commented Dec 3, 2016 •
edited
Loading

@greg0ire Ignore the base-branch changes; was going to merge immediately into 2.0, which would not cause the live documentation build process to kick off, and would afford some time for comments prior to merging back to 1.0 (which will immediately cause the build process to kick off).

I'm cautious with documentation changes, so I told my phone to remind me to come back to this PR tomorrow night (I do love Google voice control on Android: "OK Google, remind me [...] tomorrow night"). Assuming no objections or change requests arise, this'll be merged then. Thanks @greg0ire for the PR!

@greg0ire
Copy link
Contributor Author

greg0ire commented Dec 3, 2016 •
edited
Loading

I'm cautious with documentation changes, so I told my phone to remind me to come back to this PR tomorrow night (I do love Google voice control on Android: "OK Google, remind me [...] tomorrow night").

Jeez, I hope they pay you well 😆 Anyway, thanks for everything, looking forward to the merge!

@alexwilson
Copy link
Collaborator

No objections from me, cheers for the contribution and nice work with reformatting!

Off-topic but do we have the documentation standards noted down anywhere so I can try and help catch this stuff in future?

"Ok Google, remind me to pay @robfrawley in 🍺"!

@robfrawley robfrawley merged commit e345af3 into liip:1.0 Dec 5, 2016
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Dec 5, 2016
@robfrawley
Copy link
Collaborator

Thanks @greg0ire!

@greg0ire greg0ire deleted the document_web_path_resolver_url_construction branch December 5, 2016 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robfrawley robfrawley robfrawley approved these changes

@alexwilson alexwilson alexwilson approved these changes

Assignees
No one assigned
Labels
Type: Documentation This item pertains to documentation of this project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@greg0ire @robfrawley @alexwilson @lsmith77