Skip to content

Remove unnecessary error handler replacement in SPL #6955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Remove unnecessary error handler replacement in SPL #6955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 27 additions & 38 deletions ext/spl/spl_directory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,8 @@ static inline int spl_filesystem_is_dot(const char * d_name) /* {{{ */
/* }}} */

/* {{{ spl_filesystem_dir_open */
/* open a directory resource */
/* open a directory resource
* Can emit an E_WARNING as it reports errors from php_stream_opendir() */
static void spl_filesystem_dir_open(spl_filesystem_object* intern, zend_string *path)
{
int skip_dots = SPL_HAS_FLAG(intern->flags, SPL_FILE_DIR_SKIPDOTS);
Expand Down Expand Up @@ -297,7 +298,9 @@ static void spl_filesystem_dir_open(spl_filesystem_object* intern, zend_string *
}
/* }}} */

static int spl_filesystem_file_open(spl_filesystem_object *intern, int use_include_path, int silent) /* {{{ */
/* Can generate E_WARNINGS as we report errors from stream initialized via
* php_stream_open_wrapper_ex() */
static zend_result spl_filesystem_file_open(spl_filesystem_object *intern, bool use_include_path) /* {{{ */
{
zval tmp;

Expand Down Expand Up @@ -441,7 +444,6 @@ static spl_filesystem_object *spl_filesystem_object_create_info(spl_filesystem_o
{
spl_filesystem_object *intern;
zval arg1;
zend_error_handling error_handling;

if (!file_path || !ZSTR_LEN(file_path)) {
#ifdef PHP_WIN32
Expand All @@ -450,8 +452,6 @@ static spl_filesystem_object *spl_filesystem_object_create_info(spl_filesystem_o
return NULL;
}

zend_replace_error_handling(EH_THROW, spl_ce_RuntimeException, &error_handling);

ce = ce ? ce : source->info_class;

intern = spl_filesystem_from_obj(spl_filesystem_object_new_ex(ce));
Expand All @@ -465,7 +465,6 @@ static spl_filesystem_object *spl_filesystem_object_create_info(spl_filesystem_o
spl_filesystem_info_set_filename(intern, file_path);
}

zend_restore_error_handling(&error_handling);
return intern;
} /* }}} */

Expand Down Expand Up @@ -558,8 +557,9 @@ static spl_filesystem_object *spl_filesystem_object_create_type(int num_args, sp
intern->u.file.open_mode_len = open_mode_len;
intern->u.file.zcontext = resource;

/* spl_filesystem_file_open() can generate E_WARNINGs which we want to promote to exceptions */
zend_replace_error_handling(EH_THROW, spl_ce_RuntimeException, &error_handling);
if (spl_filesystem_file_open(intern, use_include_path, 0) == FAILURE) {
if (spl_filesystem_file_open(intern, use_include_path) == FAILURE) {
zend_restore_error_handling(&error_handling);
zval_ptr_dtor(return_value);
ZVAL_NULL(return_value);
Expand Down Expand Up @@ -735,6 +735,7 @@ void spl_filesystem_object_construct(INTERNAL_FUNCTION_PARAMETERS, zend_long cto
}
intern->flags = flags;

/* spl_filesystem_dir_open() may emit an E_WARNING */
zend_replace_error_handling(EH_THROW, spl_ce_UnexpectedValueException, &error_handling);
#ifdef HAVE_GLOB
if (SPL_HAS_FLAG(ctor_flags, DIT_CTOR_GLOB) && memcmp(ZSTR_VAL(path), "glob://", sizeof("glob://")-1) != 0) {
Expand All @@ -747,10 +748,9 @@ void spl_filesystem_object_construct(INTERNAL_FUNCTION_PARAMETERS, zend_long cto
spl_filesystem_dir_open(intern, path);

}
zend_restore_error_handling(&error_handling);

intern->u.dir.is_recursive = instanceof_function(intern->std.ce, spl_ce_RecursiveDirectoryIterator) ? 1 : 0;

zend_restore_error_handling(&error_handling);
}
/* }}} */

Expand Down Expand Up @@ -1227,17 +1227,13 @@ PHP_METHOD(SplFileInfo, getLinkTarget)
spl_filesystem_object *intern = Z_SPLFILESYSTEM_P(ZEND_THIS);
ssize_t ret;
char buff[MAXPATHLEN];
zend_error_handling error_handling;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

zend_replace_error_handling(EH_THROW, spl_ce_RuntimeException, &error_handling);

if (intern->file_name == NULL) {
if (spl_filesystem_object_get_file_name(intern) != SUCCESS) {
zend_restore_error_handling(&error_handling);
RETURN_THROWS();
}
}
Expand All @@ -1249,7 +1245,6 @@ PHP_METHOD(SplFileInfo, getLinkTarget)
if (!IS_ABSOLUTE_PATH(ZSTR_VAL(intern->file_name), ZSTR_LEN(intern->file_name))) {
char expanded_path[MAXPATHLEN];
if (!expand_filepath_with_mode(ZSTR_VAL(intern->file_name), expanded_path, NULL, 0, CWD_EXPAND )) {
zend_restore_error_handling(&error_handling);
php_error_docref(NULL, E_WARNING, "No such file or directory");
RETURN_FALSE;
}
Expand All @@ -1270,8 +1265,6 @@ PHP_METHOD(SplFileInfo, getLinkTarget)

RETVAL_STRINGL(buff, ret);
}

zend_restore_error_handling(&error_handling);
}
/* }}} */

Expand All @@ -1281,17 +1274,13 @@ PHP_METHOD(SplFileInfo, getRealPath)
spl_filesystem_object *intern = Z_SPLFILESYSTEM_P(ZEND_THIS);
char buff[MAXPATHLEN];
char *filename;
zend_error_handling error_handling;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

zend_replace_error_handling(EH_THROW, spl_ce_RuntimeException, &error_handling);

if (intern->type == SPL_FS_DIR && !intern->file_name && intern->u.dir.entry.d_name[0]) {
if (spl_filesystem_object_get_file_name(intern) != SUCCESS) {
zend_restore_error_handling(&error_handling);
RETURN_THROWS();
}
}
Expand All @@ -1313,8 +1302,6 @@ PHP_METHOD(SplFileInfo, getRealPath)
} else {
RETVAL_FALSE;
}

zend_restore_error_handling(&error_handling);
}
/* }}} */

Expand Down Expand Up @@ -2069,28 +2056,29 @@ PHP_METHOD(SplFileObject, __construct)
intern->u.file.open_mode_len = 1;
}

/* spl_filesystem_file_open() can generate E_WARNINGs which we want to promote to exceptions */
zend_replace_error_handling(EH_THROW, spl_ce_RuntimeException, &error_handling);
if (spl_filesystem_file_open(intern, use_include_path) == FAILURE) {
zend_restore_error_handling(&error_handling);
RETURN_THROWS();
}
zend_restore_error_handling(&error_handling);
Comment on lines +2061 to +2065
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (spl_filesystem_file_open(intern, use_include_path) == FAILURE) {
zend_restore_error_handling(&error_handling);
RETURN_THROWS();
}
zend_restore_error_handling(&error_handling);
zend_result retval = spl_filesystem_file_open(intern, use_include_path);
zend_restore_error_handling(&error_handling);
if (retval == FAILURE) {
RETURN_THROWS();
}

How I'd personally write this.


if (spl_filesystem_file_open(intern, use_include_path, 0) == SUCCESS) {
path_len = strlen(intern->u.file.stream->orig_path);

if (path_len > 1 && IS_SLASH_AT(intern->u.file.stream->orig_path, path_len-1)) {
path_len--;
}

while (path_len > 1 && !IS_SLASH_AT(intern->u.file.stream->orig_path, path_len-1)) {
path_len--;
}
path_len = strlen(intern->u.file.stream->orig_path);

if (path_len) {
path_len--;
}
if (path_len > 1 && IS_SLASH_AT(intern->u.file.stream->orig_path, path_len-1)) {
path_len--;
}

intern->path = zend_string_init(intern->u.file.stream->orig_path, path_len, 0);
while (path_len > 1 && !IS_SLASH_AT(intern->u.file.stream->orig_path, path_len-1)) {
path_len--;
}

zend_restore_error_handling(&error_handling);
if (path_len) {
path_len--;
}

intern->path = zend_string_init(intern->u.file.stream->orig_path, path_len, 0);
} /* }}} */

/* {{{ Construct a new temp file object */
Expand All @@ -2116,8 +2104,9 @@ PHP_METHOD(SplTempFileObject, __construct)
intern->u.file.open_mode = "wb";
intern->u.file.open_mode_len = 1;

/* spl_filesystem_file_open() can generate E_WARNINGs which we want to promote to exceptions */
zend_replace_error_handling(EH_THROW, spl_ce_RuntimeException, &error_handling);
if (spl_filesystem_file_open(intern, 0, 0) == SUCCESS) {
if (spl_filesystem_file_open(intern, /* use_include_path */ false) == SUCCESS) {
intern->path = ZSTR_EMPTY_ALLOC();
}
zend_string_release(file_name);
Expand Down
9 changes: 1 addition & 8 deletions ext/spl/spl_iterators.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,6 @@ static void spl_recursive_it_it_construct(INTERNAL_FUNCTION_PARAMETERS, zend_cla
zval *iterator;
zend_class_entry *ce_iterator;
zend_long mode, flags;
zend_error_handling error_handling;
zval caching_it, aggregate_retval;

switch (rit_type) {
Expand All @@ -500,7 +499,6 @@ static void spl_recursive_it_it_construct(INTERNAL_FUNCTION_PARAMETERS, zend_cla
RETURN_THROWS();
}

zend_replace_error_handling(EH_THROW, spl_ce_InvalidArgumentException, &error_handling);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getIterator() could warn -- but I agree that we shouldn't be promoting arbitrary warnings inside getIterator().

if (instanceof_function(Z_OBJCE_P(iterator), zend_ce_aggregate)) {
zend_function **getiterator_cache = Z_OBJCE_P(iterator)->iterator_funcs_ptr
? &Z_OBJCE_P(iterator)->iterator_funcs_ptr->zf_new_iterator : NULL;
Expand All @@ -525,7 +523,6 @@ static void spl_recursive_it_it_construct(INTERNAL_FUNCTION_PARAMETERS, zend_cla
RETURN_THROWS();
}

zend_replace_error_handling(EH_THROW, spl_ce_InvalidArgumentException, &error_handling);
if (instanceof_function(Z_OBJCE_P(iterator), zend_ce_aggregate)) {
zend_function **getiterator_cache = Z_OBJCE_P(iterator)->iterator_funcs_ptr
? &Z_OBJCE_P(iterator)->iterator_funcs_ptr->zf_new_iterator : NULL;
Expand All @@ -542,7 +539,6 @@ static void spl_recursive_it_it_construct(INTERNAL_FUNCTION_PARAMETERS, zend_cla
zval_ptr_dtor(iterator);
}
zend_throw_exception(spl_ce_InvalidArgumentException, "An instance of RecursiveIterator or IteratorAggregate creating it is required", 0);
zend_restore_error_handling(&error_handling);
return;
}

Expand Down Expand Up @@ -592,8 +588,6 @@ static void spl_recursive_it_it_construct(INTERNAL_FUNCTION_PARAMETERS, zend_cla
intern->iterators[0].haschildren = NULL;
intern->iterators[0].getchildren = NULL;

zend_restore_error_handling(&error_handling);

if (EG(exception)) {
zend_object_iterator *sub_iter;

Expand Down Expand Up @@ -1382,11 +1376,9 @@ static spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, z
return NULL;
}
intern->dit_type = DIT_AppendIterator;
zend_replace_error_handling(EH_THROW, spl_ce_InvalidArgumentException, &error_handling);
object_init_ex(&intern->u.append.zarrayit, spl_ce_ArrayIterator);
zend_call_method_with_0_params(Z_OBJ(intern->u.append.zarrayit), spl_ce_ArrayIterator, &spl_ce_ArrayIterator->constructor, "__construct", NULL);
intern->u.append.iterator = spl_ce_ArrayIterator->get_iterator(spl_ce_ArrayIterator, &intern->u.append.zarrayit, 0);
zend_restore_error_handling(&error_handling);
return intern;
case DIT_RegexIterator:
case DIT_RecursiveRegexIterator: {
Expand All @@ -1405,6 +1397,7 @@ static spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, z
return NULL;
}

/* pcre_get_compiled_regex_cache() might emit E_WARNINGs that we want to promote to exception */
zend_replace_error_handling(EH_THROW, spl_ce_InvalidArgumentException, &error_handling);
intern->u.regex.pce = pcre_get_compiled_regex_cache(regex);
zend_restore_error_handling(&error_handling);
Expand Down