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

enable app theme with `default_enable` during installation/update #27819

Merged
merged 1 commit into from May 12, 2017

Conversation

Projects
None yet
3 participants
@phisch
Contributor

phisch commented May 5, 2017

Description

Apps are disabled during the installation and update wizard. Thats why we have to find and enable the app theme manually. This PR provides functionality to find the first app that has the type theme and is default_enable. We can't use existing functions for that, because they always check the database which is not setup at this point yet.

Related Issue

https://github.com/owncloud/enterprise/issues/1956

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
provide function to find the first default enabled app theme, initial…
…ize app theme during installation or update wizard

@phisch phisch self-assigned this May 5, 2017

@phisch phisch requested review from IljaN, PVince81 and DeepDiver1975 May 5, 2017

@phisch phisch added the 3 - To Review label May 5, 2017

@phisch phisch added this to the 10.0.1 milestone May 5, 2017

@@ -637,6 +637,23 @@ public static function getAppVersionByPath($path) {
return isset($appData['version']) ? $appData['version'] : '';
}
/**
* @return false|string

This comment has been minimized.

@PVince81

PVince81 May 8, 2017

Member

PHPDoc ?

@PVince81

PVince81 May 8, 2017

Member

PHPDoc ?

$apps = self::getAllApps();
$parser = new InfoParser();
foreach ($apps as $app) {
$info = $parser->parse(self::getAppPath($app) . '/appinfo/info.xml');

This comment has been minimized.

@PVince81

PVince81 May 8, 2017

Member

Hmm, I thought there were other functions that would load app metadata already ? Not too happy to have yet another /appinfo/info.xml path hard-coded here.
Did you find the code that enables all default apps ?

@PVince81

PVince81 May 8, 2017

Member

Hmm, I thought there were other functions that would load app metadata already ? Not too happy to have yet another /appinfo/info.xml path hard-coded here.
Did you find the code that enables all default apps ?

This comment has been minimized.

@phisch

phisch May 8, 2017

Contributor

Yes, there is another function that loads metadata, but in some cases it tries to connect to the database, which of course fails in this case. I'm not too happy about it either.
The code that enables all default apps: \OC\Installer::installShippedApps

@phisch

phisch May 8, 2017

Contributor

Yes, there is another function that loads metadata, but in some cases it tries to connect to the database, which of course fails in this case. I'm not too happy about it either.
The code that enables all default apps: \OC\Installer::installShippedApps

This comment has been minimized.

@PVince81

PVince81 May 9, 2017

Member

Ok fine... this will need a good refactoring at some point

@PVince81

PVince81 May 9, 2017

Member

Ok fine... this will need a good refactoring at some point

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member

👍 code looks good, I assume you've tested this thoroughly

Member

PVince81 commented May 9, 2017

👍 code looks good, I assume you've tested this thoroughly

@@ -842,6 +842,15 @@ public static function handleRequest() {
$setupHelper = new OC\Setup(\OC::$server->getConfig(), \OC::$server->getIniWrapper(),
\OC::$server->getL10N('lib'), new \OC_Defaults(), \OC::$server->getLogger(),
\OC::$server->getSecureRandom());
$defaultEnabledAppTheme = \OC_App::getDefaultEnabledAppTheme();

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 12, 2017

Member

sorry - but this has a bad performance impact - parsing all app info.xml on each and every request?

@DeepDiver1975

DeepDiver1975 May 12, 2017

Member

sorry - but this has a bad performance impact - parsing all app info.xml on each and every request?

This comment has been minimized.

@PVince81

PVince81 May 12, 2017

Member

this is only when "installed" is false

@PVince81

PVince81 May 12, 2017

Member

this is only when "installed" is false

This comment has been minimized.

@PVince81

PVince81 May 12, 2017

Member

basically the setup page

@PVince81

PVince81 May 12, 2017

Member

basically the setup page

@DeepDiver1975 DeepDiver1975 merged commit 9c30ac2 into master May 12, 2017

4 checks passed

Scrutinizer 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@DeepDiver1975 DeepDiver1975 deleted the load_default_enabled_theme_during_installation branch May 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment