App manager stops working when an app needs to upgrade oC

[oparoz]

Env

8.0.3
PHP 5.5
No encryption

Steps to reproduce

1. Enable a few apps until you can't any more

Expected result

You should be able to enable as many apps as you want or be warned at some point that you need to log out and back in before being able to enable more.

Actual result

After you've enabled a few apps, you may find yourself in a situation where you can't install any more app. You get an error message ("Error enabling the app") and the app manager stops working. You get a blank page when visiting the "enabled" or "disabled" tabs.

If you click on "Files", you'll be redirected to the upgrader, so it means an upgrade was required, but instead of being notified, the app manager stops working.

Logs

{"reqId":"1d47b4b9bbcc55553481eb20a3114a58","app":"PHP","message":"preg_split() expects parameter 2 to be string, array given at lib\/private\/app.php#1230","level":0,"time":"2015-05-11T18:52:19+00:00","method":"POST","url":"\/index.php\/settings\/ajax\/enableapp.php"}
{"reqId":"1d47b4b9bbcc55553481eb20a3114a58","app":"PHP","message":"Invalid argument supplied for foreach() at lib\/private\/app.php#1233","level":0,"time":"2015-05-11T18:52:19+00:00","method":"POST","url":"\/index.php\/settings\/ajax\/enableapp.php"}
{"reqId":"59e504cb0234d7c803e27b74c2714aea","app":"PHP","message":"preg_split() expects parameter 2 to be string, array given at lib\/private\/app.php#1230","level":0,"time":"2015-05-11T18:52:19+00:00","method":"GET","url":"\/index.php\/settings\/ajax\/navigationdetect.php?app=files_locking"}
{"reqId":"59e504cb0234d7c803e27b74c2714aea","app":"PHP","message":"Invalid argument supplied for foreach() at lib\/private\/app.php#1233","level":0,"time":"2015-05-11T18:52:19+00:00","method":"GET","url":"\/index.php\/settings\/ajax\/navigationdetect.php?app=files_locking"}

[oparoz]

I think this hinders the UX. What do you think @jancborchardt ?

[jancborchardt]

@oparoz most definitely. Good catch and good analysis.

I also ran into this in the past but was not sure it was because an update is required. Why are updates required in the first place? Shouldn't everything be up to date anyway?

[oparoz]

It's apps updates. If the version changes, oC goes through a mandatory, confusing upgrade, telling the user his just updated oC needs to be updated to the same version (#16272)

[PVince81]

When you click "Update" in the app manager it only downloads the latest code of that app. A DB update might still be needed.

Maybe we should try and run the DB update within the same PHP call. Note that this would also temporarily switch OC into maintenance mode during the DB update, but that could be made transparent to the UI.

The reason why the "update ceremony" is needed is mostly because during a DB upgrade we need to block any apps from accessing OC to avoid unpredictable results while the DB is in a in-between state. So all we can do is try to improve the UX while this happens.

[oparoz]

Maybe we should try and run the DB update within the same PHP call.

I suspect this is already happening, but the UI is not updated to let the user know he's in maintenance mode and needs to logout/login to perform the upgrade.

[PVince81]

Hmmm, possible. And then if the user quickly clicks on another "Install" button, the UI is not responsive.

Maybe the UI should be disabled until the ajax call returns ?

[PVince81]

If the update would cause breakage, might be possible to detect with this approach: #17435

[nickvergessen]

Well that's just another problem of our Ajax vs maintenance mode etc.

[PVince81]

Short term the update button should return a special response that tells the UI to redirect to the update page.

Long term, the update button should attempt the database update directly, with additional failsafe mechanism that re-disable the app in case of "total breakage".

[PVince81]

not only the update button actually, "enable" can have the same effect if the app was previously enabled with an older database.

[PVince81]

Setting to high as this causes bad UX and is quite a common situation, especially after reenabling third party apps after a core update.

[oparoz]

When looking at this, could you make sure updating an app does not reset the theme in the config? It's really annoying and totally unnecessary when updating apps.

[PVince81]

@oparoz can you make a ticket specific for theme disabling ? It should be possible to detect that it's an "app update" vs "core update" and skip theme disabling in that case

[oparoz]

#19271

[PVince81]

thanks