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

Reject nil keys #639

Closed
wants to merge 1 commit into from
Closed

Reject nil keys #639

wants to merge 1 commit into from

Conversation

jshawl
Copy link

@jshawl jshawl commented Apr 10, 2018

When listing charges, if a user specifies a customer_id, but
customer_id is nil, raise a TypeError instead of silently omitting
the key and returning charges for any user.

When listing charges, if a user specifies a `customer_id`, but
`customer_id` is nil, raise a TypeError instead of silently omitting
the key and returning charges for any user.
@brandur-stripe
Copy link
Contributor

Thanks for the PR!

I'm generally in favor of this sort of safety check, but unfortunately it's a breaking change — it's a fairly common pattern in Ruby (for better or for worse) to pass that was received via something like a form into a set of parameters like these ones, and it's likely that someone out there is relying on the fact that a nil customer_id will fall back to general behavior.

In the spirit of #637, I'm going to mark this with the "future" tag for now as an improvement that we could introduce the next time we do a major version bump.

@ob-stripe
Copy link
Contributor

Hey @jshawl, sorry for the very late reply. We're currently preparing a new major release (#815) and have been debating whether to include your change or not. We've decided not to, as we think this change would be unnecessary painful for many existing users. It's something that would have been nice to have from the start but is hard to add at this point in time.

Thanks again for the contribution and for your understanding!

@ob-stripe ob-stripe closed this Aug 13, 2019
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.

3 participants