-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
enable app theme with default_enable
during installation/update
#27819
Conversation
…ize app theme during installation or update wizard
@@ -637,6 +637,23 @@ public static function getAppVersionByPath($path) { | |||
return isset($appData['version']) ? $appData['version'] : ''; | |||
} | |||
|
|||
/** | |||
* @return false|string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPDoc ?
$apps = self::getAllApps(); | ||
$parser = new InfoParser(); | ||
foreach ($apps as $app) { | ||
$info = $parser->parse(self::getAppPath($app) . '/appinfo/info.xml'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fine... this will need a good refactoring at some point
👍 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry - but this has a bad performance impact - parsing all app info.xml on each and every request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only when "installed" is false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically the setup page
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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 isdefault_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
Checklist: