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

Default requestParams #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

GuillaumeBolo
Copy link

Some simple code for default requestParams without set/get function
For example "with", "replies" and new futur param if necessary

$sc = new MyUserConsumer( OAUTH_TOKEN, OAUTH_SECRET, Phirehose::METHOD_USER );
$sc->setRequestParam( 'with', 'user' );
$sc->consume();

HTH
Guillaume

For request params without set/get function
ie : with, replies and new param if necessary
@juuga
Copy link

juuga commented Dec 10, 2015

Great PR Guillaume, I hope this is merged.

@fennb
Copy link
Owner

fennb commented Dec 11, 2015

I like this, and it seems low-risk. Mind you, I feel like ultimately the library should support the "real" parameters, but no matter.

@DarrenCook - thoughts?

@juuga
Copy link

juuga commented Dec 11, 2015

Personally, I would be happy with just a setWith('user') if only "real" parameters are hoped for, but as Guillaume said, this PR would be future proof.

@DarrenCook
Copy link
Contributor

No strong thoughts; though if "with" is the only parameter that is missed currently, a setWith() makes more sense?

If sticking with this, maybe call it setDefaultRequestParams(), so people are not so surprised when their setting is ignored. OR, change the following lines to only set things if a value was not set already. E.g. change:
if($this->lang) {
$requestParams['language'] = $this->lang;
}

to:
if($this->lang && ! array_key_exists('language',$requestParams)) {
$requestParams['language'] = $this->lang;
}

And the same for the others.

@GuillaumeBolo
Copy link
Author

@DarrenCook : maybe call it setDefaultRequestParams()

Of course, it was i wanted to mean.
Setter is a better way, but i especially wanted to have a generic solution for futur update of Twitter API

GB

@juuga
Copy link

juuga commented Jan 27, 2016

@fennb @DarrenCook any chance of this being merged, or implemented in the way Darren suggested?

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.

4 participants