-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Improve app installation experience #8742
Comments
I can’t say much to the technical stuff here, but in general it sounds good if it solves the problem that people can brick their installation. One note about the app dependencies: All of these should be fully optional enhancements. (The Tasks and Calendar dependency example because of CalDAV is a different issue though: #5939 ) cc app developers like @jbtbnl @LEDfan @georgehrke … |
@jancborchardt sure, would still be nice if we could tell users on install what the app depends on. The dependencies should only be checked on install, no need to pull everything from the appstore. What I mean is we dont need to build a package manager. But if someone writes an app that adds TT-RSS api to owncloud news it should obviously only be installable if news is installed (or at least hint the user: hey this wont work without news) |
A work in progress is going on at #10777 |
Nice summary of the problems and implementable solutions. |
This violates the owncloud architecture principle - if apps need to interact APIs in core have to be provided |
No necessary from my pov as we have assetic in place which will generate conactenated js and css files for every page - minification to be added ... |
IMO each app should be responsible for minification since it's so easy to do with grunt |
Yeah we can remove the app dependency stuff if we do it similarly to android, see the calendar and contacts libs |
Is this already resolved, thanks to #10777, which has been resolved? Or is still something missing here? |
A lot has improved already and app dependencies check were added as well. There was also another PR a while ago in 9.0 that would check whether an app bricks OC directly after enabling it and then disables it again: #17451 I'm closing this now. If you think there are additional cases missing where OC bricks itself, feel free to reopen. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
I think it makes sense to discuss this in a seperate thread (see #8736)
TL;DR: Get rid of app.php and routes.php and move stuff to config files to prevent apps from breaking owncloud and improve installation UX
PHP is an interpreted language. That means that we cant use a new compiler to use newer features. PHP's exception handling is also bad: You can't test if an app has syntax errors and if it does and you enable it, it will break every request where the file is included (or if you can, please let me know). So you can't compare it to an Android app since an android app cant brick your phone if you install it.
Problems
This leads to the following problems:
Solutions
Possible solutions that come to my mind:
Further thoughts
Other things that can be improved by modifying the info.xml:
All of the above mentioned improvements could be cached and would get rid of almost all cases where you need to include the app.php
The benefit is that it would be faster and since the app.php would not have to be included on every request it would significantly reduce the risk that a broken version (or PHP 5.4/5.5 version) of the app will completely brick the installation.
The problem of bricking the install could be further reduced by also having the routes in a seperate config file instead of appinfo/routes.php, just a thought
All these proposals are backwards compatible and would make owncloud more solid.
@karlitschek @jancborchardt @PVince81 @DeepDiver1975 @MorrisJobke
The text was updated successfully, but these errors were encountered: