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

Fix crashes when "id" is not a string. #1602

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Nov 6, 2023

Summary

This PR makes

$obj = StripeObject::constructFrom(['inner' => ['id' => ['foo' => 'bar']]]);

no longer crash. Sometimes "id" doesn't mean "identifier", sometimes it means "indonesia".

Detail

Right now, StripeObject::__construct is called from two important paths.

  1. It's called from StripeObject::constructFrom, like this:
    public static function constructFrom($values, $opts = null)
    {
        $obj = new static(isset($values['id']) ? $values['id'] : null);
  1. It's called from ApiOperations::Retrieve::retrieve, like this:
    /*
     * @param array|string $id the ID of the API resource to retrieve,
     *     or an options array containing an `id` key
     */
    public static function retrieve($id, $opts = null)
    {
        ...
        $instance = new static($id, $opts);
        $instance->refresh();

This is troublesome, because in the first case, $id is "value of an id property that we are deserializing, e.g. it came back on a response", but in the second case, $id means "stuff that the user passed into retrieve."

In both cases, $id can be an array. In constructFrom it can be an array if "id" means "indonesia" and isn't an identifier. Via retrieve it can be an array if the user wanted to send some extra query parameters along with their call to retrieve.

Right now $obj = StripeObject::constructFrom(['inner' => ['id' => ['foo' => 'bar']]]); is crashing because we are always assuming the second case, i.e. the library sees ['foo' => 'bar'] and crashes: "hey wait a minute! You're telling me to send ?foo=bar on a retrieve, but ['foo' => 'bar'] doesn't have an id field and I need to know id to do a retrieve!"

The more disciplined change would be to take this logic completely out of the StripeObject constructor. It should not be shared. That needs to happen in a major, though.

This less disciplined change is just to allow this code path to proceed without id being present. It's safe. This code path just would have crashed before.

@richardm-stripe richardm-stripe requested review from a team and anniel-stripe and removed request for a team November 6, 2023 23:04
@richardm-stripe richardm-stripe marked this pull request as ready for review November 6, 2023 23:04
Co-authored-by: pakrym-stripe <99349468+pakrym-stripe@users.noreply.github.com>
@richardm-stripe richardm-stripe merged commit 87a44c0 into master Nov 6, 2023
22 checks passed
@richardm-stripe richardm-stripe deleted the richardm-fix-normalizing-non-string-ids branch November 6, 2023 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants