Skip to content

getAppInfo has to return array - #8503

Closed
blizzz wants to merge 1 commit into
masterfrom
fix-appinfo-return-val
Closed

getAppInfo has to return array#8503
blizzz wants to merge 1 commit into
masterfrom
fix-appinfo-return-val

Conversation

@blizzz

Copy link
Copy Markdown
Member

null causes exceptions down the road (e.g. when you uncleanly remove an app only from the file system) in AppManager::getIncompatibleApps()::405, the call to isAppCompatible which expects the second parameter to be an array. Restores the same behaviour as before. Without this, you are greeted with an unstyled error page.

@blizzzblizzz added this to the Nextcloud 14 milestone Feb 22, 2018
@blizzzblizzz changed the title method returns arraygetAppInfo has to return arrayFeb 22, 2018
@nickvergessen

Copy link
Copy Markdown
Member

Add : array to the method then?

null causes exceptions down the road
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix-appinfo-return-val branch from 3b8261b to 8fbcf39CompareFebruary 22, 2018 13:32
@blizzz

Copy link
Copy Markdown
MemberAuthor

added. old habits ¯_(ツ)_/¯

try {
$appPath = $this->getAppPath($appId);
} catch (AppPathNotFoundException $e) {
return null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the old OCP has null documented:

* @return array|null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And it's also expected in some places:

if(!is_array($info)) {
thrownew \Exception(
$l->t('App "%s" cannot be installed because appinfo file cannot be read.',
[$info['name']]
)
);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Check if not array and immediately use as array. Goood )

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Already fixed in https://github.com/nextcloud/server/pull/8504… but could use a backport.

@blizzzblizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 22, 2018
@blizzz

Copy link
Copy Markdown
MemberAuthor

😕 thx. so much for a quick fix. looking later at it again.

@rullzer

Copy link
Copy Markdown
Member

No activity for a long time. I'll close this. Feel free to reopen if somebody picks it up.

@rullzerrullzer closed this May 23, 2018
@rullzer
rullzer deleted the fix-appinfo-return-val branch May 23, 2018 17:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@blizzz@nickvergessen@rullzer@zorn-v