Skip to content

Commit 625d846

Browse files
nikicnikic
committed
Consistent error handling for fgetcsv/fputcsv
Normalize the behavior between the file functions and those on SplFileObject. Be consistent about throwing regardless of whether the delimiter etc is empty or has too many characters. I don't think it's worthwhile to distinguish these cases. Back when we looked into this originally, there was some hope that we might want to add support for multiple-character delimiter etc, but after a cursory look, I really don't think this is going to happen (for fputcsv maybe, but for fgetcsv this just makes an already broken function much more complicated.) Closes GH-6188.
1 parent 72e8719 commit 625d846

16 files changed

+1050
-3833
lines changed

‎ext/spl/spl_directory.c

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2328,13 +2328,12 @@ PHP_METHOD(SplFileObject, fgetcsv)
23282328

23292329
CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern);
23302330

2331-
// TODO Align behaviour on normal fgetcsv()
23322331
switch(ZEND_NUM_ARGS())
23332332
{
23342333
case 3:
23352334
if (esc_len > 1) {
2336-
php_error_docref(NULL, E_WARNING, "escape must be empty or a single character");
2337-
RETURN_FALSE;
2335+
zend_argument_value_error(3, "must be empty or a single character");
2336+
RETURN_THROWS();
23382337
}
23392338
if (esc_len == 0) {
23402339
escape = PHP_CSV_NO_ESCAPE;
@@ -2344,15 +2343,15 @@ PHP_METHOD(SplFileObject, fgetcsv)
23442343
/* no break */
23452344
case 2:
23462345
if (e_len != 1) {
2347-
php_error_docref(NULL, E_WARNING, "enclosure must be a character");
2348-
RETURN_FALSE;
2346+
zend_argument_value_error(2, "must be a single character");
2347+
RETURN_THROWS();
23492348
}
23502349
enclosure = enclo[0];
23512350
/* no break */
23522351
case 1:
23532352
if (d_len != 1) {
2354-
php_error_docref(NULL, E_WARNING, "delimiter must be a character");
2355-
RETURN_FALSE;
2353+
zend_argument_value_error(1, "must be a single character");
2354+
RETURN_THROWS();
23562355
}
23572356
delimiter = delim[0];
23582357
/* no break */
@@ -2377,7 +2376,6 @@ PHP_METHOD(SplFileObject, fputcsv)
23772376

23782377
if (zend_parse_parameters(ZEND_NUM_ARGS(), "a|sss", &fields, &delim, &d_len, &enclo, &e_len, &esc, &esc_len) == SUCCESS) {
23792378

2380-
// TODO Align behaviour on normal fputcsv()
23812379
switch(ZEND_NUM_ARGS())
23822380
{
23832381
case 4:
@@ -2389,21 +2387,21 @@ PHP_METHOD(SplFileObject, fputcsv)
23892387
escape = (unsigned char) esc[0];
23902388
break;
23912389
default:
2392-
php_error_docref(NULL, E_WARNING, "escape must be empty or a single character");
2393-
RETURN_FALSE;
2390+
zend_argument_value_error(4, "must be empty or a single character");
2391+
RETURN_THROWS();
23942392
}
23952393
/* no break */
23962394
case 3:
23972395
if (e_len != 1) {
2398-
php_error_docref(NULL, E_WARNING, "enclosure must be a character");
2399-
RETURN_FALSE;
2396+
zend_argument_value_error(3, "must be a single character");
2397+
RETURN_THROWS();
24002398
}
24012399
enclosure = enclo[0];
24022400
/* no break */
24032401
case 2:
24042402
if (d_len != 1) {
2405-
php_error_docref(NULL, E_WARNING, "delimiter must be a character");
2406-
RETURN_FALSE;
2403+
zend_argument_value_error(2, "must be a single character");
2404+
RETURN_THROWS();
24072405
}
24082406
delimiter = delim[0];
24092407
/* no break */
@@ -2430,7 +2428,6 @@ PHP_METHOD(SplFileObject, setCsvControl)
24302428
size_t d_len = 0, e_len = 0, esc_len = 0;
24312429

24322430
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|sss", &delim, &d_len, &enclo, &e_len, &esc, &esc_len) == SUCCESS) {
2433-
// TODO Align behaviour on normal fgetcsv()
24342431
switch(ZEND_NUM_ARGS())
24352432
{
24362433
case 3:
@@ -2442,21 +2439,21 @@ PHP_METHOD(SplFileObject, setCsvControl)
24422439
escape = (unsigned char) esc[0];
24432440
break;
24442441
default:
2445-
php_error_docref(NULL, E_WARNING, "escape must be empty or a single character");
2446-
RETURN_FALSE;
2442+
zend_argument_value_error(3, "must be empty or a single character");
2443+
RETURN_THROWS();
24472444
}
24482445
/* no break */
24492446
case 2:
24502447
if (e_len != 1) {
2451-
php_error_docref(NULL, E_WARNING, "enclosure must be a character");
2452-
RETURN_FALSE;
2448+
zend_argument_value_error(2, "must be a single character");
2449+
RETURN_THROWS();
24532450
}
24542451
enclosure = enclo[0];
24552452
/* no break */
24562453
case 1:
24572454
if (d_len != 1) {
2458-
php_error_docref(NULL, E_WARNING, "delimiter must be a character");
2459-
RETURN_FALSE;
2455+
zend_argument_value_error(1, "must be a single character");
2456+
RETURN_THROWS();
24602457
}
24612458
delimiter = delim[0];
24622459
/* no break */

‎ext/spl/tests/SplFileObject_fgetcsv_delimiter_error.phpt

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,15 @@ fputcsv($fp, array(
1212
fclose($fp);
1313

1414
$fo = new SplFileObject('SplFileObject__fgetcsv3.csv');
15-
var_dump($fo->fgetcsv('invalid'));
15+
try {
16+
var_dump($fo->fgetcsv('invalid'));
17+
} catch (ValueError $e) {
18+
echo $e->getMessage(), "\n";
19+
}
1620
?>
1721
--CLEAN--
1822
<?php
1923
unlink('SplFileObject__fgetcsv3.csv');
2024
?>
21-
--EXPECTF--
22-
Warning: SplFileObject::fgetcsv(): delimiter must be a character in %s on line %d
23-
bool(false)
25+
--EXPECT--
26+
SplFileObject::fgetcsv(): Argument #1 ($delimiter) must be a single character

‎ext/spl/tests/SplFileObject_fgetcsv_enclosure_error.phpt

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,15 @@ fputcsv($fp, array(
1212
fclose($fp);
1313

1414
$fo = new SplFileObject('SplFileObject__fgetcsv5.csv');
15-
var_dump($fo->fgetcsv(',', 'invalid'));
15+
try {
16+
var_dump($fo->fgetcsv(',', 'invalid'));
17+
} catch (ValueError $e) {
18+
echo $e->getMessage(), "\n";
19+
}
1620
?>
1721
--CLEAN--
1822
<?php
1923
unlink('SplFileObject__fgetcsv5.csv');
2024
?>
21-
--EXPECTF--
22-
Warning: SplFileObject::fgetcsv(): enclosure must be a character in %s on line %d
23-
bool(false)
25+
--EXPECT--
26+
SplFileObject::fgetcsv(): Argument #2 ($enclosure) must be a single character

‎ext/spl/tests/SplFileObject_fgetcsv_escape_error.phpt

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,15 @@ fwrite($fp, '"aaa","b""bb","ccc"');
77
fclose($fp);
88

99
$fo = new SplFileObject('SplFileObject__fgetcsv8.csv');
10-
var_dump($fo->fgetcsv(',', '"', 'invalid'));
10+
try {
11+
var_dump($fo->fgetcsv(',', '"', 'invalid'));
12+
} catch (ValueError $e) {
13+
echo $e->getMessage(), "\n";
14+
}
1115
?>
1216
--CLEAN--
1317
<?php
1418
unlink('SplFileObject__fgetcsv8.csv');
1519
?>
16-
--EXPECTF--
17-
Warning: SplFileObject::fgetcsv(): escape must be empty or a single character in %s on line %d
18-
bool(false)
20+
--EXPECT--
21+
SplFileObject::fgetcsv(): Argument #3 ($escape) must be empty or a single character

‎ext/spl/tests/SplFileObject_fputcsv_variation13.phpt

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ echo "*** Testing fputcsv() : with default enclosure & delimiter of two chars **
1010

1111
$fo = new SplFileObject(__DIR__ . '/SplFileObject_fputcsv_variation13.csv', 'w');
1212

13-
var_dump($fo->fputcsv(array('water', 'fruit'), ',,', '"'));
13+
try {
14+
var_dump($fo->fputcsv(array('water', 'fruit'), ',,', '"'));
15+
} catch (ValueError $e) {
16+
echo $e->getMessage(), "\n";
17+
}
1418

1519
unset($fo);
1620

@@ -21,9 +25,7 @@ echo "Done\n";
2125
$file = __DIR__ . '/SplFileObject_fputcsv_variation13.csv';
2226
unlink($file);
2327
?>
24-
--EXPECTF--
28+
--EXPECT--
2529
*** Testing fputcsv() : with default enclosure & delimiter of two chars ***
26-
27-
Warning: SplFileObject::fputcsv(): delimiter must be a character in %s on line %d
28-
bool(false)
30+
SplFileObject::fputcsv(): Argument #2 ($delimiter) must be a single character
2931
Done

‎ext/spl/tests/SplFileObject_fputcsv_variation14.phpt

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@ echo "*** Testing fputcsv() : with enclosure & delimiter of two chars and file o
1010

1111
$fo = new SplFileObject(__DIR__ . '/SplFileObject_fputcsv_variation14.csv', 'w');
1212

13-
var_dump($fo->fputcsv(array('water', 'fruit'), ',,', '""'));
13+
try {
14+
var_dump($fo->fputcsv(array('water', 'fruit'), ',,', '""'));
15+
} catch (ValueError $e) {
16+
echo $e->getMessage(), "\n";
17+
}
1418

1519
unset($fo);
1620

@@ -21,9 +25,7 @@ echo "Done\n";
2125
$file = __DIR__ . '/SplFileObject_fputcsv_variation14.csv';
2226
unlink($file);
2327
?>
24-
--EXPECTF--
28+
--EXPECT--
2529
*** Testing fputcsv() : with enclosure & delimiter of two chars and file opened in read mode ***
26-
27-
Warning: SplFileObject::fputcsv(): enclosure must be a character in %s on line %d
28-
bool(false)
30+
SplFileObject::fputcsv(): Argument #3 ($enclosure) must be a single character
2931
Done

‎ext/spl/tests/SplFileObject_setCsvControl_error001.phpt

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,15 @@ CDATA
1313
);
1414
$s = new SplFileObject('csv_control_data_error001.csv');
1515
$s->setFlags(SplFileObject::READ_CSV);
16-
$s->setCsvControl('||');
16+
try {
17+
$s->setCsvControl('||');
18+
} catch (ValueError $e) {
19+
echo $e->getMessage(), "\n";
20+
}
1721
?>
1822
--CLEAN--
1923
<?php
2024
unlink('csv_control_data_error001.csv');
2125
?>
22-
--EXPECTF--
23-
Warning: SplFileObject::setCsvControl(): delimiter must be a character in %s on line %d
26+
--EXPECT--
27+
SplFileObject::setCsvControl(): Argument #1 ($delimiter) must be a single character

‎ext/spl/tests/SplFileObject_setCsvControl_error002.phpt

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,15 @@ CDATA
1313
);
1414
$s = new SplFileObject('csv_control_data_error002.csv');
1515
$s->setFlags(SplFileObject::READ_CSV);
16-
$s->setCsvControl('|', 'two');
16+
try {
17+
$s->setCsvControl('|', 'two');
18+
} catch (ValueError $e) {
19+
echo $e->getMessage(), "\n";
20+
}
1721
?>
1822
--CLEAN--
1923
<?php
2024
unlink('csv_control_data_error002.csv');
2125
?>
22-
--EXPECTF--
23-
Warning: SplFileObject::setCsvControl(): enclosure must be a character in %s on line %d
26+
--EXPECT--
27+
SplFileObject::setCsvControl(): Argument #2 ($enclosure) must be a single character

‎ext/spl/tests/SplFileObject_setCsvControl_error003.phpt

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,15 @@ CDATA
1515
);
1616
$s = new SplFileObject('csv_control_data_error003.csv');
1717
$s->setFlags(SplFileObject::READ_CSV);
18-
$s->setCsvControl('|', '\'', 'three');
18+
try {
19+
$s->setCsvControl('|', '\'', 'three');
20+
} catch (ValueError $e) {
21+
echo $e->getMessage(), "\n";
22+
}
1923
?>
2024
--CLEAN--
2125
<?php
2226
unlink('csv_control_data_error003.csv');
2327
?>
24-
--EXPECTF--
25-
Warning: SplFileObject::setCsvControl(): escape must be empty or a single character in %s on line %d
28+
--EXPECT--
29+
SplFileObject::setCsvControl(): Argument #3 ($escape) must be empty or a single character

‎ext/standard/file.c

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1815,31 +1815,28 @@ PHP_FUNCTION(fputcsv)
18151815

18161816
if (delimiter_str != NULL) {
18171817
/* Make sure that there is at least one character in string */
1818-
if (delimiter_str_len < 1) {
1818+
if (delimiter_str_len != 1) {
18191819
zend_argument_value_error(3, "must be a single character");
18201820
RETURN_THROWS();
1821-
} else if (delimiter_str_len > 1) {
1822-
php_error_docref(NULL, E_WARNING, "Argument #3 ($delimiter) must be a single character");
18231821
}
18241822

18251823
/* use first character from string */
18261824
delimiter = *delimiter_str;
18271825
}
18281826

18291827
if (enclosure_str != NULL) {
1830-
if (enclosure_str_len < 1) {
1828+
if (enclosure_str_len != 1) {
18311829
zend_argument_value_error(4, "must be a single character");
18321830
RETURN_THROWS();
1833-
} else if (enclosure_str_len > 1) {
1834-
php_error_docref(NULL, E_WARNING, "Argument #4 ($enclosure) must be a single character");
18351831
}
18361832
/* use first character from string */
18371833
enclosure = *enclosure_str;
18381834
}
18391835

18401836
if (escape_str != NULL) {
18411837
if (escape_str_len > 1) {
1842-
php_error_docref(NULL, E_WARNING, "Argument #5 ($escape) must be empty or a single character");
1838+
zend_argument_value_error(5, "must be empty or a single character");
1839+
RETURN_THROWS();
18431840
}
18441841
if (escape_str_len < 1) {
18451842
escape_char = PHP_CSV_NO_ESCAPE;
@@ -1953,23 +1950,19 @@ PHP_FUNCTION(fgetcsv)
19531950

19541951
if (delimiter_str != NULL) {
19551952
/* Make sure that there is at least one character in string */
1956-
if (delimiter_str_len < 1) {
1953+
if (delimiter_str_len != 1) {
19571954
zend_argument_value_error(3, "must be a single character");
19581955
RETURN_THROWS();
1959-
} else if (delimiter_str_len > 1) {
1960-
php_error_docref(NULL, E_WARNING, "Argument #3 ($delimiter) must be a single character");
19611956
}
19621957

19631958
/* use first character from string */
19641959
delimiter = delimiter_str[0];
19651960
}
19661961

19671962
if (enclosure_str != NULL) {
1968-
if (enclosure_str_len < 1) {
1963+
if (enclosure_str_len != 1) {
19691964
zend_argument_value_error(4, "must be a single character");
19701965
RETURN_THROWS();
1971-
} else if (enclosure_str_len > 1) {
1972-
php_error_docref(NULL, E_WARNING, "Argument #4 ($enclosure) must be a single character");
19731966
}
19741967

19751968
/* use first character from string */
@@ -1978,7 +1971,8 @@ PHP_FUNCTION(fgetcsv)
19781971

19791972
if (escape_str != NULL) {
19801973
if (escape_str_len > 1) {
1981-
php_error_docref(NULL, E_WARNING, "Argument #5 ($enclosure) must be empty or a single character");
1974+
zend_argument_value_error(5, "must be empty or a single character");
1975+
RETURN_THROWS();
19821976
}
19831977

19841978
if (escape_str_len < 1) {

0 commit comments

Comments
 (0)