Skip to content
This repository has been archived by the owner on Jan 18, 2018. It is now read-only.

Params Added #63

Closed
wants to merge 6 commits into from
Closed

Params Added #63

wants to merge 6 commits into from

Conversation

Balamir
Copy link

@Balamir Balamir commented Jan 6, 2016

No description provided.

Abdullah Ceylan and others added 2 commits January 6, 2016 15:11
Added icon_emoji, icon_url, link_names params
@GrahamCampbell
Copy link
Member

Please could you rebase and force push to avoid the merge commit?

@GrahamCampbell
Copy link
Member

Ping @notifymehq/owners for review.

@Balamir
Copy link
Author

Balamir commented Jan 6, 2016

For future?

@GrahamCampbell
Copy link
Member

For future?

What do you mean?

@jbrooksuk
Copy link

What do you mean?

Huh?

@jbrooksuk
Copy link

Also a 👍 for me.

@GrahamCampbell
Copy link
Member

image

Changed the "body" request to "form_params". This is fixed that error:

Passing in the "body" request option as an array to send a POST request
has been deprecated. Please use the "form_params" request option to send
a application/x-www-form-urlencoded request, or a the "multipart"
request option to send a multipart/form-data request.
@Balamir
Copy link
Author

Balamir commented Jan 6, 2016

(; Haha, funny.

I'm using Github for Windows and I couldn't find that option. So, totally it's fault.

(; Yes, a 👍 for you, @jbrooksuk

Fixed problem with guzzle's new version
@GrahamCampbell
Copy link
Member

Now there are even more merge commits. ;(

@GrahamCampbell
Copy link
Member

Just open the cli and run the git commands manually. GitHub for windows doesn't have a way to force push.

@Balamir
Copy link
Author

Balamir commented Jan 6, 2016

Ok, I'll try.

@Balamir
Copy link
Author

Balamir commented Jan 6, 2016

I tried but it's says "everything up-to-date". I guess I'm doing something wrong. Sorry :(

sshot-73

So, I couldn't figured out.

@GrahamCampbell
Copy link
Member

You need to git pull --rebase upstream master first.

@GrahamCampbell
Copy link
Member

If it says that doesn't exist, run git remote add upstream https://github.com/notifymehq/notifyme.

@Balamir
Copy link
Author

Balamir commented Jan 6, 2016

Thats the result:

sshot-74

@GrahamCampbell GrahamCampbell self-assigned this Jan 6, 2016
@GrahamCampbell
Copy link
Member

Why the guzzle change?

@Balamir
Copy link
Author

Balamir commented Jan 6, 2016

The new version of guzzlehttp is doesn't pass the 'body' option for the request. It's deprecated and changed to 'form_params'. When I try, it's give an error:

Passing in the "body" request option as an array to send a POST request
has been deprecated. Please use the "form_params" request option to send
a application/x-www-form-urlencoded request, or a the "multipart"
request option to send a multipart/form-data request.

So, I changed it.

@GrahamCampbell
Copy link
Member

We need to support both versions of guzzle.

@joecohens
Copy link
Member

if (version_compare(ClientInterface::VERSION, '6') === 1) {
    $request['form_params'] = $params;      
} else {
    $request['body'] = $params;
}

/cc @Balamir

@GrahamCampbell
Copy link
Member

There might be cases in the other adapters too.

@Balamir
Copy link
Author

Balamir commented Jan 6, 2016

Could we use like that:

    // GuzzleHttp Version < 6
    $param_querystring = 'body';
    // GuzzleHttp Version = 6
    if ( version_compare( ClientInterface::VERSION, '6' )  === 1 ) {
        $param_querystring = 'form_params';      
    }

/cc @joecohens

This should be apply to Pushover, Twilio, Yo adapters too.

Abdullah Ceylan added 2 commits January 6, 2016 23:37
This reverts commit eb80a40b505f7713234235684eb24491d00c332a.
Added GuzzleHttp version compare to fix the post problems.
@joecohens
Copy link
Member

@Balamir maybe it's simpler

$requestBodyKey = (version_compare( ClientInterface::VERSION, '6' ) === 1) ? 'form_params' : 'body';

Also can you squash please?

@Balamir
Copy link
Author

Balamir commented Jan 7, 2016

Also can you squash please?

@joecohens What do you mean?

@jbrooksuk
Copy link

@joecohens What do you mean?

Make it into one commit :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants