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.

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

fix(SetupChecks): Add reasoning/clarity to outdated PHP check #45261

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented May 10, 2024

Summary

  • Add reasoning based on our existing docs
  • Add specific version range suggested for the current Server version
  • Tweak the language a bit for improved readability (fewer disparate version numbers)
  • Eliminate need to update Server versions in the message before every major release

Note: Decided not to add upper bound check for now.

TODO

Follow-up: A lot of these dependency min/max versions should be moved to constants (maybe in version.php (e.g. PHP, MariaDB, etc.) to ease our recurring release cycle tech debt.

Checklist

@@ -47,7 +47,7 @@ public function getName(): string {

public function run(): SetupResult {
if (PHP_VERSION_ID < 80200) {
return SetupResult::warning($this->l10n->t('You are currently running PHP %s. PHP 8.1 is now deprecated in Nextcloud 30. Nextcloud 31 may require at least PHP 8.2. Please upgrade to one of the officially supported PHP versions provided by the PHP Group as soon as possible.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');
return SetupResult::warning($this->l10n->t('PHP %s detected. PHP >=8.2 and <= 8.3 is suggested for optimal performance, security, and stability with this version of Nextcloud. Additionally, the next major release of Nextcloud will likely require >= PHP 8.2.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return SetupResult::warning($this->l10n->t('PHP %s detected. PHP >=8.2 and <= 8.3 is suggested for optimal performance, security, and stability with this version of Nextcloud. Additionally, the next major release of Nextcloud will likely require >= PHP 8.2.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');
return SetupResult::warning($this->l10n->t('PHP %s detected. PHP >= 8.2 and <= 8.3 is suggested for optimal performance and stability with this version of Nextcloud. Additionally, the next major release of Nextcloud will likely require >= PHP 8.2.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');

I don't like having the security aspect in there. PHP 8.1 has official security support until 31 Dec 2025.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see going either way. Our docs describe it that way, but admittedly that's also in the context of why we overall as a project drop support for older PHP versions. I guess it's less of a concern with PHP's longer Security fixes only cycles (at least as of 8.1).

One consideration is that, for example, in v27/v28/v29 we find ourselves still listing support for PHP 8.0 even though it's not even receiving security fixes at all. So under those circumstances this message might be perfectly valid in mentioning security too (obviously the specific PHP versions mentioned would be different; this PR is only for what's true as of v30 - just trying to provide a real-world example).

But that sort of situation goes away moving forward based on the PHP's newer post-EOL continued critical security fixes for two-year policy. So admittedly it probably is reasonable to remove the reference to security moving forward given our own release cycles.

Though to muddy the waters a bit again: we have to (or at least have chosen to) account for the individual distros (which I think is both reasonable and mostly unavoidable) which have their own internal security fix policies that sometimes are in conflict with/extend beyond whatever PHP's policy is at the time.

I'm curious what @nickvergessen's opinion might be.

I'm leaning towards leaving it in still myself (at least at the moment hah).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. We explicitly state the security aspect of updating PHP in our admin documentation so I'm fine with leaving it in.

Nevertheless, could we please have the space between >= 8.2?
Should be: PHP >= 8.2 and <= 8.3 is suggested

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though to muddy the waters a bit again: we have to (or at least have chosen to) account for the individual distros (which I think is both reasonable and mostly unavoidable) which have their own internal security fix policies that sometimes are in conflict with/extend beyond whatever PHP's policy is at the time.

Well the idea was basically to allow installing on all supported distros at the moment of the release, and after that each Nextcloud major will continue to support those PHP major versions for eternity.
I'm fine with listing security as well. It should just be made clear that 8.1 is an option and there is no immediate update needed (especially since official PHP support is covering it soon, although that doesn't mean the distros are staying on top of security patches all the time).

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

@@ -47,7 +47,7 @@ public function getName(): string {

public function run(): SetupResult {
if (PHP_VERSION_ID < 80200) {
return SetupResult::warning($this->l10n->t('You are currently running PHP %s. PHP 8.1 is now deprecated in Nextcloud 30. Nextcloud 31 may require at least PHP 8.2. Please upgrade to one of the officially supported PHP versions provided by the PHP Group as soon as possible.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');
return SetupResult::warning($this->l10n->t('PHP %s detected. PHP >= 8.2 and <= 8.3 is suggested for optimal performance, security, and stability with this version of Nextcloud. Additionally, the next major release of Nextcloud will likely require >= PHP 8.2.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return SetupResult::warning($this->l10n->t('PHP %s detected. PHP >= 8.2 and <= 8.3 is suggested for optimal performance, security, and stability with this version of Nextcloud. Additionally, the next major release of Nextcloud will likely require >= PHP 8.2.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');
return SetupResult::warning($this->l10n->t('PHP %s detected. While PHP 8.1 is still supported by this version of Nextcloud, it's not recommended and PHP >= 8.2 and <= 8.3 is suggested for optimal performance, security, and stability. Additionally, the next major release of Nextcloud will likely require >= PHP 8.2.', [PHP_VERSION]), 'https://secure.php.net/supported-versions.php');

Maybe keeping it a bit more "open"?

Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants