Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

facebook module - add support for facebook/php-sdk-v4 version 5 #2722

Closed
wants to merge 8 commits into from
Closed

facebook module - add support for facebook/php-sdk-v4 version 5 #2722

wants to merge 8 commits into from

Conversation

orhan-swe
Copy link
Contributor

all test are passing now but the changes do break support for version 4 unfortunately

))->execute();

switch ($method) {
case 'GET' :

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There must be no space before the colon in a CASE statement

*/
public function seePostOnFacebookWithAttachedPlace($placeId)
public function seePostOnFacebookWithAttachedPlace($message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be named seePostOnFacebookWithMessage?

@Naktibalda
Copy link
Member

@tiger-seo Please review this pull request.

@orhan-swe
Copy link
Contributor Author

@Naktibalda Thanks, I have added the changes, I am not sure why the WebDriverTest::testTextFieldByNameFirstNotCss failed in the previous commit, it is passing on my local machine

@Naktibalda
Copy link
Member

WebDriver fails in random place in 1 run out of 10 or 20.

@orhan-swe
Copy link
Contributor Author

ok thanks, good to know

@@ -4,22 +4,30 @@
*/
namespace Codeception\Lib\Driver;

use Facebook\FacebookSession;
use Facebook\FacebookRequest;
use Facebook\Facebook as Facebook_SDK;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not consistent with PSR, please, change to BaseFacebook or FacebookSDK

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just FacebookSDK? That looks cleaner to me. It is not common to use underscores in class names (unless they come from a pre-namespace era).


public function _depends()
{
return 'Codeception\Module\PhpBrowser';
if ($this->config['depends'] == 'WebDriver')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline control structures are not allowed

private function checkPublishPermissions()
{
if (!in_array('publish_actions', $this->config['test_user']['permissions']) || !in_array(
'user_posts',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-line function call not indented correctly; expected 12 spaces but found 16

@DavertMik
Copy link
Member

Restarted tests. I hope this PR doesn't bring a BC break, right? If not, I think we can merge it.


return json_decode($response, true);
return $response->getDecodedBody();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this response is the same as it was with fb v4 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@tiger-seo
Copy link
Member

@orhan-swe please, resend your PR to branch Codeception:feature/facebook-upgrade, so that we can finish it and merge to the 2.1. thanks

ps. please, squash the commits 😉

@orhan-swe
Copy link
Contributor Author

This was wrong branch. A new pull request has been created for feature/facebook-upgrade branch: #2772

@orhan-swe orhan-swe closed this Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants