Seeking before beginning of file should fail

[merijnvdk]

When seeking in a file to a location before the beginning of the file,
the operation seems to succeed. While if i try to do the same with a
"real" file then the seek command will return an error.

So i changed the seek method to return false if the offset is less
than 0. This seems to solve my problem and behaves in the same
way as seeking in a real file. However I do not know if there are
situations where seeking before the beginning of the file could be
a valid use case. testing with fseek on unix fs always returns -1
when seeking past the beginning of the file. Added five test
assertions to validate this behavior.

[coveralls]

Coverage increased (+0.04%) to 94.346% when pulling 44348c5 on merijnvdk:master into 03c1427 on mikey179:master.

[merijnvdk]

Running the test:

// testfile.txt has content "12345"
$fh=fopen('testfile.txt','r');
for($i=-2;$i<7;$i++){
  $seek_res=fseek($fh,$i,SEEK_SET);
  echo "seek $i: $seek_res\n";
  echo "  ftell: ".ftell($fh)."\n";
}
fclose($fh);

Outputs:
seek -2: -1
ftell: 0
seek -1: -1
ftell: 0
seek 0: 0
ftell: 0
seek 1: 0
ftell: 1
seek 2: 0
ftell: 2
seek 3: 0
ftell: 3
seek 4: 0
ftell: 4
seek 5: 0
ftell: 5
seek 6: 0
ftell: 6

So it seems seeking past the end of the file is OK, but before the beginning is not. Maybe someone can test on different filesystem.

[mikey179]

Thanks for detecting this and the PR! See my inline comments.

[mikey179]

I believe it should be the other way around. Instead of changing the internal offset value and reset it if it's not valid, the new value should be calculated, then checked if it is valid, and only when it is valid the internal offset value should be changed to the new value. This way data corruption can be prevented in case something stupid happens, and it also makes it more failure proof against future changes.

Also, let's stick to camelCase for variable names as all other variable names are already camelCase.

[mikey179]

I know the existing test cases don't follow the "roughly one assertion per test" rule, but I really think new tests should do this. Otherwise it is harder to see what broke when a test fails. In this case two new test methods should be added, one for testing against the empty file and one against a non-empty file.

[coveralls]

Coverage increased (+0.04%) to 94.346% when pulling dbf28b5 on merijnvdk:master into 03c1427 on mikey179:master.

[merijnvdk]

I changed the code according to your review comments (I hope). I don't know how the review process on GIT works and if I have to re-request a review somewhere, or is it automatic?

[mikey179]

Thank you, just merged it.