[6.0][Proposal] Refactor Graph nodes

[yguedidi]

I basicly want to simplify node casting.

The idea comes from all discussions about magic. I saw while ago that all node methods are a simple call to getFiled(), but how this work? Thanks to the father class GraphNode of course! Using a protected static casting map, the casting of child field is "delayed" to the parent class, which is then responsible of all the casting. A bit "magic" right?

I suggest to simplify GraphNode and remove all the autocasting feature, and add "caster" methods like ->getFieldAsNode(), ->getFieldAsDate(), and ->getField() will return the raw value.

Then, all specialized node will be responsible about cast its field, exemple with GraphAchievement: I don't handle the case of null because I have a thought about that, who wants me open a issue? :)

public function getFrom()
{
    return new GraphUser($this->getField('from'));
}

public function getPublishTime()
{
    return $this->getFieldAsDate('publish_time'));
}

We also can imagine something like ->getFromAsUser(), ->getFromAsPage, etc.. Don't forget that with the SDK, we are in a YKWYRYKWYG (You Know What You Rrequest, You Know What You Get) basis, so it's the responsability of the developer to call the right method to get the right type.

Thoughts?

[devmsh]

mmmm, this means that you need also to remove the Array Access, because $achievement['from'] will not be auto cast? and the var_dump will return uncast fields!

It is clean, straightforward, and make support for additional casting like edge, non root nodes, and open graph nodes very easy, but I feel that we will loose some flexibility.

I'm really confused about that

[SammyK]

I certainly think that would make the Graph node casting much clearer & the complexity would be reduced. I have a few questions.

and ->getField() will return the raw value.

What would the raw return value of a Graph node be? Would it just return an array?

this means that you need also to remove the Array Access, because $achievement['from'] will not be auto cast?

I'm not sure why we'd need to remove array access since: $node['field'] === $node->getField()``

@yguedidi How would recursion of embedded nodes work? For example the Graph user node will return a GraphUser for the significant_other field. This is especially important when handling response data from a nested request.

[SammyK]

PS: If @gfosco updates the developer docs before the end of this week cough, then I'd tag this refactor as 6.0. :)

[yguedidi]

What would the raw return value of a Graph node be? Would it just return an array?

@SammyK Yes, as currently response JSON objects are decoded as arrays (This is also an issue in my list haha)

you need also to remove the Array Access, because $achievement['from'] will not be auto cast?

@devmsh Yes, as you know, I'm not a fan of array access in that case. But if some users really (really) want array access, I would need some args to understand, and please think about that:

Accessing a key is in fact a call to __offsetGet() right? what's its return type? Should it be something like string|\DateTime|GraphUser|GraphPage|GraphAlbum|GraphEvent|...|null like now? Too weird, too magic, not explict at all... That said, if we decide to keep array acces for node, then it will works as @SammyK said: $node['field'] === $node->getField('field')

[devmsh]

If we need kill all the magic, and make GraphNode and GraphNodeFactory more clear, we also need to separate the edge casting to be the responsibility of GraphEdgeFacroty and GraphEdge.

For example:

/* direct edge */
$response = $fb->get('me/events');
$events = $response->getGraphEvent();

/* nested edge */
$response = $fb->get('me?fields=events');
$user = $response->getGraphUser();
$events = $user->getEvents();

/* node */
$response = $fb->get('{eventID}');
$event = $response->getGraphEvent();

Currently GraphNodeFactory try to guess if the result is edge or node using castAsGraphNodeOrGraphEdge, but we need to move this check to FacebookResponse or kill it at all and use getGraph{type} and getGraph{type}s for node and edge respectively.

$response = $fb->get('me/events');
$events = $response->getGraphEvents();

$response = $fb->get('{eventID}');
$event = $response->getGraphEvent();

and

class GraphUser ...{
    function getEvents(){
        // whoever it come :D 
    }
}

[yguedidi]

I totaly agree, that's why I also have in mind to remove the factories.

There are useless now.

• Checking the raw data format is the responsability of the base classes (GraphNode and GraphEdge)
• Each node should have getters for its edges in case they are requested with ?fields= (ex getPhotos() in GraphUser)
• FacebookResponse should also have "caster" method like now: getAsNode(), getAsUser() for a node (/{node-id}), getAsEdge(), getAsPhotos() for an edge (/{node-id}/photos)

Some code:

// in GraphNode or GraphEdge
public function __construct($rawData)
{
    // here check the format (like in the current GraphNodeFactory)
}

// in FacebookResponse (I prefer casters to be named with "as")
public function getAsUser()
{
    return new GraphUser($this->decodedBody);
}

public function getAsPhotos()
{
    return new GraphPhotos($this->decodedBody);
}

// in GraphUser
public function getPhotos()
{
    return new GraphPhotos($this->getField('photos'));
}

Simple. Clear. Explicit. (Magicless.)

[devmsh]

Sorry, I update the comment to add the nested edge scenario before I see your comment

I think that functions like getPhotos in GraphUser must return GraphEdge - collection of GraphPhoto - rather than GraphPhotos?

[yguedidi]

GraphPhotos is a specialized GraphEdge that explicit the return type (GraphPhoto), and add its fields getters (eg getSummary() for GraphFriends)

[devmsh]

Mmmm, so you will have GraphEvents and GraphEvent, GraphPhotos and GraphPhoto ... etc?

What is the benifit of that?

[yguedidi]

What is the benefit of all node classes? :)

[yguedidi]

Here it's the same, but for edges

[devmsh]

Mmmmm Nodes have different fields so different getters included in each node class

But as I think there is no differrances between edges? The all have common functionallity expect that the special picture edge?

[yguedidi]

Some edges have fields, and the returned type when you loop over it is not the same.

[devmsh]

Can you please provide any referance or examples?

[yguedidi]

User friends edge

[devmsh]

I will review it and other edges then I will return back

[yguedidi]

No problem

[devmsh]

Based on my quick review, there is three type of fields

1. fields added to each node like perms
2. fields added in addition to the list of object like summary
3. Some edge also return a list of objects with specific fields that did not represent specific GraphNode, and this mean that GraphEdge data is not restricted to be list of GraphNode!

While we will have many empty GraphEdge sub classes, I'm :+1: the specific GraphEdge and supporting the fields that added to each node in the GraphNode not GraphEdge.

[SammyK]

I'm with you guys on the shady things going on in the GraphNodeFactory. In fact when I try to recruit new contributors, I try to point them to that code since I think it needs to be majorly refactored.

I'm :+1: the specific GraphEdge and supporting the fields that added to each node in the GraphNode not GraphEdge.

I'm curious how complicated we wanted to make the response parsing. Part of me likes the whole recursive auto-casting of Graph node subtypes, but I can see the value of explicit casting as @yguedidi has pointed out.

One of the tough parts about this refactor would be keeping support for deeply nested pagination with lines like this one in the GraphNodeFactory.

[devmsh]

I stop my contribution for a while and wait some issues to be completed, here is the list of pending issues, new features, and enhancements that paused until we close this issue and refactor the graph node.

1. GraphObject sub-classes development #401
   • complete the GraphUser
   • refractor the current nodes based on the metadata=1 response
   • support Comment, Photo, Video and Status
   • separate the non-root node types like GraphCoverPhoto in special namespace, and choose a name that maintain a solid DSL with Facebook.
   • separate the graph node documentation in different files
2. Open Graph Support #406
3. Support list fields on graph nodes #415

Other pending suggestion: enable the wiki and create a road map page or use the milestone feature to organize the issues related to each milestone.

hope that we can finish from that asap :)

[devmsh]

@yguedidi @SammyK

v5.0 finally published, and I think we must finalize this issue, so we can complete the graph node support and refactor the current nodes?

@yguedidi what is your next step?

[devmsh]

Any updates!

[yguedidi]

I don't really have the time to work on this... I'm busy for the 2 next weeks and on holidays without internet connexion for the 3 next weeks... I'll back on half august.

Some steps I can think of now: 1) stop always json_decode() as array, we should keep the data type from responses 2) remove Collection class 3) simplify node/edge field casting

[ghost]

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

[SammyK]

Oh @facebook-github-bot-0. You make me lulz.

[devmsh]

:D

[yguedidi]

I was notified, am I a core team member? lol

[SammyK]

@yguedidi You're a hardcore team member in my book. :)

[yguedidi]

Hahaha you too! ;)

[gfosco]

You guys are great, definitely core team members in my book!!

[devmsh]

@yguedidi @SammyK @gfosco

Must I wait this issue to closed? or can I complete the graph node support?

I think it will take longer than expected, and It's better to complete the graph node support then refactor them after we finish from this issue?

[SammyK]

@devmsh Sorry, I just now saw this! I think you can keep adding to the GraphNodes to provide full support from metadata. We haven't had a full 6.0 discussion yet but I'm thinking there might be one soon. :)