Update the GraphAlbum #533
Conversation
generate the Node class based on the latest metadata from Graph API metadata.
|
I will update the tests and docs once reviewed :) |
|
Looks alright to me.. continue. 👍 |
| * | ||
| * @return GraphUser|null | ||
| * @return GraphNode|null |
There was a problem hiding this comment.
This should still be GraphUser: https://developers.facebook.com/docs/graph-api/reference/v2.5/album
There was a problem hiding this comment.
from the Graph API metadata
{
"name": "from",
"description": "The profile that created the album",
"type": "user|page"
},
The album may created by User or Page, so it must be returned as GraphNode, or as I suggest before, we can have something called GraphProfile as a super class of GraphUser and GraphPage to some common functionality like name, username, picture, ... etc.
So I remove the auto cast, and change the return type to GraphNode until we have another option?
There was a problem hiding this comment.
Gotcha. Well this would be a BC (see the failing test). So we probably need to figure a way to support these changes from Graph without breaking the SDK. @yguedidi Any thoughts?
There was a problem hiding this comment.
What about something like:
/**
* Returns the profile that created the album.
*
* @param bool @isPage true if you want a GraphPage instance
*
* @return GraphUser|GraphPage|null
*/
public function getFrom($isPage = false)
{
if (!$isPage) {
return $this->getField('from');
}
// return a casted to a GraphPage instance
}I'm not really sur. But for now, all users get a GraphUser instance, so I think we should keep this behavior as the default, and provide a way to get a GraphPage. :)
There was a problem hiding this comment.
👍 to @return GraphUser|GraphPage|null docblock :)
|
Status on this, @devmsh ? |
|
Hey @devmsh! Were you able to finish this one out? It'd be great to get this merged in for the next minor release. :) |
ghost
added
the
|
Ping @devmsh. We cool with wrapping this one up? I'd like to merge it in without breaking the build on master. :) |
|
Sorry, I was very busy recently. I will make sure to update the PR maximum tomorrow, and I will review the current SDK status and resume my contributions :) |
|
@devmsh - no worries at all! I've also be crazy busy. Thanks! :) |
|
@devmsh it's been a while... how are you? Do you think you could update this PR? :) |
generate the Node class based on the latest metadata from Graph API
metadata.