[WIP] Refactor response/request

[yguedidi]

Tests still pass!! :) (EDIT: only first commit)

EDIT: see below

[SammyK]

I'm actually already reworking all this stuff. This PR will cause some pretty big conflicts with what I've got so far. :)

[yguedidi]

I'm waiting for your PR :)

[SammyK]

Fair warning - it's a doozy! :) But now that the master branch doesn't have to be perfect, if @gfosco merges it in, you can add your stuff and we can play code ping pong! :)

[yguedidi]

I think it's better to wait for your PR before considering merging mine ;)

[yguedidi]

@SammyK I'll be happy if you include my changes in your PR :)

[gfosco]

So... are we looking to merge this or close it? My time is still super limited for the next week or so, just fyi.

[yguedidi]

Waitin' for @SammyK 's answer...

[SammyK]

Sorry guys! Almost done with the refactor, but I've been sightseeing at the Grand Canyon so I haven't been able to finish it off yet. I should be back online to a steady internet connection by this weekend. :)

[SammyK]

FYI - finally back in the saddle! Gonna start finishing #141 off. :)

[yguedidi]

👍

[yguedidi]

EDIT : As I said in my comment, this is how I think the refactoring shoud be.
*/!* This is not tested and still got mistakes and missing features

End-user API

Initialisation

// A FacebookApplication entity
$facebookApp = new FacebookApplication($appId, $appSecret);
$appAccessToken = $facebookApp->getAccessToken();

// A Graph API object  that handle requests
$graphApi = new FacebookGraphApi([$httpClient, $version, $beta]); // or just Facebook :)
// with getters/setters for optional parameters

// Get an access token from a helper (or eslewhere, like database), e.g. with FacebookRedirectLoginHelper
$helper = new FacebookRedirectLoginHelper($graphApi, $facebookApp);
$accessToken = $helper->getAccessTokenFromRedirect();

Usage

// Manual
$request = new Request($accessToken, $endpoint[, $method, $params, $eTag]);
$response = $graphApi->handle($request);
$obj = $response->getGraphObject();

// Shortcuts
$obj = $graphApi->get($accessToken, $endpoint[, $params]);
$list = $graphApi->getAsArray($accessToken, $endpoint[, $params]);
$graphApi->post($accessToken, $endpoint[, $params]);
$graphApi->delete($accessToken, $endpoint[, $params]);

Batching

// Manual
$batchRequest = new BatchRequest($accessToken /* or null */, $requests /* array of Request */);
// or
$batchRequest = new BatchRequest($accessToken /* or null */);
$batchRequest->add(new Request(/*...*/)[, $name, $dependsOn, $omit]);

Internal

Global

• Remove most of the statics, only the API is public
• More decoupled code, easier to maintain
• Easier to test (FacebookGraphApi can be mocked). And don't forget that tests should test the public API, not the implementation
• Easier to understand (I think)

Service

• FacebookGraphApi can now be shared by dependency injection

Entities

All entities are value object, immutable.

• FacebookApplication describe an application
• AccessToken associated to an application
• DebugAccessToken extends the above one with debug infos (previously the GraphSessionInfo)
• Code, associated to an application, separate the auth code logic
• SignedRequest associated to an application
• Request handled by the Graph API object
• BatchRequest extends above one with function to add other Request
• Response returned by the Graph API object
• BatchResponse extends above one with functions to get each Response

Optional : Builders, can be added later

Allow fluently create a request or a batch request and execute it

• RequestBuilder build a request, e.g.

$facebookApp->request($graphApi)
    ->withAccessToken('access-token')
    ->withFields(['foo' => 'bar'])
    ->withETag('some-eTag')
    ->get('/foo')
    ->send();

• BatchRequestBuilder build a batch request, e.g.

$response = $facebookApp->batch($graphApi)
    ->newRequest()
    ->withAccessToken('access-token')
    ->get('/me?fields=name')
    ->newRequest('access-token', 'request-name')
    ->get('/me/feed')
    ->send();

@gfosco, @SammyK thoughts ? :)

[SammyK]

Nice - I wish we could have sat down at a cafe to chat about the refactor! :)

1. I really like having a FacebookApplication entity. Maybe call it FacebookApp since that's how the documentation refers to it?
2. The FacebookGraphApi service is a really sweet idea for constructor injection - that'll make it easier to mock requests to Graph with any class that needs it. That's something I couldn't for the life of me figure out how to do elegantly. I would just call it Graph though. :)
3. Since a whole lot of the devs that are using the SDK seem to be using limited features (like a quick login link to grab basic info on a user), I think it'd be beneficial to make the initialization process easy through the Facebook getter/setter/factory. That'd make it easy to new up new Request's and BatchRequest's like in [WIP] Refactored response and request handling and cleaned up the API #159.
4. The arguments to FacebookRedirectLoginHelper should be optional and fallback to defaults so the dev doesn't have to juggle so many classes.
5. I like newing up a Request entity and sending it with $response = $graphApi->handle($request); but I'd call the method getResposne() to be more explicit. And that method could also accept an array of requests and will automatically batch the request and return a BatchResponse. Again, we could also keep the Facebook factories for new requests and batches from [WIP] Refactored response and request handling and cleaned up the API #159 for convenience.
6. Not sure why we need DebugAccessToken entity. I'm all for killing GraphSessionInfo since there is no real-world Graph object called "session info" and therefore it goes against the domain-driven approach to the GraphNodes/* nomenclature, but I feel like all the stuff in DebugAccessToken should be included in AccessToken since an access token is always an access token no matter whether you've called "debug" on it or not. Can you explain a bit more why you think they should be separated?
7. The Code entity is a nice idea!
8. On batching, I like add(Request).
9. Batching with fluent interface: I'm not sure how this will work:

$response = $facebookApp->batch($graphApi)
    ->newRequest()
    ->get('/foo')
    ->newRequest()
    ->get('/bar')
    ->send();

Since batch() would return a BatchResponse entity and newRequest() would return a Request entity. So the syntax would have to look more like this:

$batchRequest = $facebookApp->batch($graphApi);
$batchRequest->newRequest()->get('/foo');
$batchRequest->newRequest()->get('/bar');
$response = $batchRequest->send();

That is how it works in #159.

All in all, this has some nice improvement on #159 and also contains a lot of the same features. As far as implementation, I can fix the conflicts on my PR and @gfosco could merge it to master and then @yguedidi could add this refactor on top of that. Does that sound good to you guys?

[yguedidi]

Thanks!

1. Maybe, as @gfosco want.
2. Same as 1. :)
3. As most of devs will use helpers, I propose something like: $helper = FacebookRedirectLoginHelper::create($appId, $appSecret[, $httpClient, ...]); with getter for graph api and app instances.
4. Resolved by 3.
5. I prefer handle because the request is handled, transformed to a response, it's make more sense to me. @gfosco will decide.
   About the array of response, good idea, but a little bit magic (it will remove the typehint). I propose a shortcut function like get, post and delete: $graphApi->batch($requests[, $accessToken]);
6. TL;DR : It's need a graph call
   Long: Infos provided by the /debug_token endpoint are also properties of an access token, they describe an access token. But there are not available before the graph call, thats why I made a new entity. It differentiate a basic access token (with just a value and and expiration) from a full one with all properties filled from the graph. It's also avoid having functions that always return null in base AccessToken before the graph call is made.
7. I ttought you propose it ling time ago ;)
8. 👍
9. It was just copied-pasted from your exemple lol
   I don't think about it yet, but I separate the responsability to builders classes, to easily made it later.
   $helper->batch() returns a BatchRequestBuilder instance, which have a fluent interface to create a BatchRequest.
   It can be something like:

$response = $helper->batch()
    ->get('/foo') // start new request
    ->withAccessToken(...)
    ->post('/bar') // adds the above request and start a new one
    ->withParams(...)
    ->send(); // add the last request and return a BatchResponse

Note the usage of the helper instance, it make more sense

Thanks again for your thoughts @SammyK.

About how to merge our PRs, its depend on which implementation @gfosco prefer.
They are too different (different new files, entities, ... you use many static, I don't, surely some other differences).
And your PR have some features that still should be discuted like graph object refactoring, the fluent interface to instanciate request (in mine it's not implemented, but it's easily possible thanks to builders) and the little teste refactoring . I think you should remove feature I listed above and focus on request/response handling :)

Let's wait for @gfosco thoughts.

[yguedidi]

UPDATE:

• Add $graphApi->batch($resquests[, $accessToken]) shortcut
• Add an abstract base helper with an abstract function getAccessToken and a static function create. Usage:

$helper = FacebookRedirectLoginHelper::create($appId, $appSecret[, $httpClient, ...]);
$graphApi = $helper->getGraphApi();
$facebookApp = $helper->getApp();

// With requests builders
$response = $helper->request()->...->send();
$batchResponse = $helper->batch()->...->send();

• Because above point, renamed getAccessTokenFromRedirect to just getAccessToken

[SammyK]

I think our PR's are more similar than you think. I can take all the magic stuff & collection/graph object casting and move it to a separate PR. Then there won't be much difference between your PR and mine. The main difference would be that mine tends to focus on making things accessible via Facebook and yours gives the dev a lot more direct access to the objects. And both approaches could be easily implemented.

It'd be cool to have Facebook be a factory for all the things instead of FacebookRedirectLoginHelper::create().

$helper = Facebook::newRedirectHelper();
$helper = Facebook::newJavascriptHelper();
$helper = Facebook::newCanvasHelper();

// and all the things
$graphHandler = Facebook::newGraphHandler(/* ...args... */); // returns FacebookGraphApi
$request = Facebook::newRequest(/* ...args... */);
$batchRequest = Facebook::newBatchRequest(/* ...args... */);

And I think you'd want to always inject FacebookGraphApi instead of the HTTP client. Only FacebookGraphApi should be concerned about HTTP client.

[yguedidi]

We have got the same approch, yours is focused on a new Facebook class, and mine use helpers as the main access.

$helper = FacebookRedirectLoginHelper::create($appId, $appSecret[, $httpClient, ...]);
$helper = FacebookJavascriptLoginHelper::create($appId, $appSecret[, $httpClient, ...]);
$helper = FacebookCanvasLoginHelper::create($appId, $appSecret[, $httpClient, ...]);

// and all the things

$graphApi = $helper->getGraphApi();
// same as $graphHandler = Facebook::newGraphHandler(/* ...args... */);

$request = $helper->request()->...->send() // use the RequestBuilder
// same as $request = Facebook::newRequest(/* ...args... */);

$request = $helper->batch()->...->send() // use the BatchRequestBuilder
// same as $batchRequest = Facebook::newBatchRequest(/* ...args... */);

From an end user point of view, it's don't make sense to me to have Facebook::newRequest(...); while it's so easy to usenew Request(...).

[SammyK]

The main point of having factories like $request = Facebook::newRequest(/* ...args... */); would be to have the httpClient or FacbeookGraphApi injection handled for you. Since most devs won't be concerned with overwriting the defaults, it'd be nice to not force them to juggle the dependencies.

Again - both ways can be implemented, the Facebook factories are just convenience. :)

And if we tightly couple the factories to the helpers, then it'll difficult to make direct calls to Graph when you already have an access token (from the database for example). Or did I misunderstand? :)

[yguedidi]

@SammyK, parameters between square brackets are optionnal! :)
So you can use $helper = FacebookRedirectLoginHelper::create($appId, $appSecret);, see this line and this one about the httpClient.
And as the builders are instanciated from the helper, there dependencies is automatically injected, take a look at AbstractFacebookHelper.

Finally, the simpliest way to use this implementation can be:

$helper = FacebookRedirectLoginHelper::create($appId, $appSecret);
$response = $helper->request()->...->send();

This will use as default access token the one from the helper getAccessToken() function.

About an existing access token, there is the manual way:

$request = new Request($accessToken, $endpoint[, $method, $params, $eTag]);
$response = $helper->getGraphApi()->handle($request);

but also shortcuts:

$response = $helper->getGraphApi()->get($accessToken, $endpoint[, $method, $params, $eTag]);

and through fluent builder:

$response = $helper->request($accessToken)->...->send();

[SammyK]

Cool - well it's quite a bit different than the API we discussed in #141, but I like some of your points (that I called out above) so I think we can let master @gfosco weigh in! :)

PS: Do you live in France? You have any plans on coming to the States anytime soon?

[yguedidi]

Finally this allow different usage:

• a basic one, used by most of users

$helper = <a helper class>::create($appId, $appSecret);
// without access token, use helper's one
$response = $helper->request()->...->send();
// with already exists access token
$response = $helper->request($accessToken)->...->send();

• an intermediate one, use shortcuts:

$helper = <a helper class>::create($appId, $appSecret);
// without access token
$accessToken = $helper->getAccessToken();
// with access token
$accessToken = new AccessToken($helper->getApp(), $value):
// then
$response = $helper->getGraphApi()->get($accessToken, $endpoint, ;..);

• an advance one, directly use core object:

// without access token, must use a helper
$helper = <a helper class>::create($appId, $appSecret);
$facebookApp = $helper->getApp();
$accessToken = $helper->getAccessToken();
$graphApi = $helper->getGraphApi();
$response = $graphApi->handle(new Request($accessToken, $endpoint, ...));

// with access token, do not need helper
$facebookApp = new FacebookApplication($appId, $appSecret);
$graphApi = new FacebookGraphApi([, $httpClient, ...]);
$accessToken = new AccessToken($facebookApp, $value):
$response = $graphApi->handle(new Request($accessToken, $endpoint, ...));

But there is no reason to not use the easier way! :)
This PR "just" refactor the core and adds some shortcut functions.
Of course, all names should be changed.
About the Facebook factory, it will surely can be added on top of this ;)

@SammyK Yes I live in France, and no plan to go in the States sorry

[yguedidi]

@gfosco @SammyK I splitted my refactor in multiple commits.
It's now easier to review this commit by commit.
I'll add tests if @gfosco like this :)

[gfosco]

Builders? I'm not sure that is necessary... BatchBuilder less so (I'm still on the side of it just being an array of normal requests).

[yguedidi]

Yes builders. They decouple the fluent interface to create requests. They just helpers to easily create requests on top of the core refactor. They are not necessary at a first time.

[yguedidi]

Closes for reasons I say in this comment

[SammyK]

Yeah, I love fluent interfaces, but if we add them to the SDK then the Facebook Query Builder could become useless! Lol. So I'm cool without adding them to the SDK. But! I would still like to add factories. More thoughts to come on that...

[yguedidi]

They aren't as advanced as your FQB ;)