Add setup check for recommended PHP modules (i.e. Imagick, intl)

[MorrisJobke]

Fixes #12767

I also added one warning for Imagick - that makes it easy for the theming related issues to find the solution.

[MorrisJobke]

/backport to stable15

[kesselb]

A few days after the release, people will be asking for a way to hide this warning 🙈 🙉

[MorrisJobke]

A few days after the release, people will be asking for a way to hide this warning 🙈 🙉

That's easy: install the modules.

But also as those warnings are only shown on the admin page it's fairly easy to ignore them - in contrast to the old banner with "code integrity check failed".

[MichaIng]

@MorrisJobke @danielkesselberg
I agree that admin panel warnings should only appear for modules, that are really required, or conditionally based on installed apps in case.

Generally people most likely want to resolve all warnings that appear on the admin panel. Yeah installing the modules works. But if the module is optional and/or only used by optional apps and/or absence can be handled by disabling a certain feature (imagick image rotation or what it was?) then people should not be forced to install/enable them, at least to resolve warnings.

So my suggestion:

• Do not show warnings about "recommended" modules.
• Instead move intl to "required" apps and show warnings about those only. Obviously intl is now required if you don't want to break client sync (in some situations at least) or have a totally spammed nextcloud.log.

[MorrisJobke]

Generally people most likely want to resolve all warnings that appear on the admin panel. Yeah installing the modules work. But if the module is optional and/or only used by optional apps and/or absence can be handled by disabling a certain feature (imagick image rotation or what it was?) then people should not be forces (at least to resolve warnings) to install/enable them.

This is also not a warning, but of level "info". IMO it's fine to show potential causes of problems to people. We want to give them the opportunity to have a nice experience with this. The same goes for OPCache or memcache. Sure the application works without it, but it even works better if you enable this. And thus we have this section. It's basically an automatic check of what we recommend against the current instance. The background for this is that nobody reads the docs until something doesn't work. So updating and improving the Nextcloud experience comes the downside that we have no direct channel to communicate knowledge from us to administrators. Those setup checks are basically here to help get the most out of your installation.

Also those requirements don't come out of nowhere: intl caused a hard to identify problem on your system: so why not be proactive and recommend to install this to make the Nextcloud work better? Same goes for Imagick: it is written in the theming already, but it seems that this is still overlooked and people complain about a feature to work not as nice as it could: avatars and favicon. With Imagick we can improve the experience a lot and don't have reports of this in the forums and the bug tracker here.

Other opinions on this? @skjnldsv @jancborchardt @rullzer @nickvergessen @jospoortvliet ?

[MorrisJobke]

• Do not show warnings about "recommended" apps.

This is also not a warning about a recommended app but general core features.

[skjnldsv]

Awesome!

[kesselb]

After this pr is merged we can patch patchwork\utf8 and remove the warning https://github.com/nextcloud/3rdparty/blob/2146ef45734f855676965b65990d509192666264/patchwork/utf8/src/Patchwork/Utf8/Bootup/intl.php#L18

[MichaIng]

@MorrisJobke

This is also not a warning about a recommended app but general core features.

Ah jep, didn't meant apps, but PHP modules 😉.

And you are right. For OPcache (Although one could discuss the values, 128M for only ~30M Nextcloud PHP scripts? Different topic...), APCu, Redis file locking and such, I also totally agree this should be shown. There is really no reason to not use/set this, if anyhow possible, which might only on hosting providers not be the case. So for other recommendations this is then fine as well/consistent.

About intl logs:

This is also not a warning, but of level "info".

Are you sure about this? Sadly I cleaned my log files, but from what I pasted on the other issue, and also from what I remember, it was either error or warning:

[PHP] Error: You are using a fallback implementation of the intl extension. Installing the native one is highly recommended instead. at /var/www/nextcloud/3rdparty/patchwork/utf8/src/Patchwork/Utf8/Bootup/intl.php#18

GET /nextcloud/ocs/v2.php/cloud/activity?page=0&pagesize=100&format=json
from 46.59.179.64 by Micha at 2018-12-01T00:31:04+01:00

Nevertheless, if the module can be still considered as not "required", then perhaps this PHP error should be handled to not show up in such a frequency. However THAT actually PHP throws an error, for my impression makes the module "required" 😉. Absence of recommendations should be handled internally to prevent PHP errors right from the start.

E.g. docs say: https://docs.nextcloud.com/server/stable/admin_manual/installation/source_installation.html#prerequisites-for-manual-installation

PHP module intl (increases language translation performance and fixes sorting of non-ASCII characters)

• But the modules API seems to be accessed (attempted) by Nextcloud core code in every case?

Imagick:

Docs say:

For preview generation (optional):

• PHP module imagick

vs.

Same goes for Imagick: it is written in the theming already, but it seems that this is still overlooked and people complain about a feature to work not as nice as it could: avatars and favicon. With Imagick we can improve the experience a lot and don't have reports of this in the forums and the bug tracker here.

• So the docs are outdated about this? Reading them, I would say I don't need it, since I don't browse images via WebUI much and even without it I couldn't find any image type that could not be previewed in my gallery. So the additional supported file types seem to be very rare ones?
• But theming app uses it for avatars and favicon as well? This info should perhaps then added to the docs. Still I personally would not like to have warning about it missing, since I don't use the theming app 🙂. Perhaps the warning could be shown only, if theming app is actually installed+enabled? Same could be done for other modules that are used only by certain apps (or features that can be disabled, e.g. server side encryption => mcrypt, if this is even current, since outdated?), but not core.

[jancborchardt]

I do get @MichaIng's points and I also trust your judgement @MorrisJobke. 🙂

[kesselb]

Perhaps the warning could be shown only, if theming app is actually installed+enabled? Same could be done for other modules that are used only be certain apps (or features that can be disabled, e.g. server side encryption => mcrypt, if this is even current, since outdated?), but not core.

Sounds like a good improvement to me

[MorrisJobke]

Sounds like a good improvement to me

Let me add this and then it's fine to get in.

[MorrisJobke]

Let me add this and then it's fine to get in.

Done :)

[rullzer]

🐘

[MorrisJobke]

• So the docs are outdated about this? Reading them, I would say I don't need it, since I don't browse images via WebUI much and even without it I couldn't find any image type that could not be previewed in my gallery. So the additional supported file types seem to be very rare ones?
• But theming app uses it for avatars and favicon as well? This info should perhaps then added to the docs. Still I personally would not like to have warning about it missing, since I don't use the theming app 🙂. Perhaps the warning could be shown only, if theming app is actually installed+enabled? Same could be done for other modules that are used only by certain apps (or features that can be disabled, e.g. server side encryption => mcrypt, if this is even current, since outdated?), but not core.

Yes - the docs are not up-to-date it seems. I change it now to only show once theming is enabled. mcrypt is not needed anymore and I handed in a PR to the docs today. So we are on our way to bring both of them in sync. Thanks for your feedback. This also makes Nextcloud just better :)

[MichaIng]

@MorrisJobke
Many thanks for your great work!

Good to hear that indeed mcrypt is not used anymore. I was only guessing, since everywhere else it is marked as obsolete/outdated and Debian testing repo does not have it available.

And btw: Great work on docs visual rework. I like it, looks cleaner somehow and better contrasts 👍. And... it stands out from ownCloud docs.

• In case to forward to responsible devs 🙂.

[MorrisJobke]

• In case to forward to responsible devs 🙂.

@skjnldsv ;)

[weeman1337]

Works ✓

[backportbot-nextcloud]

backport to stable15 in #12837

[kyrofa]

A few days after the release, people will be asking for a way to hide this warning 🙈 🙉

I'm coming here just for that. Imagick is a sieve with a terrible security history. The snap doesn't enable it for that reason, but now all snap users will start seeing an un-dismissable warning about it that will result in bugs being logged.

[skjnldsv]

@kyrofa well, there is also no other php library that does the job. So it's a simple warning, not a red error.
We unfortunately cannot do any other way :(

[kyrofa]

Can we make it possible for nextcloud packagers to specify the warnings that are valid for the packaging method being used? Some file that is not included in the integrity check? We're talking 20k users who will get a warning about which they can do nothing. That seems silly. I'm not trying to stop you from warning about it, I just want a way to opt out of it.

[enoch85]

The snap doesn't enable it for that reason

Same goes for the VM, based on the discussion that the Snap team had. We will not use it per default, though make it optional by installing the previewegenerator app with a warning telling the user about the security flaws.

[MorrisJobke]

I'm coming here just for that. Imagick is a sieve with a terrible security history. The snap doesn't enable it for that reason, but now all snap users will start seeing an un-dismissable warning about it that will result in bugs being logged.

In mid December I talked to somebody in person that also worked on the snap. I told him that the Imagick stuff that we use only use the functions that were not affected by the security issues (like the SVG problems). By default we only use it for the more safe formats and have disabled for example the SVG previews, because they need additional configuration by the admin to be a bit more safe.

@kyrofa Does that help you with judging on the usage? Also have a look at the sample config php file inside config/ for a little explanation of the by default enabled preview providers.

[kyrofa]

@MorrisJobke not really, if imagick is there it's still a risk, either from improper usage in nextcloud itself or from third-party apps. The only safe option is to not include it, which is what we've always done.