-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Migrate from php8.0 to php8.1 #2309
base: main
Are you sure you want to change the base?
Conversation
I'm not understanding why we would stop using Line 7 in e419b62
I understood why PHP still had 'php8.0' as it didn't have access to that variable, but with |
Sorry, I'm not sure what you mean. Anyway, also note that @binarykitchen proposed to split this pull request up into a version handling bit (see #2308, derived from this work) and a php upgrade bit. |
Right @jvolkenant MiaB must depend on a clearly defined PHP version, not on whatever is installed by random on the server. That's why my low-risk PR doesn't do that, nor is upgrading anything. All my PR does it to keep its version consistent across all the code base without having it hardcoded too much. Secondly, there is no need to upgrade PHP to v8.1 right now. Although, I'd like to give credit to @kiekerjan for some inspiration in my other PR. |
TLDR; Nextcloud's strict requirements on maximum PHP version has made our Nextcloud upgrades tricky. I don't have an answer, just stating some possibilities. I've had a few thoughts around our potential PHP8.1 and Nextcloud conundrum. This might be a bit long winded so please bear with me. I'm also likely to repeat a lot of what was raised in the other issues and PRs related to this. Nextcloud v27 minimum non-deprecated version of PHP is 8.1. and presumably in a year it will be 8.2 for whatever version is released. This is Nextcloud keeping in step with PHP release cycles (aggressively so?). As we upgrade each previous Nextcloud version through all the current ones, it might be reasoned that users will need to have PHP8.0 installed to upgrade up to Version24 (only applicable if coming from a MIAB version prior to v60 and the current version is on PHP8.1) Options I understood @kiekerjan 's function was to help resolve if we have multiple versions of PHP needed during the installation process so that Nextcloud could be upgraded no matter what version of PHP or Nextcloud database there is or is needed. Possibly the idea being able to determine what version of PHP was installed so the correct commands and sequence get called. Another similar option, having looked at the Nextcloud repo it seems that the version check for the command line is only for versions below 8.0 for v27 and the current master. Perhaps we could use PHP8.0 to do all the upgrades and then finally upgrade to PHP 8.1 (or PHP8.x in the future). This is in a way similar to the function approach but we would hardcode installing, utilising and removing PHP8.0 just for the steps that are needed. Alternatively we could enforce that users have to upgrade to a specific version of MIAB if coming from previous nextcloud ones. I have more to say on this one. Either at the start of the setup scripts or during Nextcloud's one. Or last find an alternative solution to Nextcloud so we aren't tied to their strict PHP versions. I don't think anyone has time to work on an alternative and the scripted migration process needed. If they had it would have probably been done by now :) PHP Versions different classes of users
Hopefully most are in 1 and no one is in 4 but we still need to (should?) allow for these upgrade paths. Bit more on each option Use PHP8.0 and 8.1 during nextcloud upgrade Force users to update to a certain version Updates to the upgrade doco on the website and probably a pinned post on the forum would probably also need to be done for the late comers as that is likely the first places they will look. When we move to 8.2 we may need to include something in the bootstrap to check the version and update it to the latest supported range of pre8.2. This obviously has a fair few drawbacks, and will confuse some people when it comes to updating. Summary Hopefully be corrected on the above and an easy solution is put forward. Also sorry for the long post and thank you for reading. |
Thank you for that elaborate post. I don't have a detailed reply on all your arguments right now, but I wanted to add some insight into this pull request.
I think this is what this pu request proposes. By default, php8.1 will be installed from the ubuntu repositories. This works for all software installed. I like the statement on different classes of users
Should have no issues, on update php8.1 is installed, and php8.0 removed
Similare to the previous class. On an update, php8.1 is installed. Php8.0 is already installed, might be used to accomodate nextcloud upgrades, then is removed
Same as previous class
On upgrade, php8.1 is installed. During upgrade of nextcloud, php8.0 is temporarily installed to accomodate upgrades. |
One more option that I had proposed during the discussion on upgrading to Jammy is to use docker and the Nextcloud images on docker hub to upgrade the MIAB Nextcloud database. This eliminates all OS/Nextcloud/PHP version issues because the docker image contains everything. The purpose would be to get you to a Nextcloud version that works with the desired PHP installed on the system. The steps are: o install docker, if not already installed At this point owncloud.db and config.php are upgraded to (in this example) Nextcloud 21. Repeat the steps until the database and config.php are upgraded to the desired Nextcloud version. Finally, copy the Nextcloud installation files onto the production system (download and untar), and you're good to go. The whole upgrade process doesn't even have to happen in situ. Copy the database and config to a local machine, run through the docker sequence there, then copy them back. |
Regarding this PR... Why not use PHP_VER for That would avoid the hard coded php version numbers in setup. It might be clearer that way. |
I would prefer to see an explicit version number everywhere in case there are multiple versions of PHP installed. |
And just to add another option to the mix: Modifying nextcloud's versioncheck.php so as to remove the restriction on PHP8.1 for older nextcloud versions used for the upgrade. Tested this with a test v57a upgrade to v64. Pros: Cons: I haven't done a PR for this as the branch isn't complete by itself, More wanted to demonstrate this as an idea. Happy for someone else to pick it up if it provides inspiration. |
@matidau Thanks for giving fresh ideas and listing the Pros + Cons. Regarding "risk appetite", MiaB has always been and must be well-tested for any version bumps of any dependencies. MiaB is about reliable services without excuses. If one email doesn't arrive, goodbye. Our release frequency is low and everyone is busy, so cannot afford regressions. We need more unit tests, automated tests and the missing gaps must be tested by humans. Just adding my two cents. Low risks, please. |
@JoshData I see that you have a different opinion on how multiple versions of php should be handled. I'm willing to put in some more time to realize this, but I would like to ask you for your input beforehand. Are there conditions you want to see before you would consider merging? What kind of things would you reject? |
Hello all - what's happening with this PR? Any problems? We need PHP v8.1 soon ... |
Ping? |
@kiekerjan I see you pushed and rebased. Ready for review and merge? |
This pull request proposes the upgrade from php8.0 to php8.1. This would remove the dependency on an external ppa.
The logic works by introducing on a number of places a variable containing the php version. This makes the code independent of the used php version.
The upgrade logic is embedded in the nextcloud installation logic. If a version of nextcloud that does not support php8.1 is to be installed, the code will also install php8.0. Once a new enough version of nextcloud is detected, php8.0 is removed.
Installation has been tested via vagrant. Todo: test upgrade from v60 to this, and test upgrade from v57 to this.