FR display a theme while maintenance mode is active

[steelcuts]

Description

This pull request enables our users to utilize a theme while maintenance mode is active, whether it's the currently loaded theme or a different one.

Motivation and Context

I consider this feature a 'nice-to-have,' especially since a recent customer requested it. It ensures that the Web UI always displays branding when available.

I made a change to an 'if' statement in the Router.php file that I believed was unnecessary, but I still marked it as a 'Breaking change' because I'm not a PHP developer, and this file seemed significant to me.

How Has This Been Tested?

• Baremetal oC 10.13.1

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[IljaN]

Please create a changelog entry in https://github.com/owncloud/core/tree/master/changelog/unreleased, see previous examples and README in https://github.com/owncloud/core/tree/master/changelog

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[jvillafanez]

If we're in maintenance mode, we want to load the minimum number of apps possible. I don't think we want to remove that check.

I think it's better something like

if (!\OC::$server->getConfig()->getSystemValue('maintenance', false) && !Util::needUpgrade()) {
  \OC_Apps::loadApps();
} else {
  $themeApp = \OC::$server->getConfig()->getSystemValue('maintenance.theme', '');
  if ($themeApp !== '') {
    \OC_Apps::loadApp($themeApp);
  }
}

We might also want to check if we need to upgrade there.

[steelcuts]

if (!\OC::$server->getConfig()->getSystemValue('maintenance', false) is also present in loadApps() right at the beginning. That's why I removed it and incorporated the theme functionality directly into the loadApps function itself.

[jvillafanez]

ok, that's fine, but there is another problem in the loadApps(). The method is expected to load the type of apps provided, so if I want to load authentication apps, why the theming apps are loaded with them? If I wanted to load also the theming apps I'd requested them later. The method is doing more than what is requested.

This way is clearer that we're in maintenance mode and we just want to load that specific app. Note that the loadApp method (singular) doesn't have the maintenance check.

[steelcuts]

As far as I know, loadApps() will only allow you to load specific apps when you are not in maintenance mode. The theme will only be loaded if maintenance mode is active. However, this is just what I intended to code, and I'm not certain if this is indeed the case.

[jvillafanez]

core/lib/private/legacy/app.php

Line 174 in 28f19ce

public static function loadApp($app, $checkUpgrade = true) {

is what I'm referring to

[steelcuts]

Is it problematic that there isn't a maintenance check? In my commit, "loadApp" will only be called with the theme if maintenance is enabled.

[jvillafanez]

Maintenance mode is whatever we want to define as maintenance mode. Usually, it should be just the base product, but we can extend it to be the base product + the theme, so loading the theme should be fine as long as it doesn't crash.

In any case, the maintenance check is in the "lib/private/Router/Router.php", in the piece you've changed.

[DeepDiver1975]

The theme is not enabled in maintenance mode by intention. Any theme (just like any other app) can cause issues during upgrade (which is the main purpose of the maintenance mode).

From my pov this would be an acceptable change to show the theme in maintenance mode if we are not upgrading.

[jvillafanez]

My main complain is that I think the "load maintenance app" piece is misplaced.

If I call loadApps() and it returns false to signal a failure, why there are apps being loaded? What's the meaning of returning false then?
It gets more confusing if I want to load the authentication apps and what I get is that just a theming app is loaded, which is something I didn't requested.

In addition, placing the check within the loadApps method will affect the webdav and ocs endpoints among others. Do we need to load the theme in those cases? I don't think so.
In general, the performance drop caused by loading the theme will be meaningless. However, we don't have control over what the app is doing, so the worst case would be that the app takes a lot of time loading (maybe with a legit reason). If there is no reason for the webdav endpoint (and maybe others) to load the maintenance theme, let's avoid that.

[sonarcloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 53%)

See analysis details on SonarCloud