@ThaDafinser opened this Pull Request on February 29th 2016 Contributor
@ThaDafinser commented on February 29th 2016 Contributor

Starting with 66 failures in SystemTests
https://travis-ci.org/piwik/piwik/jobs/112586859#L7636

@ThaDafinser commented on March 1st 2016 Contributor

@tsteur @mattab since i started to work on fixing the issues of the sorting between PHP5 / 7 i got down from 66 failures to 32 failures (1 failure in submodule is also fixed, so 31).

I only changed this sorting method and fixed the expected results
https://github.com/piwik/piwik/pull/9858/files#diff-2d0bb82438495c9259aa4ef3211b6b36

Now it sorts by the count and then the label, instead of only the count.

_My question_
Is this way good for you? Or is this a problem, since it will need a little bit performance and the result changes a bit?
If the way is fine for you, i will fix this for PHP 5.3 and we can merge it and continue to work on fixes for the missing 32 failures.

@tsteur commented on March 6th 2016 Owner

Is this way good for you? Or is this a problem, since it will need a little bit performance and the result changes a bit?

It will make things slower, but I think there is no way around as we have to run tests on PHP 7 as well. Maybe it'll be faster in some other cases where we can also use the index position maybe somehow. I reckon the only case where performance re sorting matters most is here: https://github.com/piwik/piwik/blob/39f6964dbfd3b66520dd04728b64cdc856a532c3/core/DataTable/Filter/Sort.php#L250-L289 It shouldn't add too much overhead though.

If the way is fine for you, i will fix this for PHP 5.3 and we can merge it and continue to work on fixes for the missing 32 failures.

Yes, it's awesome you're working on this. I'm looking forward to merge this once tests are green.

@mattab commented on September 23rd 2016 Owner

Hi @ThaDafinser - Thanks for the PR. Would you be interested to continue the work on this so we can merge it and run our CI job AllTests on PHP7? Would be awesome to have before the 3.0.0 release :+1: let us know if we can help

@ThaDafinser commented on September 27th 2016 Contributor

Since i started studiing, i'm afraid i can't complete it the next weeks.

The PR would also need a complete rebase - i think best is to open a complete new one and take some parts out of it.

@mattab commented on September 27th 2016 Owner

Thanks for the update @ThaDafinser - all the best for your studies!

Created this issue: https://github.com/piwik/piwik/issues/10586
Hopefully someone else will help and finish your nice work :+1:

This Pull Request was closed on September 27th 2016
Powered by GitHub Issue Mirror