search

Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
80 stars 29 forks source link

Fix get related order IDs method #624

Closed wjrosa closed 1 month ago

wjrosa commented 1 month ago

Fixes p1715949726180409-slack

Description

We have a merchant that sometimes receives a fatal error when listing subscriptions. This is happening because, for their specific case, the related orders retrieval method is not always returning an array as expected, but a serialized array (still unknown reasons). This PR adds a type check for the returned value to avoid the issue.

How to test this PR

This is hard to reproduce since, right now, we are still not sure how this is happening for the merchant. Since code review should be enough.

Product impact

wjrosa commented 1 month ago

Thanks @wjrosa! I just found this PR after responding in the slack thread (p1715949726180409-slack-C7U3Y3VMY) 😅

As I mentioned in the thread, this fix only hides the real problem here and would cause subscriptions to fetch incorrect related order data which will result in much bigger issues.

For this particular case, the issue is caused by our related orders cache being stored as a serialised string of a serialised empty array: s:6:"a:0:{}";, and so when we unserialise it, we end up with the string 'a:0:{}' which is throwing the fatal.

It's unclear how our arrays are being serialised as strings but I suspect it's either the result of a bad migration/import or some other third-party code doing something they shouldn't be doing 😢

The only solution right now is for merchants to clear their related order cache and then regenerate them to make sure they're in the correct format which is very manual.

I think a better way to handle this would be introduce some sort of validation check so rather than ignore this fatal error, what are your thoughts on changing get_related_order_ids_from_cache() to check if it's not an array, and if it's invalid, return '' so that we will generate a new cache.

Thank you for the tip, @mattallan ! Your suggestion makes sense. I have updated the code to match it. But do you think we also need some kind of metric/Tracks so we can monitor when this happens on our side?

mattallan commented 1 month ago

do you think we also need some kind of metric/Tracks so we can monitor when this happens on our side?

We don't typically send errors like this off for monitoring/tracking purposes but we could do something like check if WP_DEBUG is enabled and write to a log or simply error_log 🤔

I'm mainly concerned about those really large stores that could have this issue, so another option could be to write to a log file, but only once per hour using a transient, just so we know the issue is present, but not bombard the logs with potentially hundreds of thousands of logs. This seems like overkill as well, so I'm okay to leave it as it is right now with validating the cache and regenerating it if it's invalid.

wjrosa commented 1 month ago

do you think we also need some kind of metric/Tracks so we can monitor when this happens on our side?

We don't typically send errors like this off for monitoring/tracking purposes but we could do something like check if WP_DEBUG is enabled and write to a log or simply error_log 🤔

I'm mainly concerned about those really large stores that could have this issue, so another option could be to write to a log file, but only once per hour using a transient, just so we know the issue is present, but not bombard the logs with potentially hundreds of thousands of logs. This seems like overkill as well, so I'm okay to leave it as it is right now with validating the cache and regenerating it if it's invalid.

Oh ok, you have a good point. I will leave this as is then! Thank you