Add autodetected http/https protocols #592
Conversation
|
Good start. You'll want to use Probably easiest to make |
|
Thank you for the guidance, @tpope. I didn't know how well-versed you were with the win32 side of things in The non-win32 version I think should work since I haven't seen a system without access to Also, if you'd rather the solution stick to one possible approach, I can work instead on replacing the Thanks again for all of your help and for such a great plugin. |
dblanken
force-pushed
the
autodetect_uri_scheme_on_preview
branch
2 times, most recently
from
26c29ae to
bc54347
Compare
|
Let's start by doing just the interface change, with hard-coded
I'm leaning towards using the I don't actually consider Windows support mandatory for merging, but indeed an added bonus of the |
|
Actually hold off; the interface change is tricky and touches some bugs that need to be fixed. I'll take care of it and you can pick up from there. |
Most notably, now include the http:// as part of the binding. References: #592
|
Okay have at it. |
When running without changes, it looks as if the ) character was not escaped. Escaping this allows a successful http binding.
get_binding_for now references a uri_scheme variable to prepend the http:// to the binding.
This is done in preparation of the uri_scheme detection. Since a return is found here, the detection of SSL must occur before this, and only on a valid binding.
Use curl to detect if the binding used is over https or http. * Created a local method to handle detection given a non-URI prepended binding * Set uri_scheme variable before returns where binding is in scope
dblanken
force-pushed
the
autodetect_uri_scheme_on_preview
branch
from
bc54347 to
1e2f345
Compare
|
Thank you for doing all of that! I tried to break up the commits a little more this time. Also I wondered if you could verify my regex fix to make sure you're having the issue as well before the fix. I was only able to test that on my Mac, so that's what that was based on. I can revert if needed. I was also iffy on if you were ok with a local method or not for this--I figured since it had to be done twice in that method, it helped not make it so long. Finally, I added the -k to the curl to allow insecure certs. For my case, it caused my Preview to hang for quite a bit without it, but I could see that allowing some security concerns. Again, thanks for looking at all of this and for your feedback/help. |
Yeah I'm not sure why I didn't catch that when testing. I've gone ahead and ported it to master.
I don't see why it needs to be done twice. I deliberately restructured it to have a singular |
|
I'll remove the regex hopefully tonight from the PR. Thanks for verifying and porting that one. For the return, I think it was line 1991 that I then broke up in commit 1549556 in order to add what I consider the second check. I did notice you gave a great avenue to one return everywhere else, but thought that possibly remained for a different reason. I can revert that change there and just have it at the end before the return if you feel it isn't needed in that return. |
|
You can delete that |
Removed return line in netstat conditional as it should not have remained, allowing for one return flow in the get_binding_for method. References: [Discussion on netstat conditional](tpope#592 (comment))
Move contents of s:UriSchemeFor into get_binding_for now that we have one return in that method.
Added a backup check for wget in the event curl did not exist. Both can return a 200 OK, so we can check for that in both cases.
|
Sorry for such a delay in getting back. I removed the line, which allowed us to put the method details back into |
This was residual from the netstat change before and should have been removed with it.
|
Merged with some tweaks, thanks. |
|
Tested your merge on my Mac. I think the
Thanks for all of your guidance and help on adding this. I really appreciate the opportunity to contribute. |
This is in response to @tpope's response on PR #590 to allow https to be used for localhost connections using Preview. There, he mentioned that he'd like the uri scheme protocol to be autodetected, which I appreciate. I opted to use 'possible approach 3' to ps/grep for ssl:// to see if it was being used or not.