Skip to content

fix(appinfo): properly type info parser and app manager for appinfo.xml types #60066

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.

Sign up for GitHub

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

Open
susnux wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(appinfo): properly type info parser and app manager for appinfo.xml types #60066

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ac75ea
fix(AppManager): properly type AppInfo type based on XSD
susnux May 2, 2026
255e828
fix(InfoParser): properly handle collaboration plugins
susnux May 2, 2026
70e1607
fix(app-info-shipped): add `remote` as its still available in code base
susnux May 2, 2026
e3044a3
test(appinfo): add test cases for list parsing of appinfo
susnux May 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/updatenotification/lib/Controller/APIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public function getAppList(string $newVersion): DataResponse {
* @return UpdateNotificationApp
*/
protected function getAppDetails(string $appId): array {
$app = $this->appManager->getAppInfo($appId, false, $this->language);
$app = $this->appManager->getAppInfo($appId, lang: $this->language ?? 'en');
$name = $app['name'] ?? $appId;
return [
'appId' => $appId,
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
'OCP\\AppFramework\\Services\\InitialStateProvider' => $baseDir . '/lib/public/AppFramework/Services/InitialStateProvider.php',
'OCP\\AppFramework\\Utility\\IControllerMethodReflector' => $baseDir . '/lib/public/AppFramework/Utility/IControllerMethodReflector.php',
'OCP\\AppFramework\\Utility\\ITimeFactory' => $baseDir . '/lib/public/AppFramework/Utility/ITimeFactory.php',
'OCP\\App\\AppInfoDefinition' => $baseDir . '/lib/public/App/AppInfoDefinition.php',
'OCP\\App\\AppPathNotFoundException' => $baseDir . '/lib/public/App/AppPathNotFoundException.php',
'OCP\\App\\Events\\AppDisableEvent' => $baseDir . '/lib/public/App/Events/AppDisableEvent.php',
'OCP\\App\\Events\\AppEnableEvent' => $baseDir . '/lib/public/App/Events/AppEnableEvent.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\AppFramework\\Services\\InitialStateProvider' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Services/InitialStateProvider.php',
'OCP\\AppFramework\\Utility\\IControllerMethodReflector' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Utility/IControllerMethodReflector.php',
'OCP\\AppFramework\\Utility\\ITimeFactory' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Utility/ITimeFactory.php',
'OCP\\App\\AppInfoDefinition' => __DIR__ . '/../../..' . '/lib/public/App/AppInfoDefinition.php',
'OCP\\App\\AppPathNotFoundException' => __DIR__ . '/../../..' . '/lib/public/App/AppPathNotFoundException.php',
'OCP\\App\\Events\\AppDisableEvent' => __DIR__ . '/../../..' . '/lib/public/App/Events/AppDisableEvent.php',
'OCP\\App\\Events\\AppEnableEvent' => __DIR__ . '/../../..' . '/lib/public/App/Events/AppEnableEvent.php',
Expand Down
15 changes: 6 additions & 9 deletions lib/private/App/AppManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
use OCP\Settings\IManager as ISettingsManager;
use Psr\Log\LoggerInterface;

/**
* @psalm-import-type AppInfoDefinition from \OCP\App\AppInfoDefinition
*/
class AppManager implements IAppManager {
/**
* Apps with these types can not be enabled for certain groups only
Expand All @@ -63,7 +66,7 @@ class AppManager implements IAppManager {
private array $alwaysEnabled = [];
private array $defaultEnabled = [];

/** @var array */
/** @var array<string, AppInfoDefinition|null> */
private array $appInfos = [];

/** @var array */
Expand Down Expand Up @@ -838,14 +841,8 @@ public function getAppsNeedingUpgrade($version) {
return $appsToUpgrade;
}

/**
* Returns the app information from "appinfo/info.xml".
*
* @param string|null $lang
* @return array|null app info
*/
#[\Override]
public function getAppInfo(string $appId, bool $path = false, $lang = null) {
public function getAppInfo(string $appId, bool $path = false, $lang = null): ?array {
if ($path) {
throw new \InvalidArgumentException('Calling IAppManager::getAppInfo() with a path is no longer supported. Please call IAppManager::getAppInfoByPath() instead and verify that the path is good before calling.');
}
Expand Down Expand Up @@ -877,7 +874,7 @@ public function getAppInfoByPath(string $path, ?string $lang = null): ?array {
$parser = new InfoParser($this->memCacheFactory->createLocal('core.appinfo'));
$data = $parser->parse($path);

if (is_array($data)) {
if ($lang !== null && is_array($data)) {
$data = $parser->applyL10N($data, $lang);
}

Expand Down
46 changes: 33 additions & 13 deletions lib/private/App/InfoParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
use OCP\ICache;
use function simplexml_load_string;

/**
* @psalm-import-type AppInfoLocalizedEntry from \OCP\App\AppInfoDefinition
* @psalm-import-type AppInfoXmlDefinition from \OCP\App\AppInfoDefinition
* @psalm-import-type AppInfoDefinition from \OCP\App\AppInfoDefinition
*/
class InfoParser {
/**
* @param ICache|null $cache
Expand All @@ -21,15 +26,15 @@ public function __construct(

/**
* @param string $file the xml file to be loaded
* @return null|array where null is an indicator for an error
* @return AppInfoXmlDefinition|null - The parsed app info or null if an error occurred
*/
public function parse(string $file): ?array {
if (!file_exists($file)) {
return null;
}

$fileCacheKey = $file . filemtime($file);
if ($this->cache !== null) {
$fileCacheKey = $file . filemtime($file);
if ($cachedValue = $this->cache->get($fileCacheKey)) {
return json_decode($cachedValue, true);
}
Expand All @@ -42,14 +47,14 @@ public function parse(string $file): ?array {
libxml_clear_errors();
return null;
}
$array = $this->xmlToArray($xml);

$array = $this->xmlToArray($xml);
if (is_string($array)) {
return null;
}

if (!array_key_exists('info', $array)) {
$array['info'] = [];
if (!array_key_exists('description', $array)) {
$array['description'] = '';
Comment on lines +56 to +57
Copy link
Copy Markdown
Contributor Author

@susnux susnux May 5, 2026

Choose a reason for hiding this comment

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

@provokateurin there is no info inside the info, the <info> is the outer element - thats just always empty and unused.

The new code is just moved from applyL10N because there it made no sense but file sanitizing belongs to the parsing.

}
if (!array_key_exists('remote', $array)) {
$array['remote'] = [];
Expand Down Expand Up @@ -211,6 +216,9 @@ public function parse(string $file): ?array {
$array['category'] = [$array['category']];
}

/**
* @var AppInfoXmlDefinition $array
*/
Comment on lines +219 to +221
Copy link
Copy Markdown
Member

@provokateurin provokateurin May 6, 2026

Choose a reason for hiding this comment

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

This @var should not be there. To do it correctly you'd add all the checks that are needed to ensure the data matches the shape.

While I would like to see this, it's probably a bit overkill for this PR 🙈

if ($this->cache !== null) {
$this->cache->set($fileCacheKey, json_encode($array));
}
Expand Down Expand Up @@ -283,27 +291,39 @@ public function xmlToArray(\SimpleXMLElement $xml): array|string {

/**
* Select the appropriate l10n version for fields name, summary and description
*
* @param AppInfoXmlDefinition $data
* @return AppInfoDefinition
*/
public function applyL10N(array $data, ?string $lang = null): array {
if ($lang !== '' && $lang !== null) {
if (isset($data['name']) && is_array($data['name'])) {
public function applyL10N(array $data, string $lang): array {
if (isset($data['name'])) {
if (is_array($data['name'])) {
$data['name'] = $this->findBestL10NOption($data['name'], $lang);
}
if (isset($data['summary']) && is_array($data['summary'])) {
$data['name'] = trim($data['name']);
}
if (isset($data['summary'])) {
if (is_array($data['summary'])) {
$data['summary'] = $this->findBestL10NOption($data['summary'], $lang);
}
if (isset($data['description']) && is_array($data['description'])) {
$data['summary'] = trim($data['summary']);
}
if (isset($data['description'])) {
if (is_array($data['description'])) {
$data['description'] = trim($this->findBestL10NOption($data['description'], $lang));
}
} elseif (isset($data['description']) && is_string($data['description'])) {
$data['description'] = trim($data['description']);
} else {
$data['description'] = '';
}

/** @var AppInfoDefinition $data */
Copy link
Copy Markdown
Member

@provokateurin provokateurin May 6, 2026

Choose a reason for hiding this comment

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

Same here

return $data;
}

/**
* @param AppInfoLocalizedEntry|list<string|AppInfoLocalizedEntry> $options - The available l10n options for a field
Copy link
Copy Markdown
Member

@provokateurin provokateurin May 6, 2026

Choose a reason for hiding this comment

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

Why string? I would have expected the type to be string|AppInfoLocalizedEntry|list<AppInfoLocalizedEntry> or something similar 🤔

* @param string $lang - The desired language code
* @return string - The best matching l10n option for the given language
*/
protected function findBestL10NOption(array $options, string $lang): string {
// only a single option
if (isset($options['@value'])) {
Expand Down
Loading
⚡