Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebServerBundle] Added ability to display the current hostname address if available when binding to 0.0.0.0#28586
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| /** | ||
| * @return string contains resolved hostname if available, empty string otherwise | ||
| */ | ||
| publicfunctiongetExtraAddress() |
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.
What about renaming this method asgetRealAddress() orgetResolvedAddress() ?
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.
@javiereguiluz will do, it reads better.
| $io->success(sprintf('Server listening on http://%s',$config->getAddress())); | ||
| $message =sprintf('Server listening on http://%s',$config->getAddress()); | ||
| if ('' !==$extra =$config->getExtraAddress()) { | ||
| $message =sprintf('%s (http://%s)',$message,$extra); |
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.
Not sure if we can hardcodehttp:// here. What if we are usinghttps:// in the local environment? Maybe it's better to detect that and add it in thegetExtraAddress() method.
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.
@javiereguiluz we assume this as the php server won't support SSL, besides we already assume this in the original message.
| $io->success(sprintf('Server listening on http://%s',$config->getAddress())); | ||
| $message =sprintf('Server listening on http://%s',$config->getAddress()); | ||
| if ('' !==$extra =$config->getExtraAddress()) { | ||
| $message =sprintf('%s (http://%s)',$message,$extra); |
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.
To make it a bit easier to understand for users, we could change the output from this:
[OK] Server listening on http://0.0.0.0:8080 (http://172.20.0.2:8080)to something like this:
[OK] Server listening on http://0.0.0.0:8080 (resolved as http://172.20.0.2:8080)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.
@javiereguiluz will do the change asresolved, this should also match the method name change (more consistent)
88b2ad2 to6417342Compare| /** | ||
| * @return string contains resolved hostname if available, empty string otherwise | ||
| */ | ||
| publicfunctiongetResolvedAddress() |
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'd suggest calling it getDisplayAddress actually (nothing is "resolved" to me)
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.
Do you recommend to reword the message displayed to users too? Currently it says... (resolved as ...) I think it's easy to understand that way ... but may there's a better wording. Thanks!
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.
We already have a suggestion in the issue
[OK] Server listening on all interfaces, port 8080 -- see http://127.0.0.1:8080…ss if available when binding to 0.0.0.0
6417342 todfd2e8bComparerespinoza commentedOct 9, 2018
Updated with the new messages, thanks to@jkufner suggestion, I believe the message now looks cleaner and conveys what we want. |
fabpot commentedOct 10, 2018
Thank you@respinoza. |
… hostname address if available when binding to 0.0.0.0 (respinoza)This PR was merged into the 4.2-dev branch.Discussion----------[WebServerBundle] Added ability to display the current hostname address if available when binding to 0.0.0.0| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | ? (no tests for server commands)| Fixed tickets |#28585| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Tries to get the local hostname into an ip (if available) using the PHP gethostname* methods.Commits-------dfd2e8b [WebServerBundle] Added ability to display the current hostname address if available when binding to 0.0.0.0
Uh oh!
There was an error while loading.Please reload this page.
Tries to get the local hostname into an ip (if available) using the PHP gethostname* methods.