search

braintree / braintree_php

Braintree PHP library
https://developer.paypal.com/braintree/docs/start/overview
MIT License
545 stars 225 forks source link

Update TransactionGateway.php #326

Open 719media opened 1 year ago

719media commented 1 year ago

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.

hollabaq86 commented 1 year ago

👋 @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 commented 1 year ago

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 commented 1 month ago

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 commented 1 month ago

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!