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

Fixed in place sorting order bug for PHP version >= 8 #18131

Merged
merged 3 commits into from Oct 13, 2021

Conversation

AltamashShaikh
Copy link
Contributor

Description:

Fixed in place sorting order bug for PHP version >= 8
Fixes : #18130

Review

@AltamashShaikh AltamashShaikh added the Needs Review PRs that need a code review label Oct 11, 2021
@AltamashShaikh AltamashShaikh added this to the 4.6.0 milestone Oct 11, 2021
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it maybe better for this to try sorting on something other than 'name' if $a['name'] === $b['name'] to ensure consistent results?

@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 11, 2021
@AltamashShaikh
Copy link
Contributor Author

AltamashShaikh commented Oct 12, 2021

Is it maybe better for this to try sorting on something other than 'name' if $a['name'] === $b['name'] to ensure consistent results?

We have total 3 fields

  1. name (Eg: Visits)
  2. id (Eg: nb_visits)
  3. documentation

The next possible is id and that seems to work as expected.

Something like this

usort($metrics, function ($a, $b) {
            if ($a['name'] === $b['name']) {
                $value = strcmp($a['id'], $b['id']);
            } else {
                $value = strcmp($a['name'], $b['name']);
            }

            return $value;
        });

@tsteur
Copy link
Member

tsteur commented Oct 12, 2021

@AltamashShaikh seems some test is failing now in https://app.travis-ci.com/github/matomo-org/matomo/jobs/542640794#L760 and needs to be fixed.

same is also visible in the two glossary screenshots which need to be updated: https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/49950/

@AltamashShaikh
Copy link
Contributor Author

@AltamashShaikh seems some test is failing now in https://app.travis-ci.com/github/matomo-org/matomo/jobs/542640794#L760 and needs to be fixed.

same is also visible in the two glossary screenshots which need to be updated: https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/49950/

@tsteur Does it looks good to you now ?

@tsteur tsteur merged commit 68674d8 into 4.x-dev Oct 13, 2021
@tsteur tsteur deleted the getGlossaryMetrics_Sort_Fix branch October 13, 2021 03:50
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.

Fixed in place sorting order bug for PHP8+
3 participants