[6.0] Roadmap

[SammyK]

Thoughts? :)

1. Use Guzzle v6 & delete all the HTTP client code
2. Use paragonie/random_compat PHP 7 polyfill & delete all the CSPRNG stuff
3. Remove the autoloader (since SDK will have to be installed with composer)
4. Require PHP 5.6; 5.5 EOL is July 10th!!
5. Remove session handling stuff and make user keep track of CSRF value which will remove all the confusion and bugs associated with the CSRF tokens; see how the PHP League does it
6. Update get() to accept array of params since it's the most requested feature
7. Make default_graph_version required and remove the fallback constant
8. Lock down the public API by making all internal methods private
9. Reduce the number of exceptions (maybe just have one?)
10. Remove the Facebook prefix from nearly all classes
11. Move repo from facebook-php-sdk-v4 to facebook-php-sdk and add 301; abandon facebook/php-sdk-v4 on Packagist and create new alias: facebook/sdk
12. Remove specific GraphNode's and GraphEdge's and just return a Guzzle response stream?
13. Improve batch support
14. Additional tweaks to the public API?
15. Call the "helpers" something more descriptive; perhaps put them under a Context or Environment namespace?

[EmanueleMinotto]

Wow, there are a lot of good changes here!

Require PHP 5.6; 5.5 EOL is July 10th!!

I'm confused only about this point, outside the EOLs and the dependencies' required PHP version what is the reason? Are there any PHP 5.6+ features those could improve the codebase? If yes than it's great, otherwise I'm a bit against this.

My only suggestion is to include a log system (possibly based on PSR-3) for the requests and the API requests debugging.

[browner12]

since this is a major version change, i'm all for switching to a v5.6 requirement.

what's the thought behind removing multiple exceptions and switching to just one? while there could be one parent FacebookException class, it's nice to have more descriptive children so we have more granularity in the things we catch and how we deal with them.

thanks!

[yguedidi]

1. 👎 use HTTPlug, this will help @EmanueleMinotto for logging by using a PSR-7 middleware
2. 👍
3. 👍
4. 👍
5. 👍
6. +0
7. 👍
8. 👍
9. 👎 I'm with @browner12, I like to be specific
10. 👍
11. 👍 but facebook/graph-sdk
12. 👎 but maybe we can find a better way to use them
13. 👍
14. 👍
15. I suggest to remove them, all we need is an access token or a signed request, which are strings. Let the user retrieve them from $_GET, $_POST or $_COOKIE

I'll think of other ideas when I have time :)

[SammyK]

outside the EOLs and the dependencies' required PHP version what is the reason?

EOL means that version of PHP no longer gets updated - for both bug fixes or security issues. So the reason for the upgrade is to ensure the SDK can't be exploited on an old known security hole in an old version of PHP. Plus 5.6 has some TLS improvements that will be helpful for the SDK. :)

what's the thought behind removing multiple exceptions and switching to just one?

The idea would be we could have just two:

1. FacebookSDKException: When a general error is generated from within the SDK
2. FacebookResponseException: When an error response from the Graph API is returned

This is the same as v5 but in v5 FacebookResponseException does a lot of parsing in order to best-guess a sub exception to throw as a "previous exception". It makes the API cumbersome especially when you want to figure out specifically what happened:

try {
    $me = $fb->get('/me');
} catch(FacebookResponseException $e) {
    if ($e->getPrevious() instanceof FacebookAuthenticationException) {
        if ($e->getCode() == 10 || ($e->getCode() >= 200 && $e->getCode() <= 299)) {
            // Missing permissions
        } else {
            // General authentication failure
        }
    } elseif ($e->getPrevious() instanceof FacebookResumableUploadException) {
        //
    } elseif ($e->getPrevious() instanceof FacebookThrottleException) {
        //
    } elseif ($e->getPrevious() instanceof FacebookClientException) {
        //
    }
}

Whereas the API to find the specific error could be made more clear:

try {
    $me = $fb->get('/me');
} catch(FacebookResponseException $e) {
    switch ($e->getType()) {
        case FacebookResponseException::AUTHENTICATION_FAILURE;
            //
            break;
        case FacebookResponseException::MISSING_PERMISSIONS;
            //
            break;
        case FacebookResponseException::RESUMABLE_UPLOAD_FAILURE;
            //
            break;
        case FacebookResponseException::THROTTLE_LIMIT_REACHED;
            //
            break;
        case FacebookResponseException::DUPLICATE_POST;
            //
            break;
    }
}

So you get even more specificity with a cleaner API. :)

[SammyK]

@yguedidi

1. Ooh! Yes, HTTPlug is much better. :)
2. Explained above
3. I'd love to hear your thoughts on improving these. I'd be inclined to just offer a general Collection that all the responses are returned in with some methods to access meta data as collections as well. It would remove a lot of code and makes the API simpler. I'm just seeing a bit of confusion around the internet about when to use the getGraphNode() vs the getGraphEdge() method. :)
4. Love it!

[yguedidi]

@SammyK

9. The problem comes because the fact exception are "previous", but they aren't: an authentication failure is a Graph response exception.
   I think FacebookResponseException should be a parent class that maybe can be a factory for all its child classes.
   Then:

try {
    $me = $fb->get('/me');
} catch(FacebookAuthenticationFailureException $e) {
    //
} catch(FacebookMissingPermissionsException $e) {
    //
} catch(FacebookAuthenticationException $e) {
    // generic authentication exception
} catch(FacebookResumableUploadException $e) {
    //
} catch(FacebookThrottleException $e) {
    //
} catch(FacebookClientException $e) {
    //
} catch(FacebookResponseException $e) {
    // generic exception
}

Clearer isn't it? :)

[SammyK]

Yeah, that's certainly better. I'm just not a fan of throwing a separate exception for every possible error response. But that's just a preference thing so I'll go with whatever the majority want. :)

[yguedidi]

@SammyK with this, the user can choose exactly which exception he/she want to catch. Imagine you want to catch only the missing permission error, with your code you'll need to first catch, and then check for the type. :)

[SammyK]

Makes sense. :)

[tolbon]

When we start :) ?

[SammyK]

@tolbon Haha! Good question. :) I guess the first step is set up the repo for a merging strategy.

@yguedidi If I remember correctly from the last major upgrade. We make master v6 and create new branch 5.0-dev and make it default. Then set up a branch alias in composer.json to point dev-master to that branch? Although I'm realizing that line already exists. Yikes! But does that sound about right? Then I guess we could just start pushing v6 through. :)

[yguedidi]

@SammyK I create two new branches:

• 5.1 based on 5.1.5 for bugfixes in that minor version
• 5.2 based on 5.2.0 for bugfixes too
• master will be used for 6.0 development

Any bugfix must go in the lowest affected branch, then we will have to merge the fixed branch with it's newer one.

What do you thing about this?

[Kastlebrick]

3. NO.. not everyone uses composer. It should not be required.. Personally I hate seeing that crap everywhere ;)

[yguedidi]

Composer is the de facto package dependency manager for PHP, so we will only support this installation method. If you don't want to use composer, you will always be able to download the zip file and write your own autoloader :)

[Kastlebrick]

so we will only support this installation method

Why? Autoloader is already there. Why not give the user the option, like it is now?

Isn't the point of the SDK's to have the needed code written for the Dev's so they don't have to write it!?

[SammyK]

Hey @Kastlebrick! The latest version of the SDK will be pulling in dependencies with composer so even if you write your own autoloader, it would need to include those dependancies.

Is there a specific reason why you're against using composer? :)

[yguedidi]

@Kastlebrick

• a code you write is a code you must maintain, and I personnaly don't want to maintain an old autoloading method
• as an SDK, we have to promote good practices, and composer in a good practice, the new way to manage dependencies
• an other good practice we will promote is to use an up to date version of PHP, that's why we bump it

Isn't the point of the SDK's to have the needed code written for the Dev's so they don't have to write it!?

It isn't! Even a framework will not write your business logic for you :)

BTW, I'm curious about the reason why you don't like composer..

[Kastlebrick]

@SammyK Never used it, don't have it, and it's one less thing I would have to install and maintain on my server.

@yguedidi I think the point I am trying to make is that the SDK should be self contained. And I/we shouldn't have to rely on some other 3rd party code just to make it work. Yes I 'write the business logic', but (in a nutshell) the Autoloader simply loads what is needed for the SDK so I can make that connection to FB.

I really don't want to argue semantics, but what you call 'good practice' I call bad. If you feel it's good to offer the option to use composer, then at least to both.. and don't require it.

I guess, in the end, you will do what you want.. and this is just my two cents, but I don't understand why you would require and rely on some other 3rd party just to make your code work (when it's not necessary).

[kkopachev]

Have you guys thought about distributing sdk in a phar? It will be self-contained, no autoloader needed.

[kbsali]

@SammyK it could be nice to update the description of that issue where each point would mention their corresponding PR (when there is one!) :)

[jintor]

Hi, This should be added in the roadmap