Skip to content
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

Correctly remove admin sections and settings #1287

Merged
merged 1 commit into from
Sep 7, 2016

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Sep 6, 2016

Steps

  1. Enable "Server info" app
  2. Disable "Server info" app
  3. Go to the admin page with the log
  4. Refresh page

See https://help.nextcloud.com/t/usage-report-0-1-5-error-in-logs-if-we-disable-this-app/2963

Expected

nothing

Actually

Each page refresh logs a new QueryException

We can't really create an update step for this, because on update apps are disabled and therefor we can't check, if the class would exist. So manual clean up is required.

@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the annotation information on this pull request, we identified @blizzz and @LukasReschke to be potential reviewers

@@ -105,10 +105,10 @@ public function onAppDisabled($appId) {
$appInfo = \OC_App::getAppInfo($appId); // hello static legacy

if(isset($appInfo['settings'][IManager::KEY_ADMIN_SECTION])) {
$this->remove(self::TABLE_ADMIN_SECTIONS, $appInfo['settings'][IManager::KEY_ADMIN_SECTION]);
$this->remove(self::TABLE_ADMIN_SECTIONS, trim($appInfo['settings'][IManager::KEY_ADMIN_SECTION], '\\'));
Copy link
Member Author

Choose a reason for hiding this comment

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

all other methods use get_class which has no leading slash: https://3v4l.org/rquje

@blizzz
Copy link
Member

blizzz commented Sep 6, 2016

LGTM

1 similar comment
@rullzer
Copy link
Member

rullzer commented Sep 7, 2016

LGTM

@rullzer rullzer merged commit 1d04c9e into master Sep 7, 2016
@rullzer rullzer deleted the correctly-remove-admin-stuff branch September 7, 2016 12:30
@rullzer
Copy link
Member

rullzer commented Sep 7, 2016

@nickvergessen could you prepare the backports?

@nickvergessen
Copy link
Member Author

Yes, backport in #1305

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 bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants