-
Notifications
You must be signed in to change notification settings - Fork 576
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
set userinfo using Mojo::URL in Mojo::UserAgent Basic auth example #1407
Conversation
@mojolicious/core Calling for a vote |
I don't know that I agree with removing the first style, I could see adding the other style too. Might also want to show url escaped example |
I considered that but it seems too busy for what is a tiny segment of an already pretty full synopsis. I think it's best to only show the best way that covers all situations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to give this a 👎, because i don't like the style.
lib/Mojo/UserAgent.pm
Outdated
@@ -385,7 +385,9 @@ Mojo::UserAgent - Non-blocking I/O HTTP and WebSocket user agent | |||
my $tx = $ua->put('[::1]:3000' => {'Content-Type' => 'text/plain'} => 'Hi!'); | |||
|
|||
# Quick JSON API request with Basic authentication | |||
my $value = $ua->get('https://sri:t3st@example.com/test.json')->result->json; | |||
use Mojo::URL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that this example contains a use
line. That doesn't feel right for a synopsis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer it just be omitted? (The example for the prepare event does this already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the use line
f0d4043
to
77bbd09
Compare
Restarting the vote @mojolicious/core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to recommend this safer variant. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving the change as it is clearly correct, I'm voting neutral however, as I'm not completely sold on removing the other form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from me. I think this is easier to read and less prune for errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I prefer this form.
Summary
Basic auth commonly uses username and password values from external sources, so they should be set using the Mojo::URL userinfo method rather than directly in the URL where they need to be escaped.
Motivation
More correct example for a common use case
References
IRC discussion