Skip to content

Conversation

@joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented Jan 1, 2026

Summary

  • Extracts nested directory scanning and app filtering logic into dedicated helper methods, reducing nesting from ~7 levels to 2-3, greatly improving readability/maintainability.
  • Modernizes directory traversal to use scandir() instead of opendir/readdir/closedir pattern.
  • Some additional helpful comments sprinkled throughout

No functional changes - preserves exact same behavior including error handling for TableExistsException during upgrades.

Before:

		foreach (\OC::$APPSROOTS as $app_dir) {
			if ($dir = opendir($app_dir['path'])) {
				while (false !== ($filename = readdir($dir))) {
					if ($filename[0] !== '.' && is_dir($app_dir['path'] . "/$filename")) {
						if (file_exists($app_dir['path'] . "/$filename/appinfo/info.xml")) {
							if ($this->config->getAppValue($filename, 'installed_version') === '') {
								$enabled = $this->appManager->isDefaultEnabled($filename);
								if (($enabled || in_array($filename, $this->appManager->getAlwaysEnabledApps()))
									  && $this->config->getAppValue($filename, 'enabled') !== 'no') {
									if ($softErrors) {
										try {
											$this->installShippedApp($filename, $output);
										} catch (HintException $e) {
											if ($e->getPrevious() instanceof TableExistsException) {
												$errors[$filename] = $e;
												continue;
											}
											throw $e;
										}
									} else {
										$this->installShippedApp($filename, $output);
									}
									$this->config->setAppValue($filename, 'enabled', 'yes');
								}
							}
						}
					}
				}
				closedir($dir);
			}
		}

TODO

  • ...

Checklist

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 <[email protected]>
@joshtrichards joshtrichards added this to the Nextcloud 33 milestone Jan 1, 2026
@joshtrichards joshtrichards requested a review from come-nc January 1, 2026 16:26
@joshtrichards joshtrichards added technical debt ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) labels Jan 1, 2026
@joshtrichards joshtrichards added the 3. to review Waiting for reviews label Jan 1, 2026
@joshtrichards joshtrichards marked this pull request as ready for review January 1, 2026 17:52
@joshtrichards joshtrichards requested a review from a team as a code owner January 1, 2026 17:52
@joshtrichards joshtrichards requested review from leftybournes, salmart-dev and yemkareems and removed request for a team January 1, 2026 17:52
* @param string $path Path to the app directory to scan
* @return string[] App IDs found
*/
private function scanAppsInDirectory(string $path): array {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this shoud be a Generator?

Comment on lines +555 to +556
&& is_dir("$path/$entry")
&& file_exists("$path/$entry/appinfo/info.xml")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the is_dir necessary? The file_exists should be enough I think.

* @param string $appId The app ID to check
* @return bool True if app should be installed
*/
private function shouldInstallShippedApp(string $appId): bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick but I would prefer a test for each case returning early, it feels bad doing more getAppValue calls than necessary.

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

Labels

3. to review Waiting for reviews feature: apps management feature: install and update ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants