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

Support PHP8.1 #29287

Closed
ChristophWurst opened this issue Oct 18, 2021 · 137 comments
Closed

Support PHP8.1 #29287

ChristophWurst opened this issue Oct 18, 2021 · 137 comments
Assignees

Comments

@ChristophWurst
Copy link
Member

ChristophWurst commented Oct 18, 2021

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

// Show warning if > PHP 8.0 is used as Nextcloud is not compatible with > PHP 8.0 for now
if (PHP_VERSION_ID >= 80100) {
http_response_code(500);
echo 'This version of Nextcloud is not compatible with > PHP 8.0.<br/>';
echo 'You are currently running ' . PHP_VERSION . '.';
exit(1);
}
.

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

@ChristophWurst ChristophWurst added enhancement 1. to develop Accepted and waiting to be taken care of labels Oct 18, 2021
@skjnldsv
Copy link
Member

cc @come-nc since you asked :)

@ChristophWurst
Copy link
Member Author

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

@come-nc
Copy link
Contributor

come-nc commented Oct 18, 2021

Hello.

I know at least that the use of LDAP resources will be a problem since there are calls to is_resource 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: PHPCompatibility/PHPCompatibility#1299

@come-nc
Copy link
Contributor

come-nc commented Oct 18, 2021

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
Copy link
Member

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
Copy link
Contributor

come-nc commented Oct 19, 2021

Needed for 8.1 support:

@come-nc
Copy link
Contributor

come-nc commented Oct 21, 2021

Does anyone know the reason for d690f90 ?

@ChristophWurst
Copy link
Member Author

Does anyone know the reason for d690f90 ?

#23780 (comment)

I suggest we revert ans see what happens :)

@come-nc
Copy link
Contributor

come-nc commented Oct 21, 2021

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
Copy link
Contributor

come-nc commented Oct 21, 2021

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
Copy link
Member

@LukasReschke will give you a lesson about it

@come-nc
Copy link
Contributor

come-nc commented Oct 21, 2021

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
Copy link
Contributor

come-nc commented Oct 21, 2021

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
Copy link
Contributor

come-nc commented Oct 25, 2021

opis/closure#107 (comment) (and related opis/closure#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
Copy link
Contributor

come-nc commented Oct 25, 2021

doctrine/dbal#4906 (comment) This is good news on the other hand :-)

@come-nc
Copy link
Contributor

come-nc commented Oct 25, 2021

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
Copy link
Member Author

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

That has already been addressed in #29344

@come-nc
Copy link
Contributor

come-nc commented Oct 25, 2021

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
Copy link
Member

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

@come-nc
Copy link
Contributor

come-nc commented Oct 25, 2021

@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
Copy link
Contributor

come-nc commented Oct 28, 2021

Fix pimple (or wait for a fix)

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

@come-nc come-nc self-assigned this Oct 28, 2021
@come-nc come-nc added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Oct 28, 2021
@jamiew0w
Copy link

hi guys and girls. is there any plans to help people stuck on nc22 upgrade to nc24? (IE an easier upgrade path)

@nanaya
Copy link

nanaya commented May 12, 2022

I haven't tried this but if you can get older php binary somehow in the system (phpenv, etc), maybe try running /path/to/bin/php7.4 updater/updater.phar from cli to update to version 24? (or whatever the php version and path)

@rdlab-upc
Copy link

rdlab-upc commented May 20, 2022

@jclsn @elyograg @jamiew0w @C0rn3j et al

Maybe this workaround will help for Ubuntu 22.04 and Nextcloud 22/23/24, at least our tests worked fine and clean. Ondrej (now) provides php 8.0 packages for Jammy 22.04 (https://launchpad.net/~ondrej/+archive/ubuntu/php)

HowTo add php8.0 under Ubuntu 22.04

  1. Add Ondrej repository
    sudo add-apt-repository ppa:ondrej/php

  2. Update sources

sudo apt update

  1. Install php8.0 with desired modules

apt-get install php8.0 php8.0-cli php8.0-fpm .....

  1. (OPTIONAL) make php8 default in your system (for cron/cli executions)

sudo update-alternatives --config php

P.S: Probably you need to change/restart the fastcgi_pass unix:/run/php/php-fpm.sock; in your apache/nginx configuration to point out to /var/run/php/php8.0-fpm.sock

P.S2: You can also delete all php-8.1 system packages if no more system dependencies exist, but IMHO is better having both versions available in the system.

@AndyScherzinger @pooh22 First of all a big thank you and real acknowledge to the NC team, great job!. It is plain to see the big effort you made this past months.

Said so, IMHO, this issue could be addressed more politely from the developer team. (some ;) Claims are nicely motivated and explained and replies like "So I keep my fingers crossed looking at 8.2 of PHP that we won't run into the same scenario and all the used libraries will support it right from the start but there is no guarantee." do not provide trustworthiness in this project.

I know and understand that this is a community initiative lead by NC staff, so the community (we would like to believe we belong to) must actively contribute and not only complain about "my own needs". This is why we share this "howto" in order to help address this problem.

Sorry for the looong post ;)

Thank you!

@notandxor
Copy link

Got bitten by this by a fedora 36 upgrade. Downgardig to php 8.0, this saved my day:
https://rpms.remirepo.net/wizard/

@xxblx
Copy link

xxblx commented May 22, 2022

Thanks @notandxor! It's also possible to install php 8.0 as a separate version and use it for upgrading only, i.e., without downgrading php system-wide.

Workaround for Fedora 36 - enabling remi and installing php 8.0 in parallel with php 8.1

dnf install https://rpms.remirepo.net/fedora/remi-release-36.rpm
dnf config-manager --set-enabled remi
# check package names and adapt to match your setup (postgres / mysql, etc.)
dnf install php80 php80-php-pgsql php80-php-pecl-apcu php80-php-intl php80-php-intl php80-php-gd php80-php-xml php80-php-pdo php80-php-process php80-php-pecl-zip php80-php-mbstring

Once php 8.0 is installed, do manual upgrade with php80. I applied --define as I didn't configure php80 because I was not going to keep it after the upgrade.

php80 --define apc.enable_cli=1 occ upgrade

I upgraded 22.2.5 => 22.2.8 => 23.0.5 with php80 and after that upgraded nc to 24.0.1 using default php from the fedora repository. So far 24.0.1 has been working fine with php 8.1.

@blacklight
Copy link

blacklight commented May 25, 2022

I've kept my php* packages on hold on Arch Linux for way too long (while waiting for NC 24 to be rolled out, hopefully with PHP 8.1 support). And this is starting to affect other occ commands that can no longer find the versions of the libraries that they expect:

PHP Warning:  PHP Startup: Unable to load dynamic library 'intl' (tried: /usr/lib/php/modules/intl (/usr/lib/php/modules/intl: cannot open shared object file: No such file or directory), /usr/lib/php/modules/intl.so (libicudata.so.70: cannot open shared object file: No such file or directory)) in Unknown on line 0

Note that Arch Linux doesn't support partial upgrades, so this was expected to happen at some point. I just hoped that I didn't have to hold on the PHP system packages for ~7 months, and now the system itself is starting to fall apart.

How am I supposed to tackle this when the upgrade to NC 24 comes to my instance, since occ commands are most likely to fail because some of the dependencies that they rely on have been upgraded?

As a lesson for next time, could you guys start testing NC against the upcoming version of PHP before the new version is rolled out to all the systems out there, instead of being caught by surprise every time, and rush to get a fix after the first issues have already been open?

@nettybun
Copy link

nettybun commented May 25, 2022

@blacklight you should not have needed to hold back PHP versions, as Arch patched their Nextcloud 23 to support PHP 8.1 months ago beginning in January archlinux/svntogit-community@c78893c

Also mentioned at the start of https://wiki.archlinux.org/title/Nextcloud

@zynexiz
Copy link

zynexiz commented May 25, 2022

@blacklight Had my PHP on hold to for a while, but upgraded to v24.0.0 a week or two ago, released the hold and upgraded everything. It all works very good again now, and think actually NC got a bit faster (feels snappier anyway). Just updated to 24.0.1 yesterday, still running good. I don't use NC package from repo thou because of my structure on my web server and stuff.

@blacklight
Copy link

@blacklight you should not have needed to hold back PHP versions, as Arch patched their Nextcloud 23 to support PHP 8.1 months ago beginning in January archlinux/svntogit-community@c78893c

I don't have the NC package installed via pacman. I installed it directly in my Apache directory because of quirks/customizations with my server. And the upgrade to NC 24 hasn't yet come in my mailbox. And when it comes, it's likely to break because of the mismatch between expected/available dependencies of PHP. What shall I do in this case? A few options:

  • Install the php80* AUR versions for all the PHP packages?
  • Upgrade the system php packages and cross my fingers?
  • Do a fresh installation?

@zynexiz
Copy link

zynexiz commented May 25, 2022

@blacklight You should be able to upgrade thru the UI (or shell), v24.0.0 has been there for a while now (and also v24.0.1, at least for the stable release). I did my upgrade a couple of weeks ago. Can confirm that it works with latest PHP in arch repo. I upgraded NC first, then released the packages and did pacman -Syyu on everything.

@nariox
Copy link

nariox commented May 25, 2022 via email

@AndyScherzinger
Copy link
Member

@blacklight if the updater doesn't offer v24 yet, just switch to the beta channel, than it will pop-up, upgrade to 24, then switch back to stable. This way you can work around the staged roll-out.

@rdlab-upc regarding the trustworthiness I feel the need to reply to that and clearify this a bit more. Also thank you for your detailed comment and guidance for others. We do our best to prevent such situations, see i.e. nextcloud/groupware#40 where we try to catch such things early on. Like I stated Nextcloud like many software systems nowadays depend on 3rd party libraries to implement their functionality i.e. horde and sabreDAV. So when the 3rd party libraries aren't yet compatible then also Nextcloud is stuck with the defined PHP versions. Because of that we do document the versions Nextcloud is compatible with. Sometimes you are lucky and can work around incompatibilities in libraries, sometimes you can't. So there is nothing you as a dev can do about this.
That is why I basically stated that Nextcloud can't guarantee compatibility with new php versions on day one but will always try do achieve that. So I rather find such a statement trustworthy compared to stating "yeah, we will" even though technically speaking we can't ensure this except also taking over all the used libs.

@blacklight
Copy link

blacklight commented May 26, 2022

@AndyScherzinger thanks for the instructions. I have eventually managed to upgrade manually (downloaded the latest zip to the Apache dir and ran occ upgrade). Then after upgrading to NC 24 I upgraded PHP to 8.1 and everything went fine. I was afraid that some of the missing system dependencies would break the occ upgrade process, but luckily the only PHP module that was broken (because it relied on the libcuuc* libraries that were upgraded) was intl, which apparently is not mandatory for the upgrade to run.

@AndyScherzinger
Copy link
Member

My pleasure @blacklight and happy to hear you arrived on 24 safely 👍

@VVD
Copy link

VVD commented Jun 27, 2022

Several errors:

strtolower(): Passing null to parameter #1 ($string) of type string is deprecated at /nextcloud/3rdparty/swiftmailer/swiftmailer/lib/classes/Swift/Transport/EsmtpTransport.php#144

3rdparty/swiftmailer/swiftmailer/lib/classes/Swift/Transport/EsmtpTransport.php#144:

-$encryption = strtolower($encryption);
+$encryption = strtolower($encryption ?? '');
str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated at /nextcloud/apps/mail/vendor/bytestream/horde-mime/lib/Horde/Mime/Headers/Element.php#145

apps/mail/vendor/bytestream/horde-mime/lib/Horde/Mime/Headers/Element.php#145:

-return str_replace("\0", '', $data);
+return str_replace("\0", '', $data ?? '');

@AndyScherzinger
Copy link
Member

@VVD please raise this at https://github.com/nextcloud/mail/issues including the version infos server/mail-app

@VVD
Copy link

VVD commented Jun 27, 2022

Can't find files EsmtpTransport.php and Element.php in sources of the https://github.com/nextcloud/mail/.
Both are something else.

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Jun 27, 2022

@VVD
Copy link

VVD commented Jun 27, 2022

Thanks for information.

3rdparty/swiftmailer

https://github.com/nextcloud/3rdparty / https://packagist.org/packages/swiftmailer/swiftmailer

This package is abandoned and no longer maintained.
The author suggests using the [symfony/mailer](https://packagist.org/packages/symfony/mailer) package instead.

Found commit with PHP8.1 support: swiftmailer/swiftmailer@ee8a1d9
It have "my" patch too.

bytestream/horde-mime

https://github.com/bytestream/Mime, addressed in bytestream/Mime#3

Already fixed, but when we can see it in our installation? What are the plans?

@VVD
Copy link

VVD commented Jul 19, 2022

During upgrade from 24.0.2 to 24.0.3 I got error:

<b>Deprecated</b>:  Return type of RecursiveDirectoryIteratorWithoutData::accept() should either be compatible with FilterIterator::accept(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in <b>/opt/nextcloud/updater/index.php</b> on line <b>40</b><br />
{"proceed":true}

Patch:

--- updater/index.php.orig
+++ updater/index.php
@@ -37,7 +37,7 @@
 }
 
 class RecursiveDirectoryIteratorWithoutData extends \RecursiveFilterIterator {
-       public function accept() {
+       public function accept(): bool {
                /** @var \DirectoryIterator $this */
                $excludes = [
                        '.rnd',

@AndyScherzinger
Copy link
Member

@VVD can you please raise a new issue for this one, thanks, your effort is highly appreciated, cc @PVince81

@VVD
Copy link

VVD commented Jul 20, 2022

@AndyScherzinger, #33295

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Jul 20, 2022
(nextcloud/server#29287)

This version of Nextcloud is not compatible with PHP>=8.2.
You are currently running 8.2.0alpha1.

Reported by:	Kevin Day (kevin@your.org)
mickenordin added a commit to SUNET/nextcloud-custom that referenced this issue Aug 22, 2022
Support issue is closed as fixed:
nextcloud/server#29287
@creopard
Copy link

creopard commented Nov 9, 2022

Several errors:

strtolower(): Passing null to parameter #1 ($string) of type string is deprecated at /nextcloud/3rdparty/swiftmailer/swiftmailer/lib/classes/Swift/Transport/EsmtpTransport.php#144

3rdparty/swiftmailer/swiftmailer/lib/classes/Swift/Transport/EsmtpTransport.php#144:

-$encryption = strtolower($encryption);
+$encryption = strtolower($encryption ?? '');

@ChristophWurst (cc @AndyScherzinger )
The above issue with swiftmailer found by @VVD is still open in Nextcloud 25.0.1 and pops up regularely in the NC error log when sending an email.

What about the existing fixes swiftmailer/swiftmailer@ee8a1d9 that were not yet integrated into https://github.com/nextcloud/3rdparty/tree/master/swiftmailer/swiftmailer ?

@ChristophWurst
Copy link
Member Author

Could you open that as a separate ticket please @creopard? Then one of the server engineers can have a look.

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

No branches or pull requests