Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(updater): hide overwrites from disabled apps list on upgrade #29988

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

MichaIng
Copy link
Member

@MichaIng MichaIng commented Nov 30, 2021

If an incompatible app is enabled manually, it is added to the app_install_overwrite array in config.php. Nextcloud upgrades won't disable any app in this array, but they were still shown on the upgrade page as being disabled.

This commit assures that only apps are shown as "These incompatible apps will be disabled:" which are really disabled, i.e. which are not in the app_install_overwrite array.

@MichaIng MichaIng added enhancement 3. to review Waiting for reviews php Pull requests that update Php code labels Nov 30, 2021
@MichaIng MichaIng added this to the Nextcloud 24 milestone Nov 30, 2021
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from 6867b9e to 9a86907 Compare December 20, 2021 02:41
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from 9a86907 to 9dc53c3 Compare January 3, 2022 23:38
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from 9dc53c3 to 4d909a3 Compare January 20, 2022 15:32
@szaimen
Copy link
Contributor

szaimen commented Jan 27, 2022

Hiding app overwrite on minor updates is fine, imo.
For major updates, app overwrites should be deleted, though, ihmo.

@MichaIng
Copy link
Member Author

MichaIng commented Jan 27, 2022

For major updates, app overwrites should be deleted, though, ihmo.

This actually makes sense. Hmm, this would need to be done as an additional migration step, I guess? The array would then not be updated yet when this code runs, so we'd need to additionally check for a major upgrade and then change this code like:

if ($isMajorUpgrade || !in_array($appInfo['name'], $incompatibleOverwrites)) {

with $isMajorUpgrade being a boolean depending on whether $installedVersion and $currentVersion differ in major version number or not. I can do that once the overwrite deletion step is added.

@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from 4d909a3 to 4744ce8 Compare February 4, 2022 14:36
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
@blizzz blizzz mentioned this pull request Apr 7, 2022
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from 4744ce8 to a5e8fb3 Compare April 12, 2022 00:05
@skjnldsv
Copy link
Member

Hiding app overwrite on minor updates is fine, imo. For major updates, app overwrites should be deleted, though, ihmo.

This is very important, can you raise that today at the server call @szaimen ? :)

@szaimen
Copy link
Contributor

szaimen commented Apr 12, 2022

This is very important, can you raise that today at the server call @szaimen ? :)

I can try :)

@blizzz blizzz mentioned this pull request Apr 13, 2022
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from a5e8fb3 to d9c5524 Compare April 18, 2022 21:01
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from d9c5524 to cf76077 Compare June 1, 2022 20:30
This was referenced Aug 12, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
@@ -188,8 +189,10 @@
$updater->listen('\OC\Updater', 'dbUpgrade', function () use ($output) {
$output->writeln('<info>Updated database</info>');
});
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use ($output) {
$output->writeln('<comment>Disabled incompatible app: ' . $app . '</comment>');
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use ($output, &$incompatibleOverwrites) {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Hooks\EmitterTrait::listen has been marked as deprecated
@@ -188,8 +189,10 @@
$updater->listen('\OC\Updater', 'dbUpgrade', function () use ($output) {
$output->writeln('<info>Updated database</info>');
});
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use ($output) {
$output->writeln('<comment>Disabled incompatible app: ' . $app . '</comment>');
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use ($output, &$incompatibleOverwrites) {

Check notice

Code scanning / Psalm

MissingClosureParamType Note

Parameter $app has no provided type
@@ -162,8 +163,10 @@
$updater->listen('\OC\Updater', 'appUpgrade', function ($app, $version) use ($eventSource, $l) {
$eventSource->send('success', $l->t('Updated "%1$s" to %2$s', [$app, $version]));
});
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use (&$incompatibleApps) {
$incompatibleApps[] = $app;
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use (&$incompatibleApps, &$incompatibleOverwrites) {

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Hooks\EmitterTrait::listen has been marked as deprecated
@@ -162,8 +163,10 @@
$updater->listen('\OC\Updater', 'appUpgrade', function ($app, $version) use ($eventSource, $l) {
$eventSource->send('success', $l->t('Updated "%1$s" to %2$s', [$app, $version]));
});
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use (&$incompatibleApps) {
$incompatibleApps[] = $app;
$updater->listen('\OC\Updater', 'incompatibleAppDisabled', function ($app) use (&$incompatibleApps, &$incompatibleOverwrites) {

Check notice

Code scanning / Psalm

MissingClosureParamType Note

Parameter $app has no provided type
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from 4ecc956 to d98e29a Compare January 26, 2024 15:45
@skjnldsv skjnldsv force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from d98e29a to 2d5a6cf Compare February 27, 2024 16:34
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 27, 2024
@MichaIng MichaIng changed the title Hide overwrites from disabled apps list on upgrade enh(updater): hide overwrites from disabled apps list on upgrade Feb 27, 2024
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from 2d5a6cf to ab6c32e Compare February 27, 2024 17:47
@MichaIng
Copy link
Member Author

Any idea what "Conventional Commits" does not like about the commits text? I added type and scope, so that should be fine: https://www.conventionalcommits.org/en/v1.0.0/

Probably something for the dev documentation, and probably the action can be configured to show the actual tests/rules which have been failed, to give any hint.

@skjnldsv
Copy link
Member

Any idea what "Conventional Commits" does not like about the commits text? I added type and scope, so that should be fine: https://www.conventionalcommits.org/en/v1.0.0/

Enh is not valid. Feat is :)

@MichaIng
Copy link
Member Author

MichaIng commented Feb 27, 2024

Ah, it says "types other than fix: and feat: are allowed", so I thought any type is valid. But it seams that it is one of this array by default: https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional#type-enum

This PR is not really a (new) feature, I'd say, but there is no better matching type indeed 🙂.
EDIT: Jep, that was it, thanks!

@MichaIng MichaIng changed the title enh(updater): hide overwrites from disabled apps list on upgrade feat(updater): hide overwrites from disabled apps list on upgrade Feb 27, 2024
If an incompatible app is enabled manually, it is added to the "app_install_overwrite" array in config.php. Nextcloud upgrades won't disable any app in this array, but they were still shown on the upgrade page and logs as being disabled.

This commit assures that only apps which are really disabled, i.e. which are not in the "app_install_overwrite" array, are shown and logged as disabled during upgrades.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng MichaIng force-pushed the enh/hide-overwrites-from-disabled-apps-list branch from ab6c32e to 7b137dd Compare February 27, 2024 19:20
@skjnldsv skjnldsv merged commit 8df55ef into master Feb 29, 2024
159 of 160 checks passed
@skjnldsv skjnldsv deleted the enh/hide-overwrites-from-disabled-apps-list branch February 29, 2024 10:47
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: apps management feature: install and update php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants