[PEPr] Comment on Web Services::Services_Facebook

Joe Stump (http://pear.php.net/user/jstump) has commented on the proposal for Web Services::Services_Facebook. Comment: > * What about moving the connection data out of > Services_Facebook_Common into Services_Facebook_Connection, the > making the Common class instantiate the connection? That allows for > changes to happen specific to the connection without touching every > single object (by inheritance). I'm specifically thinking of > Services_Facebook_Connection_Driver classes to handle curl, sockets, > pecl/http, etc., etc. I'd never implement the driver interface mainly because I doubt a person cares *how* you're connecting to Facebook. That being said, when PEAR2's HTTP_Request comes out I'll use that and add an accept method to you can make connections however you want. > * I would move Services_Facebook_Common::checkRequest() into a > Services_Facebook_Util class as a public so it can be explicitly > tested When would you need to check results explicitly? That's handled in the request method anyways. > * The Users::$userFields array needs to be accessible somehow. I'm > thinking of the cases where you want everything but one or two > fields, or in future cases where you want everything and a few new > fields. Making it public doesn't seem like a great idea though. > Possibly making it protected so you could sub-class S_F_Users and > add your own default fields. How about this: make $userFields > protected, and add a public static $defaultUserFields that is used > on each instance to start the $userFields. Hrm. Good idea. How about a setDefaultUserFields()? I'm fine even making it public. I now see a reason to expose it; it's just a matter of how to go about exposing it? > * Services_Facebook_Photos::$imageTypes is similar to above. It's > not as big of deal here, but just for future extensibility when the > hot new image type is supported and you've moved on to Python or > Ruby :-) The joys of Open Source; when I've bailed for Python you all can add the new image type. > * What is Services_Facebook::isValidRequest() doing? I don't see a > call to it anywhere in the code. Has grep failed me? This function checks *incoming* requests to your server. You'd use it to validate requests to your canvas callback page are, in fact, coming from Facebook. Hence the reason it's not used else where since the other classes are all outgoing. > <code> > $facebook = new Services_Facebook('my-api-key', 'my-secret'); > $facebook->groups->getMembers('gid'); > $facebook->photos->getAlbumsByUser('some-uid'); > $facebook->users->getLoggedInUser(); > $facebook->users->isAppAdded(); > </code> > > That reads more like what is seen in the API docs. The various > properties could be lazy-loaded via __get(), removing the need to > explicitly call factory() all together. I like this a lot. It makes sense. Would you then make Services_Facebook::factory() private? Or leave it exposed? > That might be something better suited to building on top of a > Services_Facebook package as Services_Facebook_Extended. :-) Yeah, how about version 2.0? :-) Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=513 -- Sent by PEPr, the automatic proposal system at http://pear.php.net