spaze / phpstan-stripe

Stripe SDK extension for PHPStan
MIT License
5 stars 6 forks source link

How can we properly type event data objects? #28

Open msbit opened 1 week ago

msbit commented 1 week ago

In my event handler, I have the following:

$refund = $event->data->object;
if ($refund->status !== Refund::STATUS_FAILED) {
    // The refund has not failed
    return;
}

Without spaze/phpstan-stripe I get the following PHPStan error:

Access to an undefined property Stripe\StripeObject::$object.

With, I get the following instead:

Access to an undefined property Stripe\StripeObject::$status.

This appears to be due to Stripe\Event::$data being marked as Spaze\PHPStan\Stripe\Retrofit\EventData, which has its object attribute defined as a StripeObject.

Would there be the path to allow proper typing without having to add a @var phpdoc here, or is this inherently too dynamic?

anthonyryan1 commented 1 week ago

In my own code I currently use @var hints within specific types, for example:

switch ($event->type) {
    case "charge.succeeded":
    case "charge.updated":
        /** @var Stripe\Charge $charge */
        $charge = $event->data->object;

        // ...
        break;
    case 'customer.deleted':
        /** @var Stripe\Customer $customer */
        $customer = $event->data->object;

        // ...
        break;
}

The problem I see is that event handlers can be anything, and the same endpoint can receive many different types of events.

The best we might be able to do is infer the type within blocks that have checked for $event->type already.

// NOTE: This is not yet implemented, but theoretically possible 
if ($event->type == 'refund.created') {
    // Within this block, we could infer that $event->data->object is a Refund instead of a StripeObject.
    $refund = $event->data->object;

    if ($refund->status !== Refund::STATUS_FAILED) {
        // The refund has not failed
        return;
    }
} else {
    // And here it would be a StripeObject, because we've got no exact value for $event->type
}

If you feel like this is a significant improvement over the @var annotations, let me know and I can explore the feasibility of implementing it.

Either way, the sample code block you provided still will throw a false positive. It'll either need a @var annotation or a comparison against $event->type.

msbit commented 1 week ago

Hey @anthonyryan1 thanks for taking a look!

I have done the same in my code (sprinkling @var as necessary) and it's not too onerous. The question I'd have about the approach there would be how general the type narrowing would be (I've only used very simple MethodsClassReflectionExtension and PropertiesClassReflectionExtension implementations myself), i.e. would any test of Event::type against a known list provide the type narrowing, or just the specific if (...) construct?