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

Use traits for API operations methods #411

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Jan 4, 2018

r? @brandur-stripe
cc @stripe/api-libraries

Traits are a feature available in PHP 5.4+. Now that we're dropping support for PHP <5.4, we can start using traits to factorize code.

This PR adds traits for common API operations methods (CRUD + list).

@ob-stripe ob-stripe changed the title Use traits for API operations methods [WIP] Use traits for API operations methods Jan 4, 2018
lib/Account.php Outdated
use ApiOperations\Create;
use ApiOperations\Update;
use ApiOperations\Delete;
use ApiOperations\All;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but maybbeeee consider alphabetizing this lists just so that we have stable sorting if new traits are added down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point. Done :)

@brandur-stripe
Copy link
Contributor

brandur-stripe commented Jan 4, 2018

+1 million. This is awesome!

Maybe the one large-ish thing I noticed: when you pull in one of the new traits, it assumes the existence presence of a method on its new host like _retrieve_, meaning that the only class you could ever plausibly mix one of these into is ApiResource. Maybe this isn't that bad, but it seems a little unusual for these traits to have an undocumented, implicit dependency on ApiResource.

(And to be fair, I think the same thing is happening in languages like Ruby already.)

I don't think this will be that much of a problem in practice, and I'm having trouble coming up with specific recommendations. We could try moving _retrieve, _create, etc. to a more general utility class that each trait can just invoke statically, but even then, you'd still need to figure out what to do with classUrl, _staticRequest, etc., which would be hard.

You could check for the presence of methods you're about to use like _retrieve before you use them so that at least you could produce a nicer error message, but that may not be worth the runtime overhead (I tried to see if there's a "included" callback for traits like there is in Ruby, but I guess not).

Anyway, probably not a big deal, but wanted to bring it up for a little discussion.

@ob-stripe ob-stripe changed the title [WIP] Use traits for API operations methods Use traits for API operations methods Jan 4, 2018
@ob-stripe ob-stripe mentioned this pull request Jan 4, 2018
9 tasks
@ob-stripe
Copy link
Contributor Author

Yeah, this is something that crossed my mind as well. Unfortunately PHP does not have a syntax to ensure that a trait is only applied to instances of a certain class, so the only way to enforce this would be to check at runtime.

I will add some PHPDoc to each trait to explicitly flag that these traits can and should only be applied to classes that derive from ApiResource.

@brandur-stripe
Copy link
Contributor

I will add some PHPDoc to each trait to explicitly flag that these traits can and should only be applied to classes that derive from ApiResource.

Cool. Sounds reasonable. Thanks for taking a look!

@ob-stripe ob-stripe merged commit 0b320c6 into integration-v6 Jan 4, 2018
@ob-stripe ob-stripe deleted the ob-use-traits branch January 4, 2018 21:38
ob-stripe added a commit that referenced this pull request Jan 4, 2018
Use traits for API operations methods
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.

2 participants