-
Notifications
You must be signed in to change notification settings - Fork 622
Add support for overriding stats logger's base URL #115
Conversation
d971ddf
to
2d491dc
Compare
2d491dc
to
70ffe41
Compare
Tried it on my own environment, fixes the issue for me. I set my config to: 'base_url_override' => 'http://caddy', |
IMHO I would expect the override to be the exact url called, not being appended with the route path still. So I think you should either call this a |
It is called base URL override though. 😉 The way I see it, it directly relates to the value of APP_URL / |
I'm not even sure what I was reading then, sorry about that 😂 |
@mpociot just wondering if you could take a look at this? |
@mpociot Can you take a look at this? It is very relevant in Docker setups. |
I run on a docker swarm, and I was able to get things working by setting APP_URL to my statistics server frontproxy ip, and making sure dns resolving is on in the config. After much debugging i realised that a dns resolving options appeared in a new config file that defaults to off, meaning you always connect to localhost. |
@mpociot Can you please merge this? the statistics are not been saved to the database when the project is running inside a container. |
$url_override = config('websockets.statistics.base_url_override', null); | ||
|
||
if ($url_override !== null) { | ||
return $url_override.action($action, [], false); |
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 is this supposed to do? 🤔 If $url_override
is a static string, wouldn't this break?
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 not sure I understand your question. Basically it just overrides the base URL, for example I would set "http://caddy" as the base because that's the name of my web server's docker container. Localhost doesn't work for me because my websocket server is not in the same container and the APP_URL doesn't work for me because that's the external user facing URL. I need to specify an internal URL for the websocket server to reach my laravel app over http.
See #114 where I explained my need for this
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 realizing maybe you're possibly misreading the .
as a JS method call? That's a PHP concatenation 😛
I'd typically have spaces around the operator. Maybe I changed it because StyleCI complained, I don't remember.
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.
The signature for action is function action($name, $parameters = [], $absolute = true)
, passing false for the 3rd param goes down to here https://github.com/laravel/framework/blob/6.x/src/Illuminate/Routing/RouteUrlGenerator.php#L77 which makes the base URL not get included in the output, so that it returns just the path without the base. Then we prefix that with the base URL override configured.
@mpociot Can you please merge this? the only thing that is keeping us from using in production. |
Any update on this? |
Are you asking me to fix that? Or are you leaving that as a note for yourself? |
@francislavoie Sorry 😆 I cannot create the fixing PR from StyleCI so I'd like them to be fixed manually for the moment. |
I'm not at a computer right now, but you should be able to commit the change manually to my branch, no? |
@francislavoie Sure thing. I asked for a rebase because there are are some PRs I should take care of and I need to be pretty straightforward, but I'll commit it instead ^^ |
Sorry for reverting this PR, but I realized too late that it's a better choice to make the HttpLogger instance configurable via the config file and let anyone implement their own way to override the behavior. 😯 |
Fixes #114
I didn't add any tests, I'm unclear on how would be best to test this.
Needs documentation added, probably around here: https://github.com/beyondcode/docs.beyondco.de/blob/master/resources/docs/laravel-websockets/1.0/debugging/dashboard.md