Make Matomo compatible with PHP8.1 #17686
Comments
Findus23
added
the
Task
|
Some more unsorted warnings: Comment by @sgiehl: This one is fixed ✔️ |
Findus23
added
the
Help wanted
|
matomo.log gets really large really quickly as there are many deprecation warnings that are called many times per request. So I wrote a quick script to deduplicate all warnings to make it easier to see them: logging.pyimport re
basepath = "/home/lukas/public_html/matomophp8/"
regex = re.compile(r"\[[\d\w]+\] ([/\w\-\.()]+):(.+) - Matomo 4.4.+ ((?:(?:#0)|(?:\[internal function\])).*)")
warnings = {}
with open("tmp/logs/matomo.log") as f:
for line in f:
if not line.startswith("WARNING"):
continue
# print(line)
result = regex.findall(line)
if not result:
print(line)
print(result)
continue
path, warning, trace = result[0]
trace = trace.replace("#","$").split(",")
warnings[path] = (warning, trace) # overwrites warnings in the same line
warnings = dict(sorted(warnings.items(), key=lambda item: item[0]))
for path, data in warnings.items():
warning, trace = data
path = path.replace(basepath, "").strip()
print("- [ ]", path)
print(">", warning.strip())
for line in trace:
print(">",line)
file = path.split("(")[0]
lineno = path.split("(")[-1][:-1]
if "vendor" not in file:
print("> "
"https://github.com/matomo-org/matomo/"
"blob/99136cbf3183833b5d9dedb7943873fb8c3a8da5/"
f"{file}#L{lineno}")
print()
print(len(warnings), "unique warnings")I'll add all new warnings I find in the issue above (now also with stack trace as I think some of them are only fixable when knowing where the data comes from). |
|
Is it better to make a number of smaller PRs to get these fixes merged or combine them all into one? I've made a few fixes in #17929, there are also many plugin PRs coming. |
|
@justinvelluppillai small ones be a lot better. |
should not break anything and gets rid of a warning
|
Is this too small or a good size? I could simply add more commits like this to not make too many PRs. |
|
It's hard to say re too small or good size @geekdenz . You want to put things that fix the same kind of issue into the same PR. Like if |
|
OK. Thanks @tsteur . I recommend we leave this until at least root@42b8ea256385:/usr/src/myapp# pecl install xdebug
pecl/xdebug requires PHP (version >= 7.2.0, version <= 8.0.99), installed version is 8.1.0RC1
No valid packages found
install failedI could not login and it seemed to be in a loop which is hard to debug without a debugger. |
Basics WorkI gave it another shot without running the VisitorGenerator plugin and basics seem to work now. :-)
|
|
My take on "too small" would be to limit the number of PRs, but make a separate commit per fix.
I think the basics (as in clicking around in the Matomo UI) already worked before I made any changes. But I might be misremembering something. |
It's using one's best judgement what is too small or too big i guess.
Yeah, just had a quick look at the ui which seems to work. The main problem is probably the tracker. |
|
When running tests locally there occurs a Which we can't fix without updating symfony console (if it's already fixed there). --> will be fixed with #18328 |
|
After fresh installation (with existing database), system check page works well but then after clicking next following error appears in my apache2 error.log Installed packages: |
|
@WebWire-NL Just to be sure: This happens in the installation process after the system check, right? Could you check if a config file was already created and if so which plugins are listed as active within in? |
|
@sgiehl Hi, i'm sorry but i reverted to php7.4 :( i can't check anymore.... but i guess nothing was done as the server responded with a 500 error. The error appears after the system check page... so system check only gives a warning about git because i tried the latest version.... and then if you try to go to the next step a 500 error will occur and this was what i could find in the server logs. |
|
@WebWire-NL Which version did you try to install on PHP 8.1? A latest git checkout or a specific release? |
|
I tried branch 4.6.2 and another Branch (might be even the master branch?)
which both had last change date of 2 days ago....
Op ma 29 nov. 2021 11:50 schreef Stefan Giehl ***@***.***>:
… @WebWire-NL <https://github.com/WebWire-NL> Which version did you try to
install on PHP 8.1? A latest git checkout or a specific release?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17686 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD5FZL7MKBNWWBDPCKNOGVTUONLHFANCNFSM464PY6OQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Got WARNING on stripos() (4.6.1) |
|
From https://forum.matomo.org/t/container-php-68-deprecated-container-php-73-deprecated/43930 on page index.php?module=MultiSites&action=index&idSite=1&period=range&date=previous30
on page: /index.php?module=Installation&action=systemCheckPage&idSite=1&period=range&date=previous30&showtitle=1&random=1227
|
|
@sgiehl does one of your last updates fix #17686 (comment) ? |
|
@heurteph-ei That should have been fixed here: https://github.com/matomo-org/matomo/pull/18330/files#diff-71a1671fa6d1cb22b6c82a3eeb8f845616ad6e8b916070267ba75025d5f08d48R156 I guess meanwhile most issues should be sorted out. At least those that were reported here or I was able to find when running the tests on PHP 8.1. There might still be some issues in third party libs we are using, but most of them published updates as well. |
|
In PHP8.1 on Debian I'm seeing the same dependency injection explosion mentioned in #17686 (comment) Is this known? Should I file a separate issue? What additional details do you need? Details(double-escaping due to logging setup, sorry) |
|
@thcipriani Where exactly did that error occur? After updating an existing instance or when trying to install a new one? |
Happened when "updating" an existing install. Debian sid swapped out the |
|
I'll close this one now. Wasn't able to find any other issues while clicking randomly through the UI or while running tests on PHP 8.1. There for sure might be some issues somewhere that didn't pop up yet, or that are introduced with new code later. But guess it would be good to handle those in new issues, so we are better able to track them down. |
|
This issue has been mentioned on Matomo forums. There might be relevant details there: https://forum.matomo.org/t/matomo-4-8-0-upgrade-warning-for-htmlspecialchars-using-php8-1/45056/3 |
|
@sgiehl it seems some |
The first alpha of php8.1 was just released:
https://www.php.net/archive/2021.php#2021-06-10-1
There are a few breaking changes, so it might be useful to already start fixing things to get them released in time.
Changes: https://github.com/php/php-src/blob/php-8.1.0alpha1/UPGRADING
The major breaking change is that built-in functions now (just like user-defined functions) no longer accept null in non-nullable arguments:
https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg
Click to show fixed issues
https://github.com/Seldaek/monolog/pull/15541.26.1./console core:updatematomo/plugins/GeoIp2/LocationProvider/GeoIp2/Php.php
Line 256 in c973567
matomo/core/Access.php
Line 164 in ea561b5
Update PHPMailer to at least 6.5.1 (first version with provisional support for PHP 8.1)
Iterators
matomo/libs/Zend/Db/Statement/Pdo.php
Lines 262 to 270 in f21acfe
$this->_stmtwhich can be null is notTraversable.https://github.com/twigphp/Twig/pull/3552(no release yet)$idSite == 'all'and use$idSite === 'all'and similar ones see https://3v4l.org/17cZ2to be updated...
The text was updated successfully, but these errors were encountered: