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

Fully document the ProviderInterface #273

Merged
merged 2 commits into from Apr 27, 2015
Merged

Fully document the ProviderInterface #273

merged 2 commits into from Apr 27, 2015

Conversation

shadowhand
Copy link
Member

Move existing documentation from AbstractProvider and fill in any missing docblocks.

* @param array $result
* @return void
*/
public function errorCheck(array $result);
Copy link
Member Author

Choose a reason for hiding this comment

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

@ramsey This method was missing from the interface. As per our discussion earlier about the error handling flow, I plan to modify this method (and name it better).

@shadowhand
Copy link
Member Author

@ramsey I'm not sure why this build failed only on HHVM... any clue?

@ramsey
Copy link
Contributor

ramsey commented Apr 27, 2015

The failure message on HHVM is:

Fatal error: Class Mock_AbstractProvider_c5873f51 contains abstract method (urlAuthorize) and must therefore be declared abstract or implement the remaining methods in /home/travis/build/thephpleague/oauth2-client/vendor/phpunit/phpunit-mock-objects/src/Framework/MockObject/Generator.php(301) : eval()'d code on line 1

I'm not sure why it's specific to HHVM, though.

@shadowhand
Copy link
Member Author

@ramsey found the issue, it's a HHVM bug: sebastianbergmann/phpunit-mock-objects#223 and facebook/hhvm#5170. Should I apply the work around (define the methods in the abstract class) or are we okay with breaking HHVM usage?

@ramsey
Copy link
Contributor

ramsey commented Apr 27, 2015

Is this only because our mock provider doesn't fully implement all the methods? Or is it a general issue with abstract classes in HHVM, where they always need to implement the full interface?

If it's only an issue with our mock provider for the tests, then please define the methods in the mock object for the test. Otherwise, let's think about it for a bit. I want to follow up with HHVM folks to see if this is something they'll be fixing in future versions.

@shadowhand
Copy link
Member Author

@ramsey based on the HHVM issue I referenced, it looks like they intend to fix it, but have no timeline for doing so. I think the best thing to do for now would be to implement all the methods in the abstract.

@ramsey
Copy link
Contributor

ramsey commented Apr 27, 2015

Okay. That's fine. When HHVM does fix it, we can remove them.

Move existing documentation from `AbstractProvider` and fill in any missing docblocks.
*/
// Implementing these interfaces methods should not be required, but not
// doing so will break HHVM because of https://github.com/facebook/hhvm/issues/5170
// Once HHVM is working, delete the following abstract methods.
Copy link
Member Author

Choose a reason for hiding this comment

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

@ramsey added here as a separate commit that can be reverted in the future.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 84.78% when pulling 4d998f5 on shadowhand:feature/v1-interface-docblocks into b9e6b72 on thephpleague:1.0.

@shadowhand
Copy link
Member Author

Strange the coverage decreased, since no actual code was modified. I guess it's because of the added abstract methods.

Also, I made a quick script to check these after I kept missing methods.

@ramsey
Copy link
Contributor

ramsey commented Apr 27, 2015

Thanks!

ramsey added a commit that referenced this pull request Apr 27, 2015
@ramsey ramsey merged commit cd14488 into thephpleague:1.0 Apr 27, 2015
@shadowhand shadowhand deleted the feature/v1-interface-docblocks branch April 27, 2015 20:00
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

3 participants