- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.6k
[stable30] fix(Apps): fix install command check on existing apps #55065
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
[stable30] fix(Apps): fix install command check on existing apps #55065
Conversation
| otoh not sure whether this approach might cause other side effects, as  UPDATE: | 
- AppManager::isInstalled() is misleading, as it checks only whether it is enabled. But an app might not be present in some edge cases. - AppManager::getAppPath() does however only check whether an app dir is present, independent of the enabled-state. Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
8e06240    to
    ac6653e      
    Compare
  
    - includes nextcloud/server#55065 Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
- includes nextcloud/server#55065 Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not a good change.
To install an application manually, you put it in the apps folder and then run app:install, no? With this change it would not be possible anymore?
| 
 No, you run  | 
Summary
Currently, enabled apps are returned, whether they are installed or not. This is a behavioral breaking change compared to 29.
Based this now on 30 for nextcloud/univention-app#208, and I am not sure this is what we really want in server (cf. my comment at https://github.com/nextcloud/server/pull/49648/files#r2341962849).
If so, I'd do a new PR based on master, which has diverges further.
Checklist