stripe / stripe-php

PHP library for the Stripe API.
https://stripe.com
MIT License
3.74k stars 849 forks source link

Undefined Property `object` on event due to magic methods #905

Closed AntoniusGolly closed 4 years ago

AntoniusGolly commented 4 years ago

The stripe docs show examples like this:

$payload = @file_get_contents('php://input');
$event = null;

try {
    $event = \Stripe\Event::constructFrom(
        json_decode($payload, true)
    );
} catch(\UnexpectedValueException $e) {
    // Invalid payload
    http_response_code(400);
    exit();
}

// Handle the event
switch ($event->type) {
    case 'payment_intent.succeeded':
        $paymentIntent = $event->data->object; // contains a \Stripe\PaymentIntent
        // Then define and call a method to handle the successful payment intent.
        // handlePaymentIntentSucceeded($paymentIntent);
        break;
    case 'payment_method.attached':
        $paymentMethod = $event->data->object; // contains a \Stripe\PaymentMethod
        // Then define and call a method to handle the successful attachment of a PaymentMethod.
        // handlePaymentMethodAttached($paymentMethod);
        break;
    // ... handle other event types
    default:
        // Unexpected event type
        http_response_code(400);
        exit();
}

http_response_code(200);

However, our IDE's and code analyzing tools indicate in the line with $event->data->object the error "Access to an undefined property object on StripeObject". This is due to loading the property "object" by magic method (_get) of the StripeObjectclass. This simple annotation would help our IDE's and code analyzing tools to understand that the property will be accessible:

In vendor\stripe\stripe-php\lib\StripeObject.php - line 7

/**
 * Class StripeObject.
 * @property StripeObject $object
 */

Please consider adding the annotation.

ob-stripe commented 4 years ago

Hi @AntoniusGolly, thanks for the detailed report.

The root cause of the issue you're reporting is that the stripe-php library does not know anything about nested hashes. In the case of event objects, the data field is such a nested hash, but all the stripe-php library knows is that it's a StripeObject, the generic class for all hashes.

While adding the annotation you suggested would fix the issue for your specific case, it's not correct in the general case, because not all nested hashes in the API have an object property.

Unfortunately, there is no simple way to fix this. We could define proper classes for every single nested hash, like we do in our libraries for statically typed languages (e.g. stripe-java or stripe-dotnet), but in order to do that we would need to pin stripe-php to a specific API version so the type definitions match the exact shape of objects for that API version. It's something we're considering, but it would be a pretty big paradigm shift for the library and we haven't reached a conclusion yet.

AntoniusGolly commented 4 years ago

@ob-stripe Thanks for looking into this.

but all the stripe-php library knows is that [the data field] is a StripeObject

That's all good, i.m.o. I am also on the same page with you, that there is the generic class and the specific classes and that we can only type-hint for the generic class.

I am talking about the object prop of data if you want so. What I think where the issue results from is that the object is not defined a property within the generic class StripeObject. Though, it is accessible (through the magic method __get()) our infrastructure doesn't know about it prior runtime. I.m.o the fix is quite easyily done through annotations. I may me wrong here, but for me the suggested fix, line 7, did the job. But it doesn't make it to production obviously as we don't check-in /vendor.

I can send you the Phpstan config which throws an error here. As I'm using Phpstan in a pre-commit hook and in the building pipeline this was an issue for me. I thought others would be interested in the overall compatibility of the stripe SDK.

ob-stripe commented 4 years ago

I understand the issue, but my point was that not all nested hashes have an object property, so adding the annotation would be incorrect and misleading in most cases.

For instance, the request field on event objects is also a nested hash, and so is also defined as a StripeObject in the annotations (here), but it does not have an object property.

If we added the annotation you suggested, IDEs like PHPStorm would offer object as an autocompletion suggestion when accessing $event->request->, and tools like PHPStan would think that $event->request->object is valid.

The only way to fix this would be to define actual classes with the correct properties, e.g. EventData for $data and EventRequest for $request.

AntoniusGolly commented 4 years ago

Gotcha. I didn't catch how versatile the StripeObject really was. So, for me the definition of an interface with the above mentioned annotation fixed the issue.

Thank you. This issue can be closed.

ob-stripe commented 4 years ago

Glad you found a workaround, and sorry we don't have a better solution to offer at this time!

ob-stripe commented 4 years ago

As an alternative, for PHPStan specifically, you could define Stripe\StripeObject as an "universal object crate" in your configuration file: https://github.com/phpstan/phpstan#universal-object-crates.

AntoniusGolly commented 4 years ago

I read about this earlier. But Phpstorm is still complaning during pre-commit. Only a warning that can be dismissed but still grabbing alot of attention. I go with the interface declaration. But thanks for the notice!

franco-lombardo commented 11 months ago

What is the preferred solution to this problem at the current date?

Thank you.

calebdw commented 11 months ago

It would be great if this package published its own PHPStan extension for its magic (like the Carbon package does)---then the functionality would be auto-installable (depending on user's config).

At first it could be as simple as adding StripeObject to the universalObjectCratesClasses config so everyone doesn't have to manually do it. But the extension could be expanded to include other magic offered by the package.