Skip to content

Commit be414c3

Browse files
refactor(installer): reduce deep nesting in installShippedApps()
Extracts nested directory scanning and app filtering logic into dedicated helper methods, reducing nesting from 5-6 levels to 2-3. Modernizes directory traversal to use scandir() instead of opendir/readdir/closedir pattern. No functional changes - preserves exact same behavior including error handling for TableExistsException during upgrades. Relates to #8505 (app management code consolidation) Signed-off-by: Josh <josh.t.richards@gmail.com>
1 parent 7baca42 commit be414c3

File tree

1 file changed

+105
-31
lines changed

1 file changed

+105
-31
lines changed

lib/private/Installer.php

Lines changed: 105 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -485,51 +485,125 @@ public function installAppBundle(Bundle $bundle): void {
485485
/**
486486
* Installs shipped apps
487487
*
488-
* This function installs all apps found in the 'apps' directory that should be enabled by default;
489-
* @param bool $softErrors When updating we ignore errors and simply log them, better to have a
490-
* working ownCloud at the end instead of an aborted update.
491-
* @return array Array of error messages (appid => Exception)
488+
* Scans all configured app directories and installs apps that meet the criteria:
489+
* - Not already installed
490+
* - Not explicitly disabled
491+
* - Marked as default-enabled or always-enabled in core/shipped.json
492+
*
493+
* @param bool $softErrors When true (during upgrades), TableExistsException errors are
494+
* captured and returned rather than thrown, allowing the upgrade
495+
* to continue. When false (during fresh install), all errors halt
496+
* the installation process.
497+
* @param IOutput|null $output Optional output handler for logging installation progress
498+
* @return array<string, \Exception> Array of error messages mapping app ID to Exception.
499+
* Only populated when $softErrors is true.
492500
*/
493501
public function installShippedApps(bool $softErrors = false, ?IOutput $output = null): array {
494502
if ($output instanceof IOutput) {
495503
$output->debug('Installing shipped apps');
496504
}
505+
497506
$errors = [];
498-
foreach (\OC::$APPSROOTS as $app_dir) {
499-
if ($dir = opendir($app_dir['path'])) {
500-
while (false !== ($filename = readdir($dir))) {
501-
if ($filename[0] !== '.' && is_dir($app_dir['path'] . "/$filename")) {
502-
if (file_exists($app_dir['path'] . "/$filename/appinfo/info.xml")) {
503-
if ($this->config->getAppValue($filename, 'installed_version') === '') {
504-
$enabled = $this->appManager->isDefaultEnabled($filename);
505-
if (($enabled || in_array($filename, $this->appManager->getAlwaysEnabledApps()))
506-
&& $this->config->getAppValue($filename, 'enabled') !== 'no') {
507-
if ($softErrors) {
508-
try {
509-
$this->installShippedApp($filename, $output);
510-
} catch (HintException $e) {
511-
if ($e->getPrevious() instanceof TableExistsException) {
512-
$errors[$filename] = $e;
513-
continue;
514-
}
515-
throw $e;
516-
}
517-
} else {
518-
$this->installShippedApp($filename, $output);
519-
}
520-
$this->config->setAppValue($filename, 'enabled', 'yes');
521-
}
522-
}
507+
508+
// Iterate through all configured app directories
509+
foreach (\OC::$APPSROOTS as $appRoot) {
510+
foreach ($this->scanAppsInDirectory($appRoot['path']) as $appId) {
511+
if (!$this->shouldInstallShippedApp($appId)) {
512+
continue;
513+
}
514+
515+
// Fresh install: fail immediately on any errors
516+
if (!$softErrors) {
517+
$this->installShippedApp($appId, $output);
518+
} else {
519+
// During upgrades: capture TableExistsException (duplicate database table errors)
520+
// to allow the upgrade to continue even if some app tables already exist
521+
try {
522+
$this->installShippedApp($appId, $output);
523+
} catch (HintException $e) {
524+
if ($e->getPrevious() instanceof TableExistsException) {
525+
$errors[$appId] = $e;
526+
continue;
523527
}
528+
throw $e;
524529
}
525530
}
526-
closedir($dir);
531+
532+
$this->config->setAppValue($appId, 'enabled', 'yes');
527533
}
528534
}
529-
535+
530536
return $errors;
531537
}
532538

539+
/**
540+
* Scan a directory for valid apps (directories with appinfo/info.xml)
541+
*
542+
* @param string $path Path to the app directory to scan
543+
* @return string[] App IDs found
544+
*/
545+
private function scanAppsInDirectory(string $path): array {
546+
if (!is_dir($path)) {
547+
return [];
548+
}
549+
550+
$apps = [];
551+
foreach (scandir($path) as $entry) {
552+
/* valid app? */
553+
if (
554+
$entry[0] !== '.'
555+
&& is_dir("$path/$entry")
556+
&& file_exists("$path/$entry/appinfo/info.xml")
557+
) {
558+
$apps[] = $entry;
559+
}
560+
}
561+
return $apps;
562+
}
563+
564+
/**
565+
* Check if a shipped app should be installed
566+
*
567+
* @param string $appId The app ID to check
568+
* @return bool True if app should be installed
569+
*/
570+
private function shouldInstallShippedApp(string $appId): bool {
571+
// Skip if already installed or explicitly disabled
572+
$isAlreadyInstalled = $this->config->getAppValue($appId, 'installed_version') !== '';
573+
$isExplicitlyDisabled = $this->config->getAppValue($appId, 'enabled') === 'no';
574+
575+
if ($isAlreadyInstalled || $isExplicitlyDisabled) {
576+
return false;
577+
}
578+
579+
// Install if default-enabled or always-enabled
580+
$isDefaultEnabled = $this->appManager->isDefaultEnabled($appId);
581+
$isAlwaysEnabled = in_array($appId, $this->appManager->getAlwaysEnabledApps(), true);
582+
583+
return $isDefaultEnabled || $isAlwaysEnabled;
584+
}
585+
586+
/**
587+
* Execute the final installation steps for an app
588+
*
589+
* Performs all necessary setup after app files are in place:
590+
* - Registers autoloading
591+
* - Runs database migrations
592+
* - Executes repair steps (pre/post migration and install)
593+
* - Registers background jobs
594+
* - Runs legacy install.php script (if present, deprecated)
595+
* - Sets installed version and enabled state in config
596+
* - Registers remote/public handlers
597+
* - Sets app types
598+
*
599+
* Used by both installApp() and installShippedApp() as their final step.
600+
*
601+
* @param string $appPath Full filesystem path to the app directory
602+
* @param array $info Parsed app info from info.xml
603+
* @param IOutput|null $output Optional output handler for logging progress
604+
* @param string $enabled Initial enabled state: 'yes', 'no', or JSON-encoded group list
605+
* @return string The app ID
606+
*/
533607
private function installAppLastSteps(string $appPath, array $info, ?IOutput $output = null, string $enabled = 'no'): string {
534608
\OC_App::registerAutoloading($info['id'], $appPath);
535609

0 commit comments

Comments
 (0)