Guard condition in `delete_temp_backup()` shouldn't return early

[johnbillion]

In the delete_temp_backup() function there is a guard condition which triggers an early return if the arguments for any of the temp backups are empty (​ref).

This prevents further iteration of the array of temp backups. In theory this prevents the deletion of a valid backup that exists in in a subsequent element in the array.

I think the return false should be replaced with continue.

[amitraj2203]

Patch file

[amitraj2203]

@johnbillion I have submitted the patch for the same.

[jamieblomerus]

During today's bug scrub, it was highlighted that the patch needs testing, and guidance was sought for from people who know more about this. The discussion focused on whether true or false should be returned when deletion is skipped, considering the potential impact on other parts of the code. It was noted that low-level APIs typically return true for successful deletion or when the item to be deleted does not exist.

Suggestions for handling broken backups included using unset(), adding an error, or utilizing wp_trigger_error() behind a WP_DEBUG flag.

We felt like a more extensive discussion is needed.

[nhrrob]

We are very close to 6.6 RC1.
Punting to the next milestone.

[stoyangeorgiev]

Moving this one to a Future Release, since additional discussion is needed in terms of handling this one properly. Also testing would be needed for this one.

[SirLouen]

@johnbillion can you provide with the steps to set up my environment to reproduce this?

I always struggle to test and debug upgraded-related topics.
But looking for coincidences in the code, I can only see two calls:

1. ​In the automatic upgrader, the array is pretty much already defined, and doesn't make sense to iterate it given that it only has one element

2. ​As a hook call in the same file with no parameters, so in this case it will get the property temp_restores which happens to be a part of $options['hook_extra'] which seems to be also a single unit.

So I cannot see where and how it can iterate through multiple backups; hence I cannot reproduce the intent of this report. I know it's a little old report (1 year), but can you remember how did you identify this potential issue back in the day? For helping test and peer review this kind of requests, ​a Testing Use Case is gold