search

nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.43k stars 4.08k forks source link

Support PHP8.1 #29287

Closed ChristophWurst closed 2 years ago

ChristophWurst commented 3 years ago

According to https://php.watch/news/2021/03/php81-release-date PHP8.1 will be released by end of next month. That will mean the release happens somewhere around the release of Nextcloud 23 and we can assume that the early adopters will soon update their systems and find out Nextcloud 23 doesn't support 8.1. Ref https://github.com/nextcloud/server/blob/822623109f4f4b3252c04a5c0ccf6757a882e771/lib/versioncheck.php#L36-L42.

As far as I know php8.1 is "just" a minor release but some libs/code have to be adjusted. We'll have to decide whether to look into this for 23, 23.1 or 24.

Potential breaking changes

cc @AndyScherzinger @juliushaertl @nickvergessen @skjnldsv

skjnldsv commented 3 years ago

cc @come-nc since you asked :)

ChristophWurst commented 3 years ago

I checked the 3rdparty repo and no dep is actively blocking php8.1 right now (overrode the version number to test this)

come-nc commented 3 years ago

Hello.

I know at least that the use of LDAP resources will be a problem since there are calls to isresource on ldap* results in nextcloud code, and those are turned into objects instead in PHP 8.1.

I’d be happy to start a PR to work on that if you want, and at least make sure the unittest suite for user_ldap works on 8.1. I can try to work on the rest of the test suite but I know less about it.

It seems PHPCompatibility is not uptodate for 8.1 yet but at least they provide a nice list of things to look for: https://github.com/PHPCompatibility/PHPCompatibility/issues/1299

come-nc commented 3 years ago

There is this error for PHP 8.0 already: 

FILE: /home/mcmic/dev/nextcloud/server/lib/private/L10N/Factory.php
-----------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------------
 595 | ERROR | Function create_function() is deprecated since PHP 7.2 and removed since PHP 8.0; Use an anonymous function instead
 600 | ERROR | Function create_function() is deprecated since PHP 7.2 and removed since PHP 8.0; Use an anonymous function instead
-----------------------------------------------------------------------------------------------------------------------------------

Not sure if this is known already, so noting it here before I forget.

AndyScherzinger commented 3 years ago

thanks for the ping @ChristophWurst given the amount of time and people I'd not rush this for 23.0.0 also given the fact that apps might just break because they have php8.1 issues and that covers ours as well as community apps. So this seems to be to short notice for app devs and likely is also to risky for us and the v23 timeline given that we already reach beta status.

come-nc commented 3 years ago

Needed for 8.1 support:

come-nc commented 3 years ago

Does anyone know the reason for d690f909284ae4bb4dee7d00318104ee76720bfa ?

ChristophWurst commented 3 years ago

Does anyone know the reason for d690f90 ?

https://github.com/nextcloud/server/pull/23780#issuecomment-739883843

I suggest we revert ans see what happens :)

come-nc commented 3 years ago

There is a PHP error triggered by the call at https://github.com/nextcloud/server/blob/master/lib/private/Files/Cache/Propagator.php#L89 , because the second parameter is not given and it defaults to NULL at https://github.com/nextcloud/server/blob/master/lib/private/DB/QueryBuilder/ExpressionBuilder/ExpressionBuilder.php#L421 And it ends up in a call to PDO::quote on doctrine side, which triggers a warning as the type is supposed to be an int. The default value for $type in PDO::quote is PDO::PARAM_STR so I will switch to that if there are no objections. (writing all this here before forgetting why I made the change ^^) Actually we have IQueryBuilder::PARAM_STR which is an alias and I will use that to match the comment of the function

come-nc commented 3 years ago

libxml_disable_entity_loader is deprecated but I do not understand what it does and why we call it. Leaving it in for now.

nickvergessen commented 3 years ago

@LukasReschke will give you a lesson about it

come-nc commented 3 years ago

The tests for Archive run into this problem: https://stackoverflow.com/questions/64698935/using-ziparchive-with-php-8-and-temporary-files I tried fixing it by this change:

diff --git a/tests/lib/Archive/ZIPTest.php b/tests/lib/Archive/ZIPTest.php
index 17a639c9f5..b6eb947f53 100644
--- a/tests/lib/Archive/ZIPTest.php
+++ b/tests/lib/Archive/ZIPTest.php
@@ -17,6 +17,8 @@ class ZIPTest extends TestBase {
        }

        protected function getNew() {
-               return new ZIP(\OC::$server->getTempManager()->getTemporaryFile('.zip'));
+               $tmpfile = \OC::$server->getTempManager()->getTemporaryFile('.zip');
+               \unlink($tmpfile);
+               return new ZIP($tmpfile);
        }
 }

But I get a misterious warning at the end of the tests:

PHP Warning:  PHPUnit\TextUI\Command::main(): Cannot destroy the zip context: Can't remove file: No such file or directory in /home/mcmic/.config/composer/vendor/phpunit/phpunit/src/TextUI/Command.php on line 96

No idea if the actual code is concerned.

come-nc commented 3 years ago

In several cases there are PHP warnings in the tests because the Mock returns null instead of string or int. When the return type is only present in the phpdoc and is not there as a return type this happens I think.

come-nc commented 3 years ago

https://github.com/opis/closure/issues/107#issuecomment-948853378 (and related https://github.com/opis/closure/issues/102 ) sounds like bad news. If anyone with more knowledge of opis/closure and what we use it for can look into it that would be great. (If I understand correctly what I read in the second issue only version 4.x will support PHP 8.1 but they depend upon FFI being enabled and I’m not sure we can add that as a requirement for Nextcloud. There is also talk about a fork by laravel so maybe we can look into that)

come-nc commented 3 years ago

https://github.com/doctrine/dbal/issues/4906#issuecomment-949027565 This is good news on the other hand :-)

come-nc commented 3 years ago

queryBuilder::getFirstResult in doctrine now always returns an int, which defaults to 0 instead of null. This causes this test failure: Query Builder (Test\DB\QueryBuilder\QueryBuilder) ✘ First result with data set #0 [3.58 ms] │ │ Failed asserting that 0 is identical to null. │ │ /home/mcmic/dev/nextcloud/server/tests/lib/DB/QueryBuilder/QueryBuilderTest.php:132 │

Should I just remove the null from the dataset for this test?

ChristophWurst commented 3 years ago

queryBuilder::getFirstResult in doctrine now always returns an int, which defaults to 0 instead of null.

That has already been addressed in https://github.com/nextcloud/server/pull/29344

come-nc commented 3 years ago

sabre/dav will need to be updated to a newer release when one is published with PHP 8.1 support. There are already PRs fixing 8.1 support for both sabre/dav and sabre/vobject it seems.

AndyScherzinger commented 3 years ago

@skjnldsv reading through the latest comments I think we definitely stick with maxPhpVerison = 8.0 for 23 - what do you think?

come-nc commented 3 years ago

@AndyScherzinger Sounds reasonable, seeing how much both Nextcloud code and the dependencies code is impacted. (And I’m still working on core, I did not tackle the apps yet…)

come-nc commented 3 years ago

Fix pimple (or wait for a fix)

Version 3.5.0 should support 8.1: https://github.com/silexphp/Pimple/blob/main/CHANGELOG

ChristophWurst commented 3 years ago

https://github.com/sabre-io/dav/pull/1354 might also be relevant for us

come-nc commented 3 years ago

sabre-io/dav#1354 might also be relevant for us

Yes as well as https://github.com/sabre-io/vobject/pull/544

come-nc commented 3 years ago

The test Test\Avatar\GuestAvatar/Get is failing with PHP 8.1 on my local setup.Comparing the expected and actual binary string I can see that the generated avatar is off to the right by one pixel on PHP 8.1. I had to build the imagick extension to link it to my local PHP 8.1 build so maybe I messed up something, but as there are changes noted to https://www.php.net/manual/fr/function.imagettfbbox.php in PHP 8.0 maybe it’s worth looking into it. But I think the test passes in github CI for php8. Maybe that will have to wait for php8.1 in CI.

come-nc commented 3 years ago

libxml_disable_entity_loader is deprecated but I do not understand what it does and why we call it. Leaving it in for now.

https://php.watch/versions/8.0/libxml_disable_entity_loader-deprecation

come-nc commented 3 years ago
come-nc commented 3 years ago

Opened https://github.com/nextcloud/server/issues/29731 about swiftmailer situation.

come-nc commented 2 years ago

8.1 is officially out. Highlights: https://www.php.net/releases/8.1/en.php Changelog: https://www.php.net/ChangeLog-8.php#8.1.0 Migration guide: https://www.php.net/manual/en/migration81.php

come-nc commented 2 years ago

Regarding

FILE: /home/mcmic/dev/nextcloud/server/lib/private/L10N/Factory.php
-----------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------------
 595 | ERROR | Function create_function() is deprecated since PHP 7.2 and removed since PHP 8.0; Use an anonymous function instead
 600 | ERROR | Function create_function() is deprecated since PHP 7.2 and removed since PHP 8.0; Use an anonymous function instead
-----------------------------------------------------------------------------------------------------------------------------------

It appears that both calls are in OC\L10N\Factory::createPluralFunction which is never called in core, and is not tested by FactoryTest.php.

Is the method safe to remove or may it be used by applications?

ChristophWurst commented 2 years ago

Seems unused https://github.com/search?q=org%3Anextcloud+createPluralFunction&type=code

Nevertheless this is a public API so we can't "just" remove it.

nickvergessen commented 2 years ago

Should have never been public in first place from my pov. Especially since https://github.com/nextcloud/server/pull/26375 I'm fine with removing directly or making it throw an exception instead

come-nc commented 2 years ago

Should have never been public in first place from my pov. Especially since #26375 I'm fine with removing directly or making it throw an exception instead

Can you create the PR for that then?

nickvergessen commented 2 years ago

Not atm, I'm on vacation until next year

txt-file commented 2 years ago

As debian started the php7.4 → php8.1 transition on 2021-12-31 I stumbled over nextclouds missing php8.1 support. I would be happy to drop the transitional php7.4 and move to 8.1 soon. :-)

bcutter commented 2 years ago

Even NC 22 with PHP 8.0 after updating to 8.1 (but not removing 8.0) already creates issues - like the cron job not working anymore.

Also OCC commands give

This version of Nextcloud is not compatible with > PHP 8.0.<br/>You are currently running 8.1.1.

So it seems like parts of NC already (try to) use PHP 8.1.

What a mess -.- Think I will need to downgrade PHP because NC is not fast enough.

Has NC 23 PHP 8.1 support?

Update: no obviously not according to https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html so upgrading NC to v23 is not an option. Need to stay on PHP 8.0... argh 😖

come-nc commented 2 years ago

Even NC 22 with PHP 8.0 after updating to 8.1 (but not removing 8.0) already creates issues - like the cron job not working anymore.

Also OCC commands give

This version of Nextcloud is not compatible with > PHP 8.0.<br/>You are currently running 8.1.1.

So it seems like parts of NC already (try to) use PHP 8.1.

What a mess -.- Think I will need to downgrade PHP because NC is not fast enough.

Has NC 23 PHP 8.1 support?

Update: no obviously not according to https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html so upgrading NC to v23 is not an option. Need to stay on PHP 8.0... argh confounded

PHP 8.1 is not supported by Nextcloud yet (which is why this issue is "open"). It should be supported by 24 when it’s out, maybe also by some future 22/23 minor versions, not sure yet.

If you have both 8.0 and 8.1 on your system, it’s your job to make sure you run Nextcloud and occ (and the cron job) with 8.0. Otherwise you will get the compatibility error as expected.

bcutter commented 2 years ago

Figured that out meanwhile too. Used "update-alternatives" to redirect NC to the 8.0 PHP instance for the time being.

pooh22 commented 2 years ago

the update-alternatives trick didn't work for me on ubuntu 20.04 with nextcloud 22 :-( so now what do I do?

my php version comes from deb http://ppa.launchpad.net/ondrej/php/ubuntu focal main

the apache config seems to refer to the php8.0 module in the mods installed, so why is this still failing? apache -> php 8.0 alternatives -> php8.0 ???

TheColin21 commented 2 years ago

the update-alternatives trick didn't work for me on ubuntu 20.04 with nextcloud 22 :-( so now what do I do?

my php version comes from deb http://ppa.launchpad.net/ondrej/php/ubuntu focal main

the apache config seems to refer to the php8.0 module in the mods installed, so why is this still failing? apache -> php 8.0 alternatives -> php8.0 ???

you may call the desired php version directly (e.g. by using php8.0 instead of php).

pooh22 commented 2 years ago

I fixed it by removing all php packages from the server and re-installing all required php8.0- packages manually according to the documentation. Notably the apcu package drew in some php7.4 versioned packages, so I switched the config to memcached (and removed the apcu and other 7.4 packages again) and then it worked. The nextcloud settings check suggested a few more php modules to install and now all is well again.

I'm not sure why specifically calling php8.0 (which I did try) didn't fix my problems.

The bigger question is, why did my setup fail when doing regular updates...? I suspect it was because some php packages were installed without a specified version, which automatically drew in the latest available php packages and dependencies.

I doubt I'm the only one running into this...

bcutter commented 2 years ago

I doubt I'm the only one running into this...

That's for sure. More to come and land here. E. g. because of https://github.com/nextcloud/server/issues/29287#issuecomment-1008320174.

Time's running - against us -.-

ruhlandm1 commented 2 years ago

If you have both 8.0 and 8.1 on your system, it’s your job to make sure you run Nextcloud and occ (and the cron job) with 8.0. Otherwise you will get the compatibility error as expected.

Have the same problem - can someone give me advice of how to make sure that those scripts are running for sure with php 8.0? In the cronjob I´ve set it up like this:

/5 * php8.0 -f /var/www/nextcloud/cron.php > /dev/null 2>&1

but I think this is more a workaround then a solution.

Neodraccir commented 2 years ago

When will 8.1 be supported? I tried to install Nextcloud yesterday and ran into this issue. Do I have to downgrade now to 8.0 (which would be a pain in the ass, since I also have to change many Nginx settings) just to upgrade it in a few weeks again?

nickvergessen commented 2 years ago

With the next major version Nextcloud 24.

dvzrv commented 2 years ago

With the next major version Nextcloud 24.

Is there an ETA for it already? :) For Arch Linux it is a bit of a problem to not be able to bump php a minor version unfortunately and I currently have to backport two sets of patches to somehow ensure compatibility.

yan12125 commented 2 years ago

Is there an ETA for it already? :)

The current plan is end of April - https://github.com/nextcloud/server/wiki/Maintenance-and-Release-Schedule#nextcloud-24

EDIT: the schedule for Nextcloud 24 is changed

nickvergessen commented 2 years ago

Don't do that. There can be errors along the way corrupting your data or database due to incompatible libraries and code. If it would be only this line to change you can be sure we did it already.

fafische commented 2 years ago

Archlinux provides php7.4 in parallel (https://archlinux.org/news/php-80-and-php-7-legacy-packages-are-available/). Now I am using php7.4 again because I do not want to skip php-updates for >2 month. It is really annoying with nextcloud devellopement being late for every single php release..

nickvergessen commented 2 years ago

It would be really annoying with bugs, data loss and corruption eith every Nextcloud release because we simply include a brand new php version that has not been tested in real world with any of the used libraries and Nextcloud itself.

Just look through this issue which libraties are mentioned to be updated and mysqli being broken in the first post.

CapSel commented 2 years ago

@fafische Archlinux have nextcloud in community repository together with php. They just patched 23.0.0 release with some beta patches found here and then they happily bumped up php to 8.1.2. They call it 23.0.0-4.

It's a bit frightening that they risk stability and data just to chase newest php version.

I believe that every php application have to be migrated every time new version of php version comes out. It's just luck or ignorance that it "just works" after some checks and warnings in code are ignored.

Proper verification if everything is working well on new version of language takes time.