[Finder] Fix wrong implementation on sortable callback comparator #10849

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

ProPheT777 commented May 3, 2014

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR no

Working on this issue, i find SORT_BY_ACCESSED_TIME, SORT_BY_CHANGED_TIME, SORT_BY_MODIFIED_TIME was not tested.

  • Add test for SORT_BY_ACCESSED_TIME case
  • Add test for SORT_BY_CHANGED_TIME case
  • add test for SORT_BY_MODIFIED_TIME case

ProPheT777 added some commits May 3, 2014

@ProPheT777 ProPheT777 Fix wrong implementation on callback comparator e683c59
@ProPheT777 ProPheT777 Fix CS
ec48913
Contributor

ProPheT777 commented May 3, 2014

ProPheT777 changed the title from [Finder] Fix wrong implementation on callback comparator to [Finder] Fix wrong implementation on sortable callback comparator May 3, 2014

Contributor

ProPheT777 commented May 3, 2014

Do you think strcmp($a->getRealpath(), $b->getRealpath()) < 0 ? -1 : 1; is useful or return directly is enough ?

@romainneutron romainneutron commented on an outdated diff May 3, 2014

...ymfony/Component/Finder/Iterator/SortableIterator.php
@@ -41,7 +41,9 @@ public function __construct(\Traversable $iterator, $sort)
if (self::SORT_BY_NAME === $sort) {
$this->sort = function ($a, $b) {
- return strcmp($a->getRealpath(), $b->getRealpath());
+
+ //strcmp returns < 0 if str1 is less than str2; > 0 if str1 is greater than str2, and 0 if they are equal.
+ return strcmp($a->getRealpath(), $b->getRealpath()) < 0 ? -1 : 1;
@romainneutron

romainneutron May 3, 2014

Member

you should return 0 if strcmp returns 0

@ProPheT777 ProPheT777 Return 0 if strcmp return 0
9a400f5

To make it more readable, I would use the difference operator:

return ($a->getATime() - $b->getATime()) > 0 ? 1 : -1;

Contributor

ProPheT777 commented May 3, 2014

Right for yoda condition, but, i dont think we need to check the type, == is enough, because php api say : int strcmp ( string $str1 , string $str2 ) ?

Member

romainneutron commented May 3, 2014

you should always use typed comparisons in symfony except if it's on purpose

Contributor

ProPheT777 commented May 3, 2014

@staabm staabm and 1 other commented on an outdated diff May 3, 2014

...ymfony/Component/Finder/Iterator/SortableIterator.php
};
} elseif (self::SORT_BY_CHANGED_TIME === $sort) {
$this->sort = function ($a, $b) {
- return ($a->getCTime() > $b->getCTime());
+ if (($a->getCTime() === $b->getCTime())) {
+ return 0;
+ }
+
+ return ($a->getCTime() > $b->getCTime()) > 0 ? 1 : -1;
@staabm

staabm May 3, 2014

Contributor

Typo, should be - instead of > on first occurence

@ProPheT777

ProPheT777 May 3, 2014

Contributor

nice catch, i forgot it

@stof stof and 1 other commented on an outdated diff May 3, 2014

...ymfony/Component/Finder/Iterator/SortableIterator.php
@@ -51,19 +59,38 @@ public function __construct(\Traversable $iterator, $sort)
return 1;
}
- return strcmp($a->getRealpath(), $b->getRealpath());
+ //strcmp returns < 0 if str1 is less than str2; > 0 if str1 is greater than str2, and 0 if they are equal.
+ $result = strcmp($a->getRealpath(), $b->getRealpath()) < 0 ? -1 : 1;
@stof

stof May 3, 2014

Member

this change is not needed. the sort algorithm deals with == 0, >0 and <0. It does not need 1 and -1 explicitly. strcmp is meant to be used for this case precisely

@stof

stof May 3, 2014

Member

btw, this means that when sorting integers, the easiest implementation of the sort function is to return the difference

@ProPheT777

ProPheT777 May 3, 2014

Contributor

Do you think strcmp($a->getRealpath(), $b->getRealpath()) < 0 ? -1 : 1; is useful or return directly is enough ?

I was not sure, but you right, in the doc they show an example where they send direcly the result of strcmp.

@stof stof commented on an outdated diff May 3, 2014

...ymfony/Component/Finder/Iterator/SortableIterator.php
@@ -41,7 +41,15 @@ public function __construct(\Traversable $iterator, $sort)
if (self::SORT_BY_NAME === $sort) {
$this->sort = function ($a, $b) {
- return strcmp($a->getRealpath(), $b->getRealpath());
+
+ //strcmp returns < 0 if str1 is less than str2; > 0 if str1 is greater than str2, and 0 if they are equal.
+ $result = strcmp($a->getRealpath(), $b->getRealpath());
@stof

stof May 3, 2014

Member

should be reverted as well

@stof stof commented on an outdated diff May 3, 2014

...ymfony/Component/Finder/Iterator/SortableIterator.php
@@ -30,8 +30,8 @@ class SortableIterator implements \IteratorAggregate
/**
* Constructor.
*
- * @param \Traversable $iterator The Iterator to filter
- * @param int|callback $sort The sort type (SORT_BY_NAME, SORT_BY_TYPE, or a PHP callback)
+ * @param \Traversable $iterator The Iterator to filter
+ * @param int|callback $sort The sort type (SORT_BY_NAME, SORT_BY_TYPE, or a PHP callback)
@stof

stof May 3, 2014

Member

please change it from callback to callable, which is the proper type in PHP

@ProPheT777 ProPheT777 Improve readability
02cd235

ProPheT777 changed the title from [Finder] Fix wrong implementation on sortable callback comparator to [WIP][Finder] Fix wrong implementation on sortable callback comparator May 9, 2014

Contributor

ProPheT777 commented May 9, 2014

@hhamon I think you can backport the comparator implementation in your PR, she has not move since one week so. I start to write new test next week.

Owner

fabpot commented May 12, 2014

As this is a bug fix, I'd like to merge this PR independently of any other ones. Can you add some tests before I merge it though? Thanks.

Contributor

ProPheT777 commented May 16, 2014

The fix for wrong implementation is already covered by unit test, but indeed as I said in the description of the PR I'll add more test. I think I will do it this week end.

Member

stof commented May 16, 2014

It is not covered by unit tests, otherwise they would be failing. the current testsuite is passing in 2.3, which means it does not enforce a right implementation

Contributor

ProPheT777 commented May 16, 2014

Yes my bad, SORT_BY_NAME and SORT_BY_TYPE are not affected by this issue.

ProPheT777 added some commits May 18, 2014

@ProPheT777 ProPheT777 Add test coverage for file date sorting
6c4589b
@ProPheT777 ProPheT777 Improve sleep time to trigger change on metadata file, try to fix tra…
…vis build
52c8781
@ProPheT777 ProPheT777 Fix travis build
6de4487
Contributor

ProPheT777 commented May 18, 2014

Done, I met several trouble with taking effect on update system files so I sleep(1) each file operation.

If you have better idea.

Contributor

ProPheT777 commented May 19, 2014

@fabpot i saw your message in my mails but he not appear on the PR wall, weird.

Yes I have tried usleep with different value, until 99999 no change is taken, I think PHP have a threshold at 1 sec to refresh the state of files.

Member

stof commented May 19, 2014

have you tried using clearstatcache ?

@ProPheT777 ProPheT777 Improve test
4be3154
Contributor

ProPheT777 commented May 20, 2014

Yes, I tried, but nothing changes, I avoid triggering the sleep test that do not need.

  • Less sleep time (one time trigger)
  • Only sleep on related test

ProPheT777 added some commits May 20, 2014

@ProPheT777 ProPheT777 Improve test 840db54
@ProPheT777 ProPheT777 Merge remote-tracking branch 'origin/wrong-comparator-implementation'…
… into wrong-comparator-implementation

Conflicts:
	src/Symfony/Component/Finder/Tests/Iterator/SortableIteratorTest.php
f7898bc
Contributor

ProPheT777 commented May 20, 2014

Travis failed on php 5.3.3 but everything is ok weird.

Member

stof commented May 20, 2014

@ProPheT777 there is a weird thing with travis logs. they are truncated. The end is missing.

Contributor

ProPheT777 commented May 20, 2014

@stof Yes, I had not seen, in any case units tests about finder pass. I can re-trigger the build if you think it's necessary.

Member

stof commented May 20, 2014

no need. It is actually a bug in our Travis config, so the same would happen again if you retrigger it now. See #10950

Contributor

ProPheT777 commented May 20, 2014

Yep I read it just after my post

Contributor

ProPheT777 commented May 21, 2014

All is ok or any suggestions ?

Owner

fabpot commented May 22, 2014

Thank you @ProPheT777.

@fabpot fabpot added a commit that referenced this pull request May 22, 2014

@fabpot fabpot bug #10849 [WIP][Finder] Fix wrong implementation on sortable callbac…
…k comparator (ProPheT777)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #10849).

Discussion
----------

[WIP][Finder] Fix wrong implementation on sortable callback comparator

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | no

Working on this issue, i find SORT_BY_ACCESSED_TIME, SORT_BY_CHANGED_TIME, SORT_BY_MODIFIED_TIME was not tested.

- [x] Add test for SORT_BY_ACCESSED_TIME case
- [x] Add test for SORT_BY_CHANGED_TIME case
- [x] add test for SORT_BY_MODIFIED_TIME case

Commits
-------

b965fa2 [WIP][Finder] Fix wrong implementation on sortable callback comparator
2723e44

fabpot closed this May 22, 2014

ProPheT777 changed the title from [WIP][Finder] Fix wrong implementation on sortable callback comparator to [Finder] Fix wrong implementation on sortable callback comparator May 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment