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

PROXY as global in http.lua #110

Closed
svarogg opened this issue Nov 3, 2014 · 12 comments
Closed

PROXY as global in http.lua #110

svarogg opened this issue Nov 3, 2014 · 12 comments

Comments

@svarogg
Copy link

svarogg commented Nov 3, 2014

Line 189 and 201 uses global variable PROXY.
It breaks my tests as I am using a strict globals checker to make sure no globals were declared.
This was fixed 5 Jul 2013 in 1f9ccb2 but the patch was not released as a rock yet.

Since latest release(v3.0-rc1) was quite some time ago - 14 Jun 2013, when can I expect a new release that fixes that issue?

@svarogg svarogg changed the title PROXY as global in http.lua breaks tests PROXY as global in http.lua Nov 3, 2014
@diegonehab
Copy link
Contributor

I don't have an answer for this question. There are a few outstanding problems that make me unwilling to release a final version. Unfortunately, I don't have the manpower to work on them at the moment. I'd love some help.

@svarogg
Copy link
Author

svarogg commented Nov 4, 2014

I will try to see what I can do, and if I have enough knowledge to try fix those things myself.
Please let me know what are the burning issue.

@daurnimator
Copy link
Contributor

@diegonehab do you have time to review/merge the pull requests @catwell mentioned?

@jjvbsag
Copy link

jjvbsag commented Nov 4, 2018

👍 For now I have to patch all occurrences of http.lua I find (e.g. in ZeroBrane Studio). I'd prefer a released version with the fix

@diegonehab
Copy link
Contributor

Which one are you taking about? The HTTP proxy one 110? Although it is open, isn't the code merged anyway? The uClibc one (#90) has conflicts. If it is adjusted and tested, I can merge. I need someone to take charge of #81 and test it properly, as it seems to be a delicate change. #101 is not a big deal, and if somebody tests it I can also merge.

@diegonehab
Copy link
Contributor

@jjvbsag ?

@jjvbsag
Copy link

jjvbsag commented Nov 6, 2018

Sorry, was busy on the job...
This is the patch, I'm talking about:

--- http.lua.orig	2018-11-03 10:54:52.997376028 +0100
+++ http.lua	2018-11-03 10:55:33.496561255 +0100
@@ -186,7 +186,7 @@
 local function adjusturi(reqt)
     local u = reqt
     -- if there is a proxy, we need the full url. otherwise, just a part.
-    if not reqt.proxy and not PROXY then
+    if not reqt.proxy and not _M.PROXY then
         u = {
            path = socket.try(reqt.path, "invalid path 'nil'"),
            params = reqt.params,
@@ -198,7 +198,7 @@
 end
 
 local function adjustproxy(reqt)
-    local proxy = reqt.proxy or PROXY
+    local proxy = reqt.proxy or _M.PROXY
     if proxy then
         proxy = url.parse(proxy)
         return proxy.host, proxy.port or 3128

See also pkulchenko/ZeroBraneStudio#950 for how to reproduce
(the second issue mentioned there is already fixed in your master)


About testing. For me it is all about a project on an ARM target. I am willing to try anything there, but as the project time line is extremely tight I can make no promises about when...

@diegonehab
Copy link
Contributor

This seems to be in master for a long, long time. Since 2013, actually. Maybe this is a problem of releasing a new tag so distributions can pull from?

@ewestbrook
Copy link
Contributor

Will the tagging of a new release solve this problem?

We're reviewing open items in preparation for a release. If action is needed here, please add a comment. Otherwise, this issue will be closed on or after 24-Feb-2019.

Thanks!

@jjvbsag
Copy link

jjvbsag commented Feb 17, 2019

Hi! Its in the master branch, so releasing a new tagged version seems fine for me...
Regards

@catwell
Copy link
Member

catwell commented Nov 8, 2023

This issue has been fixed for a long time and a release has been shipped since then, closing.

@catwell catwell closed this as completed Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants