-
Notifications
You must be signed in to change notification settings - Fork 681
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
Stripe notice about undefined property (quantity) with metered prices #1254
Comments
I was coming here to report the same thing. :) I see it show up in my unit tests as |
Thanks all. @mfauveau I think this is in fact a bug in Cashier. I've sent in that fix here: #1255 @adamthehutt always feel free to send in PR's with those fixes. Saves a bit of time ;) |
@driesvints maybe I'm missing something but for me, the notice is triggered by https://github.com/laravel/cashier-stripe/blob/13.x/src/SubscriptionBuilder.php#L251 This is why I was saying this might be a Stripe bug. https://stripe.com/docs/api/subscriptions/create describes quantity as optional but:
|
@mfauveau it's thrown in cashier-stripe/src/Subscription.php Line 853 in 1a80ccb
|
@driesvints I think it's a different bug then... This line is not called in my case (I've tried). This is what my code does:
|
@taylorotwell @driesvints note that @adamthehutt is updating an existing subscription. My code creates a subscription from scratch with the subscription builder. In that particular situation, the notice will still show up. |
I'm sorry @mfauveau but we have tests for the exact code you're using. If you really believe there's a bug please provide a failing test for this library. |
I get that @driesvints but look:
|
Here's for example a test that covers the behavior: https://github.com/laravel/cashier-stripe/blob/13.x/tests/Feature/MeteredBillingTest.php#L208-L210 I'm wondering what |
I'm guessing you posted that before seeing my previous comment? It returns an array of prices:
|
Hmm no. I'm very sorry @mfauveau but I just don't see an issue. We have tests which very the code is working. If you really think there's a bug please provide a PR. |
This #1254 (comment) is a fresh clone of a cashier running the MeteredBillingTest and showing the error @driesvints. Not sure what you don't see here. But that's all good. It's just a notice after all. |
@mfauveau we don't get those notices in our test suite: https://github.com/laravel/cashier-stripe/runs/3720820913 No worries, I'm also just a bit confused here 😅 |
I had a feeling you were referring to the test suite. It's confusing indeed. I wonder why a fresh clone is throwing that notice but not the test suite? I'm guessing the test suite doesn't silence notices. Upon a quick inspection, it looks like the test suite pulls a different version of stripe-php (v7.97.0 vs 7.39). I'm gonna try to upgrade and run the test again. |
Never mind, still throwing notices with v7.97.0 🤷♂️ |
This fixes a Stripe Notice (Stripe Notice: Undefined property of Stripe\SubscriptionItem instance: quantity) when using the SubscriptionBuilder with metered prices. See my comments on laravel#1254.
This fixes a Stripe Notice (Stripe Notice: Undefined property of Stripe\SubscriptionItem instance: quantity) when using the SubscriptionBuilder with metered prices. See my comments on #1254.
Description:
When adding a metered price to an existing subscription, Stripe is emitting an error notice about an undefined property (quantity) on the SubscriptionItem object.
Here's the problem line.
Steps To Reproduce:
Create a subscription, then:
Null is (correctly) being passed as the quantity to the addPrice method, but then when Cashier tries to create a record of the subscription item it references the Stripe object's quantity property, which is undefined.
I believe a fix is as simple as:
I can produce a PR if you'd like.
The text was updated successfully, but these errors were encountered: