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

Services bugfix: convert nested null params to empty strings #933

Merged
merged 7 commits into from
May 15, 2020

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented May 15, 2020

With PHP services, unlike the singlon API, sending null on a method's params is translated to an empty string when it is sent to the API, to cause the API to unset that field. This was only working properly for top-level parameters. This PR causes the substitution to take place recursively, and handle null properly inside arrays (both associative and non-associative).

static::assertTrue(null !== $result['foo']['bar']);
static::assertTrue(1 === $result['foo']['baz']);

$result = \Stripe\Service\AbstractService::formatParams(['foo' => ["bar", null, null, "baz"]]);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about passing

[
  'email' => null,
  'invoice_settings' => [
    'default_payment_method' => 'null',
  ],
]

As in, this method didn't seem recurring or able to handle non top-level params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite follow -- this PR adds a recursive call https://github.com/stripe/stripe-php/pull/933/files#diff-d6b577df0148860a08e6dfb3025bd0d0R52

and this test case covers non top-level params -- everything is nested one level under "foo".

@richardm-stripe
Copy link
Contributor Author

ptal @remi-stripe

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

Approving though I'd feel better if we waited for @ob-stripe's +1 to be safe

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.

LGTM, though I'd prefer not to make the method public. You can use \ReflectionMethod to invoke protected or private methods in tests (e.g. https://github.com/stripe/stripe-php/blob/master/tests/Stripe/StripeObjectTest.php#L28-L29). Feel free to leave as is though.

@richardm-stripe
Copy link
Contributor Author

Thanks! Made private
ptal @ob-stripe

@ob-stripe
Copy link
Contributor

Formatting error is causing CI to fail, ptal @richardm-stripe

@richardm-stripe
Copy link
Contributor Author

richardm-stripe commented May 15, 2020 via email

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.

LGTM

@richardm-stripe richardm-stripe merged commit 7f582fd into master May 15, 2020
@richardm-stripe richardm-stripe deleted the richardm-recursive-formatparams branch May 15, 2020 18:28
@richardm-stripe richardm-stripe changed the title Services bugfix: handle null properly on arrays Services bugfix: convert nested null params to empty strings May 15, 2020
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.

None yet

4 participants