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

A few more PHP8.1 fixes #17989

Merged
merged 15 commits into from Sep 28, 2021
Merged

A few more PHP8.1 fixes #17989

merged 15 commits into from Sep 28, 2021

Conversation

geekdenz
Copy link
Contributor

@geekdenz geekdenz commented Sep 13, 2021

should not break anything and gets rid of a warning

Description:

refs #17686

Review

should not break anything and gets rid of a warning
@geekdenz geekdenz mentioned this pull request Sep 13, 2021
38 tasks
@geekdenz geekdenz linked an issue Sep 13, 2021 that may be closed by this pull request
38 tasks
@geekdenz geekdenz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 13, 2021
@geekdenz geekdenz marked this pull request as ready for review September 13, 2021 05:35
@geekdenz
Copy link
Contributor Author

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

@Findus23 Findus23 removed a link to an issue Sep 13, 2021
38 tasks
@geekdenz geekdenz linked an issue Sep 13, 2021 that may be closed by this pull request
38 tasks
@geekdenz geekdenz removed a link to an issue Sep 13, 2021
38 tasks
@geekdenz geekdenz added the Needs Review PRs that need a code review label Sep 15, 2021
@geekdenz geekdenz removed the Needs Review PRs that need a code review label Sep 16, 2021
@geekdenz geekdenz marked this pull request as draft September 16, 2021 02:39
@geekdenz
Copy link
Contributor Author

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 geekdenz changed the title add return type declartions #17686 add return type declartions and fix other php 8.1 deprecation warnings #17686 Sep 16, 2021
@geekdenz geekdenz marked this pull request as ready for review September 16, 2021 05:30
@geekdenz geekdenz added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Sep 16, 2021
@geekdenz geekdenz marked this pull request as draft September 16, 2021 05:31
@geekdenz
Copy link
Contributor Author

geekdenz commented Sep 16, 2021

$this->assertNull(DbHelper::getInstallVersion());

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

sgiehl commented Sep 16, 2021

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

tsteur commented Sep 16, 2021

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

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

e.g. here:

if (version_compare(DbHelper::getInstallVersion(),'4.0.0-b1', '<')) {

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

tsteur commented Sep 16, 2021

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

@tsteur fyi

image

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

@geekdenz geekdenz marked this pull request as ready for review September 17, 2021 02:02
@geekdenz geekdenz added the Needs Review PRs that need a code review label Sep 17, 2021
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Sep 25, 2021
@justinvelluppillai justinvelluppillai linked an issue Sep 28, 2021 that may be closed by this pull request
38 tasks
@justinvelluppillai justinvelluppillai changed the title add return type declartions and fix other php 8.1 deprecation warnings #17686 A few more PHP8.1 fixes Sep 28, 2021
@justinvelluppillai justinvelluppillai removed the Stale The label used by the Close Stale Issues action label Sep 28, 2021
@justinvelluppillai
Copy link
Contributor

These look ok to potentially merge.

Copy link
Member

@sgiehl sgiehl left a 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

@sgiehl sgiehl added this to the 4.6.0 milestone Sep 28, 2021
@sgiehl sgiehl merged commit 1bc9fdf into 4.x-dev Sep 28, 2021
@sgiehl sgiehl deleted the m-17686-simple-deprecated-fixes branch September 28, 2021 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants