Re: Re: [PEPr] -1 for XML::XML_RPC2

Helgi Þormar wrote: > > First using CURL really defeats the purpose of trying to create > something that could be used by future PEAR versions (given they need > xml rpc stuff) since CURL isn't installed by default and I really doubt > anyone wants to make people have CURL installed to use PEAR, anyway that > can be resolved with writing a backend. You either: 1) Use cURL 2) Write your own 'lightweight curl' 3) Use the xmlrpc extension I opted for 1). Another backend should go for 3). 2) isn't really very elegant. > I think you should rename Php to PHP and use PEAR::extensionload('curl') > there someplace since just assuming that people have curl installed and > then throwing a PHP fatal error at them if the function doesn't exists > (curl_init for example). Done. I forgot people could be using a run-time loaded cURL. > > Also don't use () when you do require_once :-) It's not a important > thing to fix but well that's what we recommend and everyone is using. Fixed it > require_once(dirname(__FILE__) . "/Backend/${backend}/Client.php"); > > This is just bad, this is not how PEAR things work, you should be doing > XML/RPC2/Backend ... until other is decided in those matter then this > package also has to ad heir to those rules. Done. > Also please try to use less @ and check for errors and give people back > a usable error message :-) I use it only once, in a call to call_user_func_array, to silence an extraneous warning that accompanies an exception when something goes wrong. The Exception already provides everything, and the warning is not needed. > > You might want to use the PEAR_Exception class ? I can't tho say that > it's a must in any way, specially since I've never had a look at that > one, seems that all the PHP5 people here want people to use it, if not > then please let me know people ;) I took part in that RFC ;-). I'll look into PEAR_Exception. If it's ok, the introduction of the dependency won't cause BC breakage, so it can be done after acceptance. > > Remember to move to the new header standard if the package is accepted. Where is it? > > do not do things like: > if ($foo) throw error > > do: > > if ($foo) { > throw error > } Fixed. > > > try to use ' instead of " where you can. I do. Have you spotted some place where I used " wrongly? > > ---- > > Seems that's just about covers it, I'm not going to go into the design > or architecture since I do not have much experience with XML_RPC stuff > nor do I want to get sucked into some OOP madness :P > Thus I won't vote on this based on my lack of knowledge, anyway those > are just general tips so you can improve things. Thanks for the input Helgi. I've uploaded the new version to the URLs in the proposal. > > - Helgi Cheers, Sérgio Carvalho