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

set userinfo using Mojo::URL in Mojo::UserAgent Basic auth example #1407

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

Grinnz
Copy link
Contributor

@Grinnz Grinnz commented Sep 16, 2019

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

@Grinnz
Copy link
Contributor Author

Grinnz commented Sep 16, 2019

@mojolicious/core Calling for a vote

@jberger
Copy link
Member

jberger commented Sep 16, 2019

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

@Grinnz
Copy link
Contributor Author

Grinnz commented Sep 16, 2019

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.

Copy link
Member

@kraih kraih left a 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.

@@ -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;
Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the use line

@Grinnz
Copy link
Contributor Author

Grinnz commented Sep 17, 2019

Restarting the vote @mojolicious/core

Copy link
Member

@kraih kraih left a 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. 👍

Copy link
Member

@jberger jberger 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 approving the change as it is clearly correct, I'm voting neutral however, as I'm not completely sold on removing the other form.

Copy link
Member

@jhthorsen jhthorsen left a 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.

Copy link
Member

@marcusramberg marcusramberg left a 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.

@Grinnz Grinnz merged commit 5a686dd into master Sep 24, 2019
@Grinnz Grinnz deleted the useragent_basic_example branch September 24, 2019 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants