Enforce proper return types for methods/functions get_order_from_event_body_intent_id

htdat commented 2 years ago

Description

Based on this convo https://github.com/Automattic/woocommerce-payments/pull/4287#pullrequestreview-984284936

It's more than one time we see fatal errors due to mixed return types.

In this specific case #4287, get_order_from_event_body_intent_id returns a few types:

https://github.com/Automattic/woocommerce-payments/blob/caf5ca2298ea77ad4e2b8f8f03c3f037f0a871c6/includes/class-wc-payments-webhook-processing-service.php#L551-L553

But when a new code is added - ref, it misses checking the proper type before using it.

Acceptance criteria

Update/refactor get_order_from_event_body_intent_id to return less types. Ideally, it would be null | WC_Order, and ensure their consumers process_webhook_payment_intent_failed and process_webhook_payment_intent_succeeded properly check their types before using.
(Maybe) Check other functions to enforce the same. It's worth checking if PHPCS or Psalm can enforce or prevent this issue.

Dev notes

IIRC, Psalm is pretty smart in detecting types and it can prevent this type of issues. Quickly found out this post https://psalm.dev/articles/php-or-type-safety-pick-any-two

Additional context

This may fit into the RFC improving the plugin code quality paJDYF-3eh-p2

htdat commented 2 years ago

cc @vbelolapotkov and @Automattic/transact-architecture as you may want to look at this issue.

jbordonado commented 2 years ago

Have you got any thoughts on this before we move this issue to be prioritized? @Automattic/transact-architecture

Thanks for your input

achyuthajoy commented 2 years ago

Checking for update here - p1657623043816659-slack-C0208C3BXHP

jessy-p commented 2 years ago

On this issue, let us try to list and resolve all functions with too many return types as given in the example boolean|WC_Order|WC_Order_Refund Moving for prioritisation.

RadoslavGeorgiev commented 2 years ago

This issue should only solve the particular instance (get_order_from_event_body_intent_id and boolean|WC_Order|WC_Order_Refund) and list all other instances of this return type. The new issue should be marked as Priority 3.

vbelolapotkov commented 7 months ago

That seems to be part of the payment flow, so routing to your team @deepakpathania for further prioritization.

Automattic / woocommerce-payments