From 47ba496a778316a8d099b28ab4478e893944a43e Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Thu, 3 Sep 2020 17:17:46 +0200 Subject: [PATCH] Error promotions in SPL --- ext/spl/spl_array.c | 6 +++--- ext/spl/spl_directory.c | 19 ++++++++++++------- ext/spl/spl_dllist.c | 5 ++--- ext/spl/spl_fixedarray.c | 4 +--- ext/spl/spl_heap.c | 4 +--- ext/spl/spl_iterators.c | 6 +++--- ext/spl/tests/bug61828.phpt | 14 +++++++++----- ext/spl/tests/bug65545.phpt | 14 ++++++++------ ext/spl/tests/iterator_044.phpt | 16 ++++++++-------- 9 files changed, 47 insertions(+), 41 deletions(-) diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c index 5beddc3391929..ce468267d089a 100644 --- a/ext/spl/spl_array.c +++ b/ext/spl/spl_array.c @@ -204,9 +204,9 @@ static zend_object *spl_array_object_new_ex(zend_class_entry *class_type, zend_o parent = parent->parent; inherited = 1; } - if (!parent) { /* this must never happen */ - php_error_docref(NULL, E_COMPILE_ERROR, "Internal compiler error, Class is not child of ArrayObject or ArrayIterator"); - } + + ZEND_ASSERT(parent); + if (inherited) { intern->fptr_offset_get = zend_hash_str_find_ptr(&class_type->function_table, "offsetget", sizeof("offsetget") - 1); if (intern->fptr_offset_get->common.scope == parent) { diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index e7bc262da5236..e782b1e70a462 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -732,7 +732,7 @@ void spl_filesystem_object_construct(INTERNAL_FUNCTION_PARAMETERS, zend_long cto intern = Z_SPLFILESYSTEM_P(ZEND_THIS); if (intern->_path) { /* object is already initialized */ - php_error_docref(NULL, E_WARNING, "Directory object is already initialized"); + zend_throw_error(NULL, "Directory object is already initialized"); return; } intern->flags = flags; @@ -1232,10 +1232,10 @@ PHP_METHOD(SplFileInfo, getLinkTarget) } #if defined(PHP_WIN32) || defined(HAVE_SYMLINK) if (intern->file_name == NULL) { - zend_restore_error_handling(&error_handling); - php_error_docref(NULL, E_WARNING, "Empty filename"); - RETURN_FALSE; - } else if (!IS_ABSOLUTE_PATH(intern->file_name, intern->file_name_len)) { + zend_value_error("Filename cannot be empty"); + RETURN_THROWS(); + } + if (!IS_ABSOLUTE_PATH(intern->file_name, intern->file_name_len)) { char expanded_path[MAXPATHLEN]; if (!expand_filepath_with_mode(intern->file_name, expanded_path, NULL, 0, CWD_EXPAND )) { zend_restore_error_handling(&error_handling); @@ -1577,6 +1577,7 @@ PHP_METHOD(GlobIterator, count) RETURN_LONG(php_glob_stream_get_count(intern->u.dir.dirp, NULL)); } else { /* should not happen */ + // TODO ZEND_ASSERT ? php_error_docref(NULL, E_ERROR, "GlobIterator lost glob state"); } } @@ -2330,6 +2331,7 @@ PHP_METHOD(SplFileObject, fgetcsv) CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern); + // TODO Align behaviour on normal fgetcsv() switch(ZEND_NUM_ARGS()) { case 3: @@ -2377,6 +2379,8 @@ PHP_METHOD(SplFileObject, fputcsv) zval *fields = NULL; if (zend_parse_parameters(ZEND_NUM_ARGS(), "a|sss", &fields, &delim, &d_len, &enclo, &e_len, &esc, &esc_len) == SUCCESS) { + + // TODO Align behaviour on normal fputcsv() switch(ZEND_NUM_ARGS()) { case 4: @@ -2429,6 +2433,7 @@ PHP_METHOD(SplFileObject, setCsvControl) size_t d_len = 0, e_len = 0, esc_len = 0; if (zend_parse_parameters(ZEND_NUM_ARGS(), "|sss", &delim, &d_len, &enclo, &e_len, &esc, &esc_len) == SUCCESS) { + // TODO Align behaviour on normal fgetcsv() switch(ZEND_NUM_ARGS()) { case 3: @@ -2685,8 +2690,8 @@ PHP_METHOD(SplFileObject, fread) CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern); if (length <= 0) { - php_error_docref(NULL, E_WARNING, "Length parameter must be greater than 0"); - RETURN_FALSE; + zend_argument_value_error(1, "must be greater than 0"); + RETURN_THROWS(); } str = php_stream_read_to_str(intern->u.file.stream, length); diff --git a/ext/spl/spl_dllist.c b/ext/spl/spl_dllist.c index 9d066ecf13216..eb01c09d80fc7 100644 --- a/ext/spl/spl_dllist.c +++ b/ext/spl/spl_dllist.c @@ -413,9 +413,8 @@ static zend_object *spl_dllist_object_new_ex(zend_class_entry *class_type, zend_ inherited = 1; } - if (!parent) { /* this must never happen */ - php_error_docref(NULL, E_COMPILE_ERROR, "Internal compiler error, Class is not child of SplDoublyLinkedList"); - } + ZEND_ASSERT(parent); + if (inherited) { intern->fptr_offset_get = zend_hash_str_find_ptr(&class_type->function_table, "offsetget", sizeof("offsetget") - 1); if (intern->fptr_offset_get->common.scope == parent) { diff --git a/ext/spl/spl_fixedarray.c b/ext/spl/spl_fixedarray.c index 866a0a9a7869c..558514eec1e50 100644 --- a/ext/spl/spl_fixedarray.c +++ b/ext/spl/spl_fixedarray.c @@ -234,9 +234,7 @@ static zend_object *spl_fixedarray_object_new_ex(zend_class_entry *class_type, z inherited = 1; } - if (!parent) { /* this must never happen */ - php_error_docref(NULL, E_COMPILE_ERROR, "Internal compiler error, Class is not child of SplFixedArray"); - } + ZEND_ASSERT(parent); funcs_ptr = class_type->iterator_funcs_ptr; if (!funcs_ptr->zf_current) { diff --git a/ext/spl/spl_heap.c b/ext/spl/spl_heap.c index e23b0f242138d..0beea11da0780 100644 --- a/ext/spl/spl_heap.c +++ b/ext/spl/spl_heap.c @@ -433,9 +433,7 @@ static zend_object *spl_heap_object_new_ex(zend_class_entry *class_type, zend_ob inherited = 1; } - if (!parent) { /* this must never happen */ - php_error_docref(NULL, E_COMPILE_ERROR, "Internal compiler error, Class is not child of SplHeap"); - } + ZEND_ASSERT(parent); if (inherited) { intern->fptr_cmp = zend_hash_str_find_ptr(&class_type->function_table, "compare", sizeof("compare") - 1); diff --git a/ext/spl/spl_iterators.c b/ext/spl/spl_iterators.c index 242fcfbfae88f..d245cc356731b 100644 --- a/ext/spl/spl_iterators.c +++ b/ext/spl/spl_iterators.c @@ -454,8 +454,8 @@ static zend_object_iterator *spl_recursive_it_get_iterator(zend_class_entry *ce, iterator = emalloc(sizeof(spl_recursive_it_iterator)); object = Z_SPLRECURSIVE_IT_P(zobject); if (object->iterators == NULL) { - zend_error(E_ERROR, "The object to be iterated is in an invalid state: " - "the parent constructor has not been called"); + zend_throw_error(NULL, "Object is not initialized"); + return NULL; } zend_iterator_init((zend_object_iterator*)iterator); @@ -2486,7 +2486,7 @@ PHP_METHOD(CachingIterator, offsetGet) } if ((value = zend_symtable_find(Z_ARRVAL(intern->u.caching.zcache), key)) == NULL) { - zend_error(E_NOTICE, "Undefined array key \"%s\"", ZSTR_VAL(key)); + zend_error(E_WARNING, "Undefined array key \"%s\"", ZSTR_VAL(key)); return; } diff --git a/ext/spl/tests/bug61828.phpt b/ext/spl/tests/bug61828.phpt index 04d435e6d6edd..2a11b760bb353 100644 --- a/ext/spl/tests/bug61828.phpt +++ b/ext/spl/tests/bug61828.phpt @@ -3,9 +3,13 @@ Bug #61828 (Memleak when calling Directory(Recursive)Iterator/Spl(Temp)FileObjec --FILE-- __construct('/tmp'); -echo "Okey"; + +try { + $x->__construct('/tmp'); +} catch (\Error $e) { + echo $e->getMessage() . \PHP_EOL; +} + ?> ---EXPECTF-- -Warning: DirectoryIterator::__construct(): Directory object is already initialized in %sbug61828.php on line 3 -Okey +--EXPECT-- +Directory object is already initialized diff --git a/ext/spl/tests/bug65545.phpt b/ext/spl/tests/bug65545.phpt index bd5a7f06dbfd8..8ebbf648c97c5 100644 --- a/ext/spl/tests/bug65545.phpt +++ b/ext/spl/tests/bug65545.phpt @@ -6,17 +6,19 @@ $obj = new SplFileObject(__FILE__, 'r'); $data = $obj->fread(5); var_dump($data); -$data = $obj->fread(0); -var_dump($data); +try { + $data = $obj->fread(0); + var_dump($data); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} // read more data than is available $data = $obj->fread(filesize(__FILE__) + 32); var_dump(strlen($data) === filesize(__FILE__) - 5); ?> ---EXPECTF-- +--EXPECT-- string(5) "