@juergenthomann opened this Pull Request on April 25th 2022 Contributor

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

@github-actions[bot] commented on May 3rd 2022 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@juergenthomann commented on May 4th 2022 Contributor

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.

This Pull Request was closed on May 6th 2022
Powered by GitHub Issue Mirror