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

Update TransactionGateway.php #326

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

Conversation

719media
Copy link
Contributor

@719media 719media commented Jan 18, 2023

The braintree php library is fairly frustrating to use from a typehint/phpdoc standpoint.
The phpdocs are often times just plain wrong.

Just some examples of bad phpdoc:
Not using @throws, putting the @throws into the return (crazy lol), classes in @returns that don't ever get returned, classes missing in @returns...

If the method could throw many/multiple things, I left as @throws Exception as a generic, but if desired you could go deeper.

I'm not sure in braintree is working on a new version that will supersede all of this, so I don't want to take the time to audit everything, but I at least wanted to call it to your attention and see what thoughts you had.

Thanks.

Just some examples of bad phpdoc:
Not using @throws, putting the @throws into the return (crazy lol). If the method could throw many/multiple things, I left as `@throws Exception` as a generic, but if desired you could go deeper
@719media 719media requested a review from a team as a code owner January 18, 2023 22:10
@hollabaq86 hollabaq86 mentioned this pull request Jan 20, 2023
@hollabaq86
Copy link
Contributor

👋 @719media thanks for the PR. For historical context on phpdoc being... not ideal, please see my comment in this issue. We'll take a look at your changes and get them merged in.

For internal tracking, ticket 2237

@719media
Copy link
Contributor Author

Yes, the PR here is just a small sample, there are other problems in this same TransactionGateway class, and many more problems exist in other classes as well.

I don't know what PHP versions you are trying to support, but you could always remove the php 7.3/7.4 support (which PHP themselves have stopped supporting: see https://www.php.net/supported-versions.php), and then you'll have access to union type returns, which will greatly simplify the complexity of tying typedoc documentation together with actual return types (although it would be good practice to still typedoc the @throws).

@cviebrock
Copy link

I came here to post an issue about the incorrect @return of exceptions (specifically for Transaction::find()) ... and see that this has already been reported and fixed in this PR.

This PR has been stale for over a year. What are the chances of it getting looked at and merged? The current SDK is a bit annoying to work with in Phpstorm (and I suspect other IDEs) that think methods might return exceptions, when in fact they might be throwing them.

Thanks!

@saralvasquez
Copy link
Contributor

Sorry for the radio silence on this! Our team had some pretty big shake-ups in the last year or so, and we are just now starting to dig out from under that. We do still have that ticket mentioned above in our backlog, it's just been an issue getting it prioritized. Unfortunately, I don't have a timeline for this, but know that it's on our radar!

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

Successfully merging this pull request may close these issues.

4 participants