Detect if a shipped app is updated and can be updated

[MorrisJobke]

See https://github.com/nextcloud/docker/issues/288

Having two app folders configure:

• /apps with all the shipped apps and the writable flag set to false
• /apps2 with the writable flag set to true to install apps via the app store

Once we publish an update of a shipped app via the app store the app is put into apps2 and does not replace the original code and thus is not properly updated.

@nickvergessen @rullzer Ideas how to fix? I would try to detect this before the update is triggered by the user or the occ upgrade script.

[nickvergessen]

I thought our code was clever enough to always load apps from the app dir where they are the newest version?!

[MorrisJobke]

I thought our code was clever enough to always load apps from the app dir where they are the newest version?!

Doesn't seem to be like that and I also haven't seen any code lately while digging through the app management code. 😕

[nickvergessen]

Found it, the following should take care of it. Maybe we lost the track to it somewhere: https://github.com/nextcloud/server/blob/96a5752340bc115ad8dffa50cfe3846557f82cc5/lib/private/legacy/app.php#L437-L477

[j-ed]

It seems that this issue ticket covers my recently reported update problem (https://github.com/nextcloud/server/issues/8938)

[nickvergessen]

Ah, I think the issue might be psr autoloading paths are not changed. When the new version is downloaded, it is stored in the new path, and "used". But the class map is still on the old path. So when the path changes, we need to re-register autoloading and overwrite static $app_dir in the code I linked.

[MorrisJobke]

Ah, I think the issue might be psr autoloading paths are not changed. When the new version is downloaded, it is stored in the new path, and "used". But the class map is still on the old path. So when the path changes, we need to re-register autoloading and overwrite static $app_dir in the code I linked.

Ah ... true that was there as well 🙈

[tilosp]

Related https://github.com/nextcloud/docker/pull/308 https://github.com/nextcloud/documentation/pull/739

[julijane]

I would like to mention as pointed out in my pull request, when the configuration below is used it actually works (at least for me in my tests).

It also makes quite some sense to have it like this, because it appears logical to the user that the first path is checked first, and of course the updated app is to be found there then. I think the "writable" flag actually is more confusing and making this unnecessary complicated. I suggest to get rid of that flag and just check the directories in the order stated and always write into the first one. Problem solved with a configuration which is intuitive and makes sense from a logical point of view.

<?php
$CONFIG = array (
  "apps_paths" => array (
      0 => array (
              "path"     => OC::$SERVERROOT."/custom_apps",
              "url"      => "/custom_apps",
              "writable" => true,
      ),
      1 => array (
              "path"     => OC::$SERVERROOT."/apps",
              "url"      => "/apps",
              "writable" => false,
      ),
  ),
);

[rullzer]

@julijane ah that makes sense yes.

@nickvergessen looking at your code then we only load the newest version right? So then the paths should actually be right. Or am I missing something?

[tilosp]

@rullzer but wouldn't this cause problems the other way around? When for example the pdf viewer get updated by the nextcloud updater. In this Scenario the version in apps would be newer than the version in custom_apps, which would than again cause the same problem.

[rullzer]

@tilosp yeah sure. we need to find a proper solution of course. Just taking the latest version.

[julijane]

@tilosp @rullzer Actually it would be nice if in such a scenario the app is deleted from the custom apps folder. So then you would have always only updated apps in custom apps folder. Maybe make it configurable to do that?

Also I am not sure if always loading the most current app is what is always desired in a multi app path configuration. With the current code it would be possible to "override" a package with a local/older version which would not be affected by updates (but the updatenotification would keep complaining then, of course, but in such a complicated setup you probably would disable it anyway). I have not tested this configuration but I am quite sure that it would work:

<?php
$CONFIG = array (
  "apps_paths" => array (
      0 => array (
              "path"     => OC::$SERVERROOT."/override_apps",
              "url"      => "/override_apps",
              "writable" => false,
      ),
      1 => array (
              "path"     => OC::$SERVERROOT."/custom_apps",
              "url"      => "/custom_apps",
              "writable" => true,
      ),
      2 => array (
              "path"     => OC::$SERVERROOT."/apps",
              "url"      => "/apps",
              "writable" => false,
      ),
  ),
);

Maybe there should be a way to pin a version of a app if desired. Also I am wondering if there should be dependencies between apps. I have seen apps which are a plugin to another app, and then they have to do something like this:

if (\OCP\App::isEnabled('news')
    && class_exists('OCA\News\Plugin\Client\Plugin')) {

(found here: https://github.com/cosenal/mailsharenewsplugin/blob/master/appinfo/app.php)

So stupid checking and no dependency on a specific / minimum version of that other app. The app store is growing and growing and in order to properly support things like apps which are plugins to other apps, there probably should be a package/dependency manager, some sort of apt-equivalent in Nextcloud. Would surely be a lot of work, but it would make a lot of sense.

Just my 2ct :)

[MorrisJobke]

@tilosp @rullzer Actually it would be nice if in such a scenario the app is deleted from the custom apps folder. So then you would have always only updated apps in custom apps folder. Maybe make it configurable to do that?

That is super hard to detect at least from the updater as it doesn't have access to the information in a easy way. I would avoid to have this logic in the updater and would properly handle this in the server code.

Also I am not sure if always loading the most current app is what is always desired in a multi app path configuration. With the current code it would be possible to "override" a package with a local/older version which would not be affected by updates (but the updatenotification would keep complaining then, of course, but in such a complicated setup you probably would disable it anyway). I have not tested this configuration but I am quite sure that it would work:

IMO we should not support something like this. It just asks for trouble :/ We should not overcomplicate things and also not depend on the order of config options too hard.

Maybe there should be a way to pin a version of a app if desired. Also I am wondering if there should be dependencies between apps. I have seen apps which are a plugin to another app, and then they have to do something like this:

Pinning apps works by not clicking "update" in the UI. For major upgrades there is often no way around this. Also we should fix the problem behind the why of "pinning" apps and not add more complexity to allow pinning of app versions.

[MorrisJobke]

So stupid checking and no dependency on a specific / minimum version of that other app. The app store is growing and growing and in order to properly support things like apps which are plugins to other apps, there probably should be a package/dependency manager, some sort of apt-equivalent in Nextcloud. Would surely be a lot of work, but it would make a lot of sense.

No - we don't support this and also don't plan this. Apps should not have inter app dependencies. Reimplementing complex versioning and dependency tools is also nothing we plan to have, because this is far too much to explain users in the web UI why something doesn't work.

[nickvergessen]

@rullzer

looking at your code then we only load the newest version right? So then the paths should actually be right. Or am I missing something?

Well the thing is the order. All apps are loaded by by our code, then the new version is downloaded and should be used from here on. But the static path map and teh set autloaders are still on the old path (a F5 at the right point would fix it, but this is one script call). We need to invalidate those caches and reregister autoloading at the moment where we unpack the new version and try to run the update code from the new location.

[skjnldsv]

What is left to do here? :) I trade this against a vue/js issue! Anyone? :grin:

[compagnon]

2 cents: apps/customapps development uses mostly class_exists ( string $class_name [, bool $autoload = TRUE ] ) : bool with autoload by default ,

As the registered ClassLoader is \Composer\Autoload\ClassLoader() from Symphony : Some Interface / Class from lib/private are not registered into ClassLoader and spl_autoload do not load implemented Interface. (and there is a Interface XXXX not found in the log) .

your issue could be linked to some use of unregistered Class or Interface / in the lib/private ?

A good example is the EmailTemplate::class , I have tried to write a class with

• implements OC\Mail\IEmailTemplate => failed because interface is not loaded , and class_exists() returns exception in Mailer.php
• extends EmailTemplate => OK but EmailTemplate implementation is overdesigned , not need to onboard its implementation for just implementing a contract interface

In fact , my mistake is to use the private not the public: OCP\Mail\IEmailTemplate

I have seen that Mailer.php uses that incorrect test : if ($class !== '' && class_exists($class) && is_a($class, EMailTemplate::class, true)) {

because not care of extends a class: we should have: if ($class !== '' && class_exists($class) && is_a($class, IEMailTemplate::class, true)) { or if (interface_exists("OCP\Mail\IEMailTemplate") && $class !== '' && class_exists($class) && is_a($class, IEMailTemplate::class, true)) {

[kesselb]

Thanks for your feedback @compagnon :+1: Would you mind to create a new issue? The problem / technical debt looks unrelated to this issue.

[szaimen]

Hi, please update to 24.0.8 or better 25.0.2 and report back if it fixes the issue. Thank you!

[nickvergessen]

cc @skjnldsv @blizzz Updating shipped apps via the appstore works nowerdays, right?

[blizzz]

yes

[szaimen]

I think this does not properly work yet: https://github.com/nextcloud/related_resources/issues/170