fix(appstore): Ensure returned apps from AppStore are valid

[solracsf]

• Resolves: [Bug]: Upgrade from 29.0.4 to 29.0.6 fails with TypeError in AppFetcher.php #47844

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[come-nc]

This does not fix the linked TypeError from what I understand.
The problem of the TypeError is more that $response['data'] was not set I suppose?
Any idea what the response looked like? Because if it was empty it would return early line 57 already.

[solracsf]

My analysis was that some elements of $response['data'] were null or not properly initialized. Specifically, when some apps didn’t have matching releases (or, in this case, were not available), $response['data'][$dataKey] = [] is set. However, there may be cases where $response['data'] elements are null or invalid, causing array_filter() to throw an error.

By adding the is_array() check, we should prevent non-array or null values from being passed to array_filter(), which avoids the TypeError. But maybe i'm not seeing it the right way... 👀

[provokateurin]

No @come-nc's analysis is right:

<?php
array_filter(null);

will give you

PHP Fatal error:  Uncaught TypeError: array_filter(): Argument #1 ($array) must be of type array, null given in /home/jld3103/.config/JetBrains/IntelliJIdea2024.2/scratches/scratch_4.php:2
Stack trace:
#0 /home/jld3103/.config/JetBrains/IntelliJIdea2024.2/scratches/scratch_4.php(2): array_filter()
#1 {main}
  thrown in /home/jld3103/.config/JetBrains/IntelliJIdea2024.2/scratches/scratch_4.php on line 2

Process finished with exit code 255

Which is exactly the same error message. The error message clearly mentions the method and the argument.

Running

<?php
array_filter([null]);

results in no error.

[provokateurin]

The if (empty($response)) { check further up should be extended to be if (empty($response) || $response['data'] === null) {.

[provokateurin]

However, there may be cases where $response['data'] elements are null or invalid, causing array_filter() to throw an error.

If no callback is given to array_filter it will remove all falsy values which includes null, so null is not an invalid element in the given array.

[solracsf]

Thanks for your inputs, six eyes see better than two 🤣
PR amended.

[provokateurin]

I wonder if we should have a warning log for the return case to inform the admin that the appstore returned a faulty response? Otherwise it is just silent and they can't know why.

[solracsf]

Agree.

Maybe something like:

$this->logger->warning('Response from appstore is invalid, apps could not be retrieved. Try again later.', ['app' => 'appstoreFetcher']);

This should inform admin that response was not valid, but also this could be temporary and it can be retried (in case appstore is down for maintenance, overloaded...).

[provokateurin]

Sounds good, but I would either omit the app parameter or set it to core.

[solracsf]

All the other warnings have it, that's why I've included it. 🤔

[provokateurin]

Ah ok, then it's good. Can you add it to the PR?

[solracsf]

Done ✅

[provokateurin]

/backport to stable30

[provokateurin]

/backport to stable29

[provokateurin]

/backport to stable28