Fix GH-18120: Honor FILE_SKIP_EMPTY_LINES even when FILE_IGNORE_NEW_LINES is not set

[alexandre-daubois]

Fix #18120

Allows to use FILE_SKIP_EMPTY_LINES without FILE_IGNORE_NEW_LINES:

$test_file = __DIR__ . "/file_skip_empty_lines.tmp";

$test_data = "First\n\nSecond\n\n\nThird\n";
file_put_contents($test_file, $test_data);

$lines = file($test_file, FILE_SKIP_EMPTY_LINES);
var_dump($lines);

/**
array(3) {
  [0]=>
  string(6) "First
"
  [1]=>
  string(7) "Second
"
  [2]=>
  string(6) "Third
"
}
*/

[alexandre-daubois]

Friendly ping @DanielEScherzer @nielsdos as you participated on the issue 🙂

[ndossche]

I'll play with this tomorrow or so. Note that this is fixing long standing behaviour, so targeting 8.5/master is likely more appropriate.

[DanielEScherzer]

^ what @nielsdos said about not targeting 8.3, but wearing my release manager hat I don't think 8.5 is appropriate

[ndossche]

<?php
$test_file = __DIR__ . "/file_skip_empty_lines.tmp";

$test_data = "\r";
file_put_contents($test_file, $test_data);
$lines = file($test_file, FILE_SKIP_EMPTY_LINES);
var_dump($lines);

$test_data = "\r\r";
file_put_contents($test_file, $test_data);
$lines = file($test_file, FILE_SKIP_EMPTY_LINES);
var_dump($lines);

produces:

array(1) {
  [0]=>
" string(1) "
}
array(0) {
}

which seems incorrect to me. Can you explain whether this is the desired behaviour?

[alexandre-daubois]

@ndossche I addressed your concern and updated the test file as well. Also, I rebased on master as suggested.

[ndossche]

I don't think target_buf has any guarantees about its length, so ZSTR_VAL(target_buf) + 2 may point out of bounds. However, doing pointer arithmetic that results in a pointer more than 1 byte out of bounds is undefined behaviour.
Use ZSTR_VAL(target_buf) - p >= 2 instead.

[alexandre-daubois]

I think the correct condition is p - ZSTR_VAL(target_buf) >= 2. PR updated 🙂