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
A few more PHP8.1 fixes #17989
A few more PHP8.1 fixes #17989
Conversation
should not break anything and gets rid of a warning
This is still WIP but in case someone else picks this up later I wanted to make it visible. |
Just realised this can still be worked on. So, maybe wait with the review until I've done all simple fixes I can do for now. |
…rg/matomo into m-17686-simple-deprecated-fixes
test fails. I lazily return However, the test fails. I would just make it pass, but maybe I don't know the whole picture here, @tsteur , @diosmosis and @sgiehl . Could you have a think/look? |
Afaik this was added somewhen in 2018. So every installation done before might not hold the install version. Simply returning a default value doesn't seem good to me. That way you you loose the information somewhere else if it was recorded or not. But actually I don't have an overview for what that is currently used. Maybe @tsteur knows more. |
@geekdenz sorry I don't quite understand what you're meaning? Which |
@tsteur Sorry, I could have made this more clear: e.g. here: matomo/plugins/VisitsSummary/Reports/Get.php Line 203 in 32357a7
is used. I didn't test (which I should have) what it would currently return if null is passed. You are right, assigning a string is losing information, but it generates a deprecation warning in PHP 8.1 (null passed instead of string) and that seemed like the easiest fix. However, I wasn't sure it would cover all the bases, which is why I raised the question. |
👍 got it now. I suppose as the method is not used in any non-core plugin and looking at all the usages we could return |
@tsteur fyi So, you're right, |
…rg/matomo into m-17686-simple-deprecated-fixes
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
These look ok to potentially merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a rough look through the changes. As tests are still passing, guess it should be good to merge
should not break anything and gets rid of a warning
Description:
refs #17686
Review