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.

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

Jump to bottom

Only load viewer in settings if the viewer app is enabled #22650

Merged
merged 1 commit into from
Sep 7, 2020
Merged

Only load viewer in settings if the viewer app is enabled #22650

merged 1 commit into from
Sep 7, 2020

Conversation

MorrisJobke
Copy link
Member

Found by Psalm:

2 errors fixed
/github/workspace/apps/settings/lib/Settings/Personal/ServerDevNotice.php:83:37:error - UndefinedClass: Class or interface OCA\Viewer\Event\LoadViewer does not exist
/github/workspace/apps/settings/lib/Settings/Personal/ServerDevNotice.php:83:60:error - UndefinedClass: Class or interface OCA\Viewer\Event\LoadViewer does not exist

@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Sep 7, 2020
@MorrisJobke MorrisJobke added this to the Nextcloud 20 milestone Sep 7, 2020
@MorrisJobke MorrisJobke mentioned this pull request Sep 7, 2020
Merged
2 tasks
@MorrisJobke
Copy link
Member Author

Regression from #22136

@kesselb
Copy link
Contributor

kesselb commented Sep 7, 2020

Viewer app is always enabled afaik.

@skjnldsv
Copy link
Member

skjnldsv commented Sep 7, 2020

Viewer app is always enabled afaik.

This 👍
You cannot disable nor remove viewer

@MorrisJobke
Copy link
Member Author

You cannot disable nor remove viewer

Didn't knew that. I thought it is only default enabled. Then we should add it to the external dependencies that are available.

@MorrisJobke
Only load viewer in settings if the viewer app is enabled
ce1e873 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member Author

I changed it to a zero-cost check that asserts the obvious, but fails in development environments properly.

@MorrisJobke MorrisJobke merged commit 0e6344f into master Sep 7, 2020
@MorrisJobke MorrisJobke deleted the fix/noid/fix-psalm-warning branch September 7, 2020 18:41
@rullzer rullzer mentioned this pull request Sep 8, 2020
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@kesselb kesselb kesselb approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 20
Development

Successfully merging this pull request may close these issues.
4 participants
@MorrisJobke @kesselb @skjnldsv @ChristophWurst