Code Review for PHP SDK support for (Albums & MediaItems)

I tried to extract out only the new stuff for albums and mediaItems for review. 

There's also a fix to the postBody in 2LeggedOAuth.

Obviously you'll not be able to hit our dev server to see these in action. I've made some changes to the examples and included example code for each of these.

Added base model for all classes to extends with setField() and getField() functions. This greatly simplifies getting and setting variables.

Also modified the System Service because it was throwing an exception.

[chabotc, 2009-08-18 09:41:12]

Here's my initial code review

http://codereview.appspot.com/108048/diff/1/2
File src/examples/__init__.php (right):

http://codereview.appspot.com/108048/diff/1/2#newcode111
Line 111: //$userId = '487802446';
if these are new valid test values then we should remove the old ones instead of
commenting them out

http://codereview.appspot.com/108048/diff/1/4
File src/osapi/auth/osapiOAuth2Legged.php (right):

http://codereview.appspot.com/108048/diff/1/4#newcode86
Line 86: unset($oauthRequest->parameters[$postBody]);
Have you tested this change on orkut/netlog/hi5/partuza?

The thinking about the body hash functionality is that:

- If the container supports this new extention, a new query param is added
(oauth_body_hash) and the body is not included in the signature
- If the container does not support it, include the raw post body in the oauth
signature

The logic here seems to *include* the post body when the container *does*
support the oauth extention, and remove the post body from the oauth signature
if it does not..

http://codereview.appspot.com/108048/diff/1/7
File src/osapi/model/osapiMediaItem.php (right):

http://codereview.appspot.com/108048/diff/1/7#newcode44
Line 44: var $url;
The old mime-type (audio/video/image) logic is still valid right? At least when
using media items in activities this is still a required field and creating
activities w/ media items without one of those 3 type values will make it fail.

I'm all for re-use & reusing this for albums/etc is great, but I would prefer to
not break the original functionality while doing so ;)

http://codereview.appspot.com/108048/diff/1/8
File src/osapi/model/osapiModel.php (right):

http://codereview.appspot.com/108048/diff/1/8#newcode51
Line 51: //require_once "osapiGroup.php";
is this work in progress? These should either be filled out or removed

http://codereview.appspot.com/108048/diff/1/9
File src/osapi/osapi.php (right):

http://codereview.appspot.com/108048/diff/1/9#newcode81
Line 81: $this->albums = new osapiAlbums();
As the list of service end-points is growing, I'm starting to think that maybe
using the magic __get function is more appropriate, in 90% of the cases only one
or two end-point types will be used anyway, so on-demand-instancing might become
a useful optimization at some point.

My thinking would be to implement something like:
function __get($var) {
  if (in_array($var, array('people','etc')) {
     $class = 'osapi' . ucfirst($var);
     $this->$var = new $class;
     return $this->$var;
  }
}

This way if an end point is already instanced (say ->people), the __get function
wont be called since the var is known, and if a unknown var is called (->foo),
it will throw a warning (since nothing is auto-instanced & returned)

This wasn't really such a big issue when we only supported the 3 main end-points
(people,activities,appdata) but at some point including everything is going to
slow it down.

Likewise I think we can do on-demand including in the service providers
(rest/rpc), and model classses (in the end point classes).

Anyhow, this is more a note-to-self to fix up once this patch goes in then
something that needs to happen in this patch :)

http://codereview.appspot.com/108048/diff/1/10
File src/osapi/providers/osapiMySpaceProvider.php (right):

http://codereview.appspot.com/108048/diff/1/10#newcode27
Line 27: "http://els1vdevroa0001/roa/09", null, "MySpace", true, $httpProvider);
This seems to be a hard-coded debug value?

http://codereview.appspot.com/108048/diff/1/10#newcode80
Line 80: if(isset($data->entry)){
Code formatting isn't really consistent with the rest of the library, but if
that's an issue/habit to hard to break I'm willing to fix that after the final
patch :)

Specifically in this case it's missing spaces around if( }else{ $key=>$value etc
type statements

http://codereview.appspot.com/108048/diff/1/11
File src/osapi/service/osapiService.php (right):

http://codereview.appspot.com/108048/diff/1/11#newcode89
Line 89: //abstract public function getSupportedFields();
In Shindig the supported fields has never been fully implemented, will MySpace
support these (properly) ? If not we can leave this function out

http://codereview.appspot.com/108048/diff/1/12
File src/osapi/service/osapiSystem.php (right):

http://codereview.appspot.com/108048/diff/1/12#newcode39
Line 39: public function get($params) {
The default value was there so that no matter how you called
system->{update,delete,create} you would always get the exception telling you
that those functions can't be called. What was the reason for removing that?

http://codereview.appspot.com/108048/diff/1/12#newcode45
Line 45: throw new osapiException("osapiCache only supports the invalidate
method");
This is more a note-to-self, copy & pasted reference to osapiCache should be
osapiSystem, and the { is on the wrong line, doh

[Harakiro, 2009-08-18 17:45:47]

Please see my responses to each comment.

Jesse

http://codereview.appspot.com/108048/diff/1/2
File src/examples/__init__.php (right):

http://codereview.appspot.com/108048/diff/1/2#newcode111
Line 111: //$userId = '487802446';
On 2009/08/18 09:41:12, chabotc wrote:
> if these are new valid test values then we should remove the old ones instead
of
> commenting them out

These are my test values, I'll create and add an account with appropriate app
permissions before commit.

http://codereview.appspot.com/108048/diff/1/4
File src/osapi/auth/osapiOAuth2Legged.php (right):

http://codereview.appspot.com/108048/diff/1/4#newcode86
Line 86: unset($oauthRequest->parameters[$postBody]);
On 2009/08/18 09:41:12, chabotc wrote:
> Have you tested this change on orkut/netlog/hi5/partuza?
> 
> The thinking about the body hash functionality is that:
> 
> - If the container supports this new extention, a new query param is added
> (oauth_body_hash) and the body is not included in the signature
> - If the container does not support it, include the raw post body in the oauth
> signature
> 
> The logic here seems to *include* the post body when the container *does*
> support the oauth extention, and remove the post body from the oauth signature
> if it does not..
> 
> 

I will do some testing with removing this revision. Before when I tested this I
could never get it to work correctly. I'll test this against some of the other
containers.

http://codereview.appspot.com/108048/diff/1/7
File src/osapi/model/osapiMediaItem.php (right):

http://codereview.appspot.com/108048/diff/1/7#newcode44
Line 44: var $url;
On 2009/08/18 09:41:12, chabotc wrote:
> The old mime-type (audio/video/image) logic is still valid right? At least
when
> using media items in activities this is still a required field and creating
> activities w/ media items without one of those 3 type values will make it
fail.
> 
> I'm all for re-use & reusing this for albums/etc is great, but I would prefer
to
> not break the original functionality while doing so ;)

Yes, I have that is a bit of TODO for all services. Adding field checks and
restrictions. The logic should be there, however, it doesn't break anything
currently.

Will look into over-riding the setField and do some logic testing inside here.

http://codereview.appspot.com/108048/diff/1/8
File src/osapi/model/osapiModel.php (right):

http://codereview.appspot.com/108048/diff/1/8#newcode51
Line 51: //require_once "osapiGroup.php";
On 2009/08/18 09:41:12, chabotc wrote:
> is this work in progress? These should either be filled out or removed

Yes, these are all the items I've been working on and have broken out for ease
of review. We can remove these for this commit.

http://codereview.appspot.com/108048/diff/1/9
File src/osapi/osapi.php (right):

http://codereview.appspot.com/108048/diff/1/9#newcode81
Line 81: $this->albums = new osapiAlbums();
On 2009/08/18 09:41:12, chabotc wrote:
> As the list of service end-points is growing, I'm starting to think that maybe
> using the magic __get function is more appropriate, in 90% of the cases only
one
> or two end-point types will be used anyway, so on-demand-instancing might
become
> a useful optimization at some point.
> 
> My thinking would be to implement something like:
> function __get($var) {
>   if (in_array($var, array('people','etc')) {
>      $class = 'osapi' . ucfirst($var);
>      $this->$var = new $class;
>      return $this->$var;
>   }
> }
> 
> This way if an end point is already instanced (say ->people), the __get
function
> wont be called since the var is known, and if a unknown var is called (->foo),
> it will throw a warning (since nothing is auto-instanced & returned)
> 
> This wasn't really such a big issue when we only supported the 3 main
end-points
> (people,activities,appdata) but at some point including everything is going to
> slow it down.
> 
> Likewise I think we can do on-demand including in the service providers
> (rest/rpc), and model classses (in the end point classes).
> 
> Anyhow, this is more a note-to-self to fix up once this patch goes in then
> something that needs to happen in this patch :)

Lazy loading is always good! great idea i'll look into adding this to this file.

http://codereview.appspot.com/108048/diff/1/10
File src/osapi/providers/osapiMySpaceProvider.php (right):

http://codereview.appspot.com/108048/diff/1/10#newcode27
Line 27: "http://els1vdevroa0001/roa/09", null, "MySpace", true, $httpProvider);
On 2009/08/18 09:41:12, chabotc wrote:
> This seems to be a hard-coded debug value?

Yes, we've not published these endpoints yet. This would be updated before code
is committed.

http://codereview.appspot.com/108048/diff/1/10#newcode80
Line 80: if(isset($data->entry)){
On 2009/08/18 09:41:12, chabotc wrote:
> Code formatting isn't really consistent with the rest of the library, but if
> that's an issue/habit to hard to break I'm willing to fix that after the final
> patch :)

I'll tweak it a bit. I think this code was written before I got my Tabs set
correctly =).
> 
> Specifically in this case it's missing spaces around if( }else{ $key=>$value
etc
> type statements

http://codereview.appspot.com/108048/diff/1/11
File src/osapi/service/osapiService.php (right):

http://codereview.appspot.com/108048/diff/1/11#newcode89
Line 89: //abstract public function getSupportedFields();
On 2009/08/18 09:41:12, chabotc wrote:
> In Shindig the supported fields has never been fully implemented, will MySpace
> support these (properly) ? If not we can leave this function out

Yes, MySpace fully supports @supportedFields based on the v0.9 REST spec.

http://codereview.appspot.com/108048/diff/1/12
File src/osapi/service/osapiSystem.php (right):

http://codereview.appspot.com/108048/diff/1/12#newcode39
Line 39: public function get($params) {
On 2009/08/18 09:41:12, chabotc wrote:
> The default value was there so that no matter how you called
> system->{update,delete,create} you would always get the exception telling you
> that those functions can't be called. What was the reason for removing that?

Once I included this class, I kept getting an error stating that These functions
must match the abstract definition. The abstract class does not have defaults
therefore there was a conflict. I only removed the defaults because of that. The
better solution is to add defaults to the abstract functions, and ALL other
services.

[Arne Roomann-Kurrik, 2009-08-18 17:53:06]

http://codereview.appspot.com/108048/diff/1/4
File src/osapi/auth/osapiOAuth2Legged.php (right):

http://codereview.appspot.com/108048/diff/1/4#newcode86
Line 86: unset($oauthRequest->parameters[$postBody]);
You should be able to tell if a lot of the OnlineTests start to fail- most of
these containers are covered by that suite (which has errors if everything is
working 'correctly', but the errors should either be quota restrictions or not
implemented notifications)

[Harakiro, 2009-08-21 17:22:09]

Addresses comments, and testing from patch 1

[Harakiro, 2009-08-24 20:25:03]

Just checking in if anyone has had a chance to review the 2nd patch I put up and
what are next steps to earn my commit stripes =).

Jesse

[Arne Roomann-Kurrik, 2009-08-26 02:16:05]

Sorry to take so long with this review.  There seems to be a couple files
missing from this patch set.

I've also updated osapiRestIO.php in the meantime - you probably won't need the
changes to constructUrl, as I'm assuming they were there to fix the problem
where an object was being passed to urlencode.

You also removed a fix for @me/@self in the myspace provider, although I see
you've changed the service URL as well, so maybe it's not needed any more.

The other changes look good, if you could add the missing files and sync with
the newest version, I'll be happy to commit.

http://codereview.appspot.com/108048/diff/2003/2007
File src/osapi/auth/osapiOAuth2Legged.php (right):

http://codereview.appspot.com/108048/diff/2003/2007#newcode85
Line 85: unset($oauthRequest->parameters[$postBody]);
I believe that this breaks the functionality of the previous version.  Certain
OAuth libraries expected the post body to be x-www-form-urlencoded, so on some
containers, if they got a POST that was application/javascript, they stuffed the
entire POST body into the request verification process, treating it like a very
long parameter with a value of "".  This code targeted containers which had the
invalid OAuth libraries.

I think most implementations should be fixed now, so we can probably remove this
fix, but that should be a whole different patch.

Does MySpace break without your modification?  We could probably add another
switch like $this->useBodyHack to continue to use the workaround on containers
that require it.

http://codereview.appspot.com/108048/diff/2003/2010
File src/osapi/io/osapiRestIO.php (right):

http://codereview.appspot.com/108048/diff/2003/2010#newcode74
Line 74: 
Every change including and below this probably isn't needed any more - there was
a bug introduced in this file a couple of versions back which caused an error by
trying to urlencode an object, but I patched a fix in today.

http://codereview.appspot.com/108048/diff/2003/2015
File src/osapi/providers/osapiMySpaceProvider.php (left):

http://codereview.appspot.com/108048/diff/2003/2015#oldcode66
Line 66: private function fixAtMe(osapiRequest &$request, &$url, osapiAuth
&$signer) {
I still get errors if I try to request @me/@self with a person request from
MySpace.  Are you sure that this fix isn't required?

http://codereview.appspot.com/108048/diff/2003/2016
File src/osapi/service/osapiService.php (right):

http://codereview.appspot.com/108048/diff/2003/2016#newcode24
Line 24: require_once "osapiAlbums.php";
I don't see osapiAlbums.php in this patch - was it left out by accident?

http://codereview.appspot.com/108048/diff/2003/2016#newcode25
Line 25: require_once "osapiMediaItems.php";
I see osapiMediaItem.php (no 's').  Is there another file for MediaItems,
plural?

[Harakiro, 2009-08-26 18:01:35]

Please see comments below.

I'll post a patch with missing files very shortly.

http://codereview.appspot.com/108048/diff/2003/2007
File src/osapi/auth/osapiOAuth2Legged.php (right):

http://codereview.appspot.com/108048/diff/2003/2007#newcode85
Line 85: unset($oauthRequest->parameters[$postBody]);
yes I've tried several times with this code and it just won't work with
mediaItem upload.

Myspace doesn't currently support the oauth_body_has, and appending the body
parameter of a binary file doesn't work at all... I'm not sure how best to fix
this and keep it backwards compatible as I don't know which services need this?

http://codereview.appspot.com/108048/diff/2003/2015
File src/osapi/providers/osapiMySpaceProvider.php (left):

http://codereview.appspot.com/108048/diff/2003/2015#oldcode66
Line 66: private function fixAtMe(osapiRequest &$request, &$url, osapiAuth
&$signer) {
You're seeing this error because currently not all v0.9 endpoints are live. If
you look at the x-opensocial-error in the header it will say that its currently
disabled.

I tested @me on albums and mediaItems and these work fine. The update changes
the rest endpoint so all non-exposed endpoints. We are planning on rolling this
out very soon which is why I'm trying to get all this commmited / commiter
status so we can build out the rest of the changes.

[Harakiro, 2009-08-26 18:08:48]

Fiinal patch for this *I hope*