Make Hybrid_Endpoint methods non-static #408

Merged
merged 5 commits into from Mar 19, 2015

Projects

None yet

3 participants

@nazar-pc
Contributor
nazar-pc commented Mar 4, 2015

Changed static methods to dynamic, also constructor added.
::process method left for backward-compatibility and calls constructor.

Should not break any existing code using this according to documentation.
Also big question whether we need all that methods and properties to be public?

nazar-pc added some commits Mar 4, 2015
@nazar-pc nazar-pc Nothing changed, just removed spaces at line endings.
Separate commit to decrease diff during real changes.
8ef52a9
@nazar-pc nazar-pc Changed static methods to dynamic, also constructor added.
`::process` method left for backward-compatibility and calls constructor.
7c9e568
@StorytellerCZ
Contributor

I agree with you, but would appreciate @miled feedback on this before merge.

@StorytellerCZ StorytellerCZ added this to the 2.x milestone Mar 5, 2015
@miled
Contributor
miled commented Mar 12, 2015

this class is definitely better now. thank you!

Also big question whether we need all that methods and properties to be public?

no. only ::process need to be public, everything else can be protected.

@StorytellerCZ StorytellerCZ added the 2.x label Mar 12, 2015
@nazar-pc
Contributor

Just made everything except ::process() and constructor protected

@StorytellerCZ
Contributor

Got an error:

Severity: Parsing Error
Message: syntax error, unexpected T_STATIC, expecting T_STRING or T_VARIABLE or '$'
Filename: Hybrid/Endpoint.php
Line Number: 69

specifically here:

public static function process( $request = NULL )
{
    new static( $request );
}
@nazar-pc
Contributor

Is that PHP 5.2?

@nazar-pc
Contributor

Can't remember when I touched 5.2 last time, anyway, should work now as late static binding for modern versions, and as just self for 5.2

@StorytellerCZ
Contributor

Darm, looks like it. I fixed my hosting to 5.4, but the fix worked as well, at least we won't hear complains from legacy people.

@StorytellerCZ StorytellerCZ merged commit 199a0f9 into hybridauth:master Mar 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment