Hannes Magnusson [2006-06-25 12:55 UTC] No die()/exit() please.
Thorsten Rinne [2006-06-25 14:20 UTC] The exit() calls are needed after a handshake step finished to finish the request and get the next authentification step with the next request.
Philippe Jausions [2006-06-26 17:42 UTC] Name doesn't seem right... HTTP_Proxy_Auth_NTLM ?
There's another NTLM proposal in PEPr, in draft status, you may want to check with that maintainer.
Anyway to integrate with HTTP_Request, or HTTP_Client, Net_Socket and so on...? (I know there might be a problem with the Keep-Alive...)
I think there's a need to split package into HTTP_Proxy with pluggable authentication modules instead (NTLM being one of them.)
Try to avoid all the sprintf(). Use ' (single quotes) and concatenation instead.
Use URLs instead of ip, server, path.
Does this work good without PHP being mod_php in Apache?
Thorsten Rinne [2006-06-26 20:17 UTC] Hi Philippe,
> Name doesn't seem right... HTTP_Proxy_Auth_NTLM ?
ouch... I like the short name NTLMProxy. :-)
> There's another NTLM proposal in PEPr, in draft status, you may want to
> check with that maintainer.
yes, but I think this is about authentication, the proposed class does not authenticate, it's just a proxy.
> Anyway to integrate with HTTP_Request, or HTTP_Client, Net_Socket and so
> on...? (I know there might be a problem with the Keep-Alive...)
I do not think this will work due to Keep-Alive...
> I think there's a need to split package into HTTP_Proxy with pluggable
> authentication modules instead (NTLM being one of them.)
This is a nice idea... :-)
> Try to avoid all the sprintf(). Use ' (single quotes) and concatenation
> instead.
Is there a reason for not using sprintf?
> Use URLs instead of ip, server, path.
I use IP, Server and path to have the possibility to set various stages like development, testing and production environments.
> Does this work good without PHP being mod_php in Apache?
no, sorry, because of Keep-Alive.
-Thorsten
Justin Patrin [2006-06-27 04:11 UTC] sprintf is far slower than any other string creation method. Single quotes are the preferred method. See:
http://pear.reversefold.com/strings/
It is especially inappropriate to use sprintf when you're not even doing anything to the string (such as sprintf("\r\n\r\n")...).
This package should use HTTP_Request and/or HTTP_Client. If there is a problem with keep-alive I doubt it would be too hard to alter HTTP_Client and Request to support it. They could easily just keep the socket open and do more sending/receiving.
I don't really like the names of the "state" functions. I would rather they were named for what they do such as connect, sendRequest, readRequest, parseRequest, etc.
die()/exit should not be called in a package. Why can't the script simply exit normally?
_createNewSocket and _createSocket duplicate code.
Don't use @ to silence notices and warnings. Write your code to avoid them instead.
Errors should raise/return a PEAR_Error.
The log is a decent I idea, but I suggest you use the Log package instead as it would allow the developer to log however they want to.
Thorsten Rinne [2006-06-27 17:14 UTC] Hi,
> sprintf is far slower than any other string creation method. Single quotes
> are the preferred method. See:
> http://pear.reversefold.com/strings/
> It is especially inappropriate to use sprintf when you're not even doing
> anything to the string (such as sprintf("\r\n\r\n")...).
if speed is the only problem then I'll be happy. ;-)
I remove the unnecessary sprintf().
> This package should use HTTP_Request and/or HTTP_Client. If there is a
> problem with keep-alive I doubt it would be too hard to alter HTTP_Client
> and Request to support it. They could easily just keep the socket open and
> do more sending/receiving.
If it's easily, please fix http://pear.php.net/bugs/bug.php?id=4806 first.
> I don't really like the names of the "state" functions. I would rather
> they were named for what they do such as connect, sendRequest,
> readRequest, parseRequest, etc.
Okay, I'll do this.
> die()/exit should not be called in a package. Why can't the script simply
> exit normally?
The need of die() is that there's a rea big problem when this situation happened... The exit() calls are needed during the NTLM handshake.
> _createNewSocket and _createSocket duplicate code.
They don't duplicate code, _createNewSocket() closes an open socket first.
> Don't use @ to silence notices and warnings. Write your code to avoid them
> instead.
I'll add that.
> Errors should raise/return a PEAR_Error.
okay, I could use it.
> The log is a decent I idea, but I suggest you use the Log package instead
> as it would allow the developer to log however they want to.
I do not want a bloated class.
-Thorsten
Justin Patrin [2006-06-27 18:54 UTC] > > sprintf is far slower than any other string creation method. Single
> quotes
> > are the preferred method. See:
> > http://pear.reversefold.com/strings/
> > It is especially inappropriate to use sprintf when you're not even
> doing
> > anything to the string (such as sprintf("\r\n\r\n")...).
>
> if speed is the only problem then I'll be happy. ;-)
Your call... Using simple concatenation is also IMHO easier to read, but this isn't something that must be done to get accepted.
> I remove the unnecessary sprintf().
>
> > This package should use HTTP_Request and/or HTTP_Client. If there is a
> > problem with keep-alive I doubt it would be too hard to alter
> HTTP_Client
> > and Request to support it. They could easily just keep the socket open
> and
> > do more sending/receiving.
>
> If it's easily, please fix http://pear.php.net/bugs/bug.php?id=4806
> first.
I'll try to take a look, but am very busy. You already have some code to deal with this, would it be all that hard to simply patch HTTP_Request/Client to support this?
Even if you don't do this, why not use Net_Socket instead of using low-level socket manipulation functions?
> > die()/exit should not be called in a package. Why can't the script
> simply
> > exit normally?
>
> The need of die() is that there's a rea big problem when this situation
> happened...
You need to explain this in more detail. The script should be able to exit on its own, shoudl it not? If you write your code to return instead of die then the calling script can end and nothing untoward shoudl happen.
> The exit() calls are needed during the NTLM handshake.
You keep saying this but never explain why. Why should this be needed? Why won't letting the script end normally work?
>
> > _createNewSocket and _createSocket duplicate code.
>
> They don't duplicate code, _createNewSocket() closes an open socket
> first.
Yes they do. All of the code is the same except the close call. Have _createNewSocket call _createSocket instead of duplicating the code.
> > The log is a decent I idea, but I suggest you use the Log package
> instead
> > as it would allow the developer to log however they want to.
>
> I do not want a bloated class.
Sorry, that's not a good reason, especially since supporting Log would add only a few lines of code. Your current log functionality only appends to a string. Allowing someone to use Log would be far more flexible. You don't even have to add any extra explicit support for this. Simple allow the developer to pass in a Log instance if they with to log and, instead of appending to an internal string check for a log object, then call $this->log->log(). Only a few extra lines of code for a far more flexible logging mechanism.
Thorsten Rinne [2006-06-27 19:31 UTC] Hi,
I will rewrite some parts and add PEAR::Log. Ist that okay? :-)
-Thorsten
Justin Patrin [2006-06-28 18:04 UTC] Please try the patch linked in this bug and see if HTTP_Request works with keep-alive for you now.
http://pear.php.net/bugs/bug.php?id=4806
Thorsten Rinne [2006-06-29 05:24 UTC] Thanks for the fix. I'll test it but I still think it's better to have not too much dependencies. I'll work on the code this weekend to improve it's quality.
-Thorsten
Justin Patrin [2006-06-29 05:52 UTC] > Thanks for the fix. I'll test it but I still think it's better to have not too much dependencies. I'll work on the code this weekend to improve it's quality.
That's not the way that PEAR works. We try to keep code duplication to a minimum in order to reduce possible bugs.
Pierre-Alain Joye [2006-06-29 09:43 UTC] As it makes sense to do not reinvent the wheel in each package, it makes no sense to use http_request in this package. It adds unnecessary complexity and points of failure. It is nice to have once a small and efficient package.
I'm not saying that http_request is a bad package, only that Thorsten does not have to use it in NTLM.
Btw, when you will work on it, can you use english for the log entries? :)
--Pierre
Thorsten Rinne [2006-07-01 14:05 UTC] I've changed some lines of code:
- renamed the "state" methods
- now using PEAR::Log for logging
- now using PEAR for error handling
-Thorsten
|
