Add support to exclude hosts from proxy #19139
Conversation
|
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
github-actions
bot
added
the
Stale
There was a problem hiding this comment.
Left a tiny comment, otherwise this looks good in general.
Regarding support for other transport methods. I see that it would be a bit more complicated to rebuilt the behavior, the no_proxy option of curl has. But we could define that for other requests methods we only do an exact host compare for now. So it would be a simple in_array check (after applying explode on the config)
|
Thank you for the review. I checked how I could integrate the changes in the other transports and that would result in having the explode + in_array logic for both transports and different logic for curl. I now changed the behavior to check this once in the getProxyConfiguration method and have it the same way for all transports. In my use case this would be enough and there was already logic to exclude localhost. I also tried to write a small unit test for it. I never had to write them before so it is mostly inspired by what I found in other tests. If something is not how it should be, let me know and I will change it. |
juergenthomann
changed the title
github-actions
bot
removed
the
Stale
There was a problem hiding this comment.
This looks good now. I've already updated the FAQ, so the new setting is mentioned there.
This will be included in Matomo 4.11.0.
Thanks for your contribution @juergenthomann
sgiehl
added
the
Enhancement
Description:
This add supports for curls noproxy option.
The support is not added for the other transports as that would involve manually splitting (if multiple should be supported) and matching of hosts. Curl also supports whole domains instead of just hosts so the logic would be a bit more complicated.
For the naming: normally I would have used the name no_proxy, but that causes names in the code like proxyNoproxy which I personally did not like, but I'll change it, if that is preferred (or even something else)
I'm not sure where I can update https://matomo.org/faq/troubleshooting/faq_121/ but that page could be updated to include the new option
Review