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

Fixing unsafe comparison warnings (== => ===, != => !==) #18195

Closed
wants to merge 38 commits into from

Conversation

hywak
Copy link

@hywak hywak commented Oct 21, 2021

Description:

#17686

Lot's of files are changed, but those are cosmetics changes only.

Review

@hywak hywak changed the title First bunch of fixes to see wether tests are passing Fixing unsafe comparison warnings (== => ===, != => !==) Oct 21, 2021
… comparison

* changed if condition to check wether strpos response is different to false
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.

Left a couple of comments. In general it might not be that easy to change == to === in all cases. Unfortunately our travis tests are currently not running as expected. As soon as they will work again we can trigger a new build to see how many tests are failing...

core/API/DataTableManipulator/LabelFilter.php Outdated Show resolved Hide resolved
core/ArchiveProcessor/Loader.php Outdated Show resolved Hide resolved
core/AssetManager.php Outdated Show resolved Hide resolved
core/AssetManager.php Outdated Show resolved Hide resolved
core/CliMulti.php Outdated Show resolved Hide resolved
core/Piwik.php Outdated Show resolved Hide resolved
core/Plugin/ControllerAdmin.php Outdated Show resolved Hide resolved
core/ProxyHeaders.php Outdated Show resolved Hide resolved
core/Settings/Storage/Backend/MeasurableSettingsTable.php Outdated Show resolved Hide resolved
core/Tracker/Model.php Outdated Show resolved Hide resolved
hywak and others added 8 commits October 21, 2021 15:12
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
core/Db.php Outdated Show resolved Hide resolved
core/Db/Adapter/Mysqli.php Outdated Show resolved Hide resolved
…erly. Added casting to int/string to pass strict comparisons
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 another look through the changes.

core/Settings/Storage/Backend/MeasurableSettingsTable.php Outdated Show resolved Hide resolved
core/Settings/Storage/Backend/PluginSettingsTable.php Outdated Show resolved Hide resolved
core/Settings/Storage/Backend/PluginSettingsTable.php Outdated Show resolved Hide resolved
core/Tracker/Model.php Outdated Show resolved Hide resolved
core/Tracker/GoalManager.php Outdated Show resolved Hide resolved
core/DataAccess/ArchiveSelector.php Outdated Show resolved Hide resolved
core/DataAccess/ArchiveSelector.php Outdated Show resolved Hide resolved
core/DataAccess/ArchiveSelector.php Outdated Show resolved Hide resolved
core/CronArchive/Performance/Logger.php Outdated Show resolved Hide resolved
core/API/DataTablePostProcessor.php Outdated Show resolved Hide resolved
hywak and others added 12 commits October 22, 2021 12:28
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
hywak and others added 7 commits October 22, 2021 12:34
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
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.

Found a couple more things. But besides that it's looking fine. We may need to wait until mid of next week till the travis tests are running again. Once we had them running we can check if anything causes problems....

core/API/DataTablePostProcessor.php Outdated Show resolved Hide resolved
core/API/DataTablePostProcessor.php Outdated Show resolved Hide resolved
core/Http.php Show resolved Hide resolved
core/Http.php Outdated Show resolved Hide resolved
hywak and others added 3 commits October 25, 2021 08:53
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@sgiehl
Copy link
Member

sgiehl commented Oct 26, 2021

@hywak would you mind merging in the latest changes from 4.x-dev branch. Travis builds should be up and running again, so we hopefully can see afterwards if all those changes break anything or not.

@sgiehl
Copy link
Member

sgiehl commented Oct 28, 2021

@hywak Seems there are a plenty of tests failing. Might be good to first looking into why the Unit tests are failing, maybe fixing those might also fix some of the other test suites:
https://app.travis-ci.com/github/matomo-org/matomo/jobs/545181203
I'm not sure when I will find some time to have a quick look myself to maybe identify some first issues, but will try to do that soon.

core/Common.php Outdated Show resolved Hide resolved
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@hywak
Copy link
Author

hywak commented Oct 28, 2021

@sgiehl I will take a look at it tomorrow or at the weekend :)

@hywak
Copy link
Author

hywak commented Oct 29, 2021

@sgiehl Is there documentation on how to run tests locally?

@sgiehl
Copy link
Member

sgiehl commented Oct 29, 2021

@hywak sure. See https://developer.matomo.org/guides/tests
If you have any problems with that, feel free to ask.

@hywak
Copy link
Author

hywak commented Oct 29, 2021

Pushed changes where all unit tests were passing locally: 45a3aa5

@sgiehl
Copy link
Member

sgiehl commented Oct 29, 2021

The System tests are still failing though. Seems to be caused by some kind of rounding issue. Guess some check doesn't do the same as before...

@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Nov 13, 2021
@github-actions
Copy link
Contributor

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale for long The label used by the Close Stale Issues action label Dec 26, 2021
@sgiehl
Copy link
Member

sgiehl commented Feb 18, 2022

@hywak I will close this one now. I don't think we will be able to easily get this PR into a state where we can safely merge. The number of changes is simply to high to identify which are causing the test failures.
If you are still keen on contributing to Matomo, it would be very helpful to create smaller pull requests with the same changes instead. If only a couple of files are changed in each it's a lot easier to find the cause for failing tests and we will be a lot more faster in merging it...

@sgiehl sgiehl closed this Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale for long The label used by the Close Stale Issues action Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants