@geekdenz opened this Pull Request on September 13th 2021 Contributor

should not break anything and gets rid of a warning

Description:

Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.

Review

@geekdenz commented on September 13th 2021 Contributor

This is still WIP but in case someone else picks this up later I wanted to make it visible.

@geekdenz commented on September 16th 2021 Contributor

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.

@geekdenz commented on September 16th 2021 Contributor

https://github.com/matomo-org/matomo/blob/6029f2c118c6ce3af4ef9b4e35a230b2faed744b/tests/PHPUnit/Integration/DbHelperTest.php#L43
test fails.

I lazily return '1' here as null to me means it is earlier than anything else and it is elsewhere compared to another version string with version_compare(). I was considering '0' or just '' as well. Maybe '0' or even '0.0.0' is best?

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?

@sgiehl commented on September 16th 2021 Member

I lazily return '1' here as null to me means it is earlier than anything else and it is elsewhere compared to another version string with version_compare(). I was considering '0' or just '' as well. Maybe '0' or even '0.0.0' is best?

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.

@tsteur commented on September 16th 2021 Member

@geekdenz sorry I don't quite understand what you're meaning? Which version_compare are you referring to? Generally for DbHelper::getInstallVersion() we still want to return null when we don't know when it was installed. That can be important to know compared to assuming version 1. Which would be for example shown in the system check potentially etc and is wrong information.

@geekdenz commented on September 16th 2021 Contributor

@tsteur Sorry, I could have made this more clear:

e.g. here:

https://github.com/matomo-org/matomo/blob/32357a76ceaab427876826058a114df7b3e39a27/plugins/VisitsSummary/Reports/Get.php#L203

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.

@tsteur commented on September 16th 2021 Member

👍 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 '0' not '1'. That way all the !empty() checks on the returned version won't break and I suppose version_compare still works

@geekdenz commented on September 16th 2021 Contributor

@tsteur fyi

image

So, you're right, '0' would be best. 👍

@github-actions[bot] commented on September 25th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

Powered by GitHub Issue Mirror