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

Added OBJECT_NAME constant to all API objects and added missing object #471

Merged
merged 1 commit into from
May 28, 2018
Merged

Added OBJECT_NAME constant to all API objects and added missing object #471

merged 1 commit into from
May 28, 2018

Conversation

nickdnk
Copy link
Contributor

@nickdnk nickdnk commented May 13, 2018

Hello guys

As in a previous PR that was a bit more of a mess I added the OBJECT_NAME as we discussed once before. This time the PR is cleaner with only this as focus. To refresh: This is to avoid situations where one may look for invoice_item instead of invoiceitem or application_fee_refund instead of fee_refund.

I also added the InvoiceLineItem object, which according to the docs is almost the same as Invoice Item. I am not sure why they both exist, but their object property is different ("invoiceitem" vs "line_item") and one has a few properties the other does not, so there must be a case for both of them. If you don't like this let me know and I'll remove it from the PR.

@brandur-stripe
Copy link
Contributor

Hey @nickdnk, thanks the patch! This is quite similar to the setup that we already have in stripe-ruby, so I think this is a reasonable approach.

Would you mind seeing if you could update the map in Util.php to take advance of these new constants? I believe that a few of our classes are missing them, so this might also help find the places where OBJECT_NAME is forgotten.

I also added the InvoiceLineItem object, which according to the docs is almost the same as Invoice Item. I am not sure why they both exist, but their object property is different ("invoiceitem" vs "line_item") and one has a few properties the other does not, so there must be a case for both of them. If you don't like this let me know and I'll remove it from the PR.

Thanks! I think it's fine to include invoice line item in here.

@nickdnk
Copy link
Contributor Author

nickdnk commented May 14, 2018

I don't understand why the build is failing with autoload 0. It says it cannot find the InvoiceLineItem class, but I've used it the exact same way as all the other classes in the Util map.

@brandur-stripe
Copy link
Contributor

Thanks for the quick fixes @nickdnk!

I don't understand why the build is failing with autoload 0. It says it cannot find the InvoiceLineItem class, but I've used it the exact same way as all the other classes in the Util map.

Check out init.php. If autoload is disabled, there's a big list of files that get required; you'll want to put a reference to the new invoice line item file in there.

@nickdnk
Copy link
Contributor Author

nickdnk commented May 14, 2018

@brandur-stripe That did the trick.

use Stripe\Topup;
use Stripe\Transfer;
use Stripe\TransferReversal;
use Stripe\UsageRecord;
Copy link
Contributor

Choose a reason for hiding this comment

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

@nickdnk Pretty minor, but given the size of this list, what do you think about just using the fully qualified path directly on the lines in the map below so we can avoid two of these big lists?

i.e., Something like this:

\Stripe\Account::OBJECT_NAME => 'Stripe\\Account',

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @nickdnk! Thanks a lot for the contribution. Would you mind addressing @brandur-stripe's comment above? Once that's done, I think we'll be ready to merge :)

@brandur-stripe
Copy link
Contributor

Left one more comment, but awesome work here. Thanks.

@nickdnk
Copy link
Contributor Author

nickdnk commented May 28, 2018

Hey guys
It seems we stalled here. Want me to change anything, @brandur-stripe?

@ob-stripe
Copy link
Contributor

@nickdnk Brandur is not available today (US bank holiday) but I took a look and I think this looks good, modulo Brandur's comment re. using fully qualified paths so that we have one less list to maintain when a new resource is added.

… use these.

Added InvoiceLineItem object to library
@nickdnk
Copy link
Contributor Author

nickdnk commented May 28, 2018

@ob-stripe Done :) Edit: See my comment in InvoiceLineItem.

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

Niiiice! LGTM :)

@ob-stripe ob-stripe merged commit f8b59fe into stripe:master May 28, 2018
@ob-stripe
Copy link
Contributor

Released in 6.7.2. Thanks again @nickdnk :)

@nickdnk
Copy link
Contributor Author

nickdnk commented May 28, 2018

@ob-stripe Cool. Did you remember to check the use traits on InvoiceLineItem? I was not sure which were applicable to that object.

@ob-stripe
Copy link
Contributor

Aw, I missed that. So InvoiceLineItem should not have any traits -- the only operation it supports is listing, and that's done via the lines list object automatically returned when fetching an invoice. I'll push a fix.

@nickdnk
Copy link
Contributor Author

nickdnk commented May 28, 2018

Okay cool. Just wanted to make sure you were on top of it. I don't want to introduce issues :)

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