Skip to content

RFC: Constructor behaviour of internal classes #1178

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 11 commits into from
Closed

RFC: Constructor behaviour of internal classes #1178

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b00cd68
Fixed PDORow behaviour and message.
Danack Mar 1, 2015
043a026
Fixed ReflectionFunction, ReflectionMethod and ReflectionParameter.
Danack Mar 1, 2015
a519838
Fixed ReflectionExtension and ReflectionProperty.
Danack Mar 1, 2015
c57bde7
Fixed SplFixedArray and tests.
Danack Mar 1, 2015
99dae96
Converted intl extension to use IntlException in constructors.
Danack Mar 15, 2015
0922eca
Made Phar throw exception on bad constructor.
Danack Mar 15, 2015
78ebf83
Fixed fileinfo behaviour.
Danack Mar 15, 2015
d3267d0
Fixed PDO constructor to not return null.
Danack Mar 15, 2015
9803fcc
Made UConverter throw an exception if the constructor fails.
Danack Mar 15, 2015
6b23264
Reverted change to function name and added note of why it is differen…
Danack Mar 15, 2015
910a324
Fixed indentation. Fixed comment style. Fixed commented out code.
Danack Mar 16, 2015
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
12 changes: 11 additions & 1 deletion ext/fileinfo/fileinfo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,18 @@ PHP_FUNCTION(finfo_open)
php_fileinfo *finfo;
FILEINFO_DECLARE_INIT_OBJECT(object)
char resolved_path[MAXPATHLEN];
zend_error_handling zeh;
int rv;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|lp", &options, &file, &file_len) == FAILURE) {
if (object) {
zend_replace_error_handling(EH_THROW, NULL, &zeh TSRMLS_CC);
rv = zend_parse_parameters(ZEND_NUM_ARGS(), "|lp", &options, &file, &file_len);
zend_restore_error_handling(&zeh TSRMLS_CC);
} else {
rv = zend_parse_parameters(ZEND_NUM_ARGS(), "|lp", &options, &file, &file_len);
}

if (rv == FAILURE) {
FILEINFO_DESTROY_OBJECT(object);
RETURN_FALSE;
}
Expand Down
4 changes: 2 additions & 2 deletions ext/intl/breakiterator/breakiterator_methods.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,12 @@ U_CFUNC PHP_FUNCTION(breakiter_set_text)
BREAKITER_METHOD_FETCH_OBJECT;

ut = utext_openUTF8(ut, text->val, text->len, BREAKITER_ERROR_CODE_P(bio));
INTL_CTOR_CHECK_STATUS(bio, "breakiter_set_text: error opening UText");
INTL_CTOR_CHECK_STATUS(bio, "breakiter_set_text: error opening UText", 0);

bio->biter->setText(ut, BREAKITER_ERROR_CODE(bio));
utext_close(ut); /* ICU shallow clones the UText */
INTL_CTOR_CHECK_STATUS(bio, "breakiter_set_text: error calling "
"BreakIterator::setText()");
"BreakIterator::setText()", 0);

/* When ICU clones the UText, it does not copy the buffer, so we have to
* keep the string buffer around by holding a reference to its zval. This
Expand Down
38 changes: 19 additions & 19 deletions ext/intl/calendar/gregoriancalendar_methods.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ static inline GregorianCalendar *fetch_greg(Calendar_object *co) {
return (GregorianCalendar*)co->ucal;
}

static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS)
static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS, zend_bool is_constructor)
{
zval *tz_object = NULL;
zval args_a[6] = {0},
Expand All @@ -51,18 +51,18 @@ static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS)
// parameter number validation / variant determination
if (ZEND_NUM_ARGS() > 6 ||
zend_get_parameters_array_ex(ZEND_NUM_ARGS(), args) == FAILURE) {
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: too many arguments", 0);
intl_error_set_ex(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: too many arguments", 0, is_constructor);
Z_OBJ_P(return_value) = NULL;
return;
}
for (variant = ZEND_NUM_ARGS();
variant > 0 && Z_TYPE(args[variant - 1]) == IS_NULL;
variant--) {}
if (variant == 4) {
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
intl_error_set_ex(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: no variant with 4 arguments "
"(excluding trailing NULLs)", 0);
"(excluding trailing NULLs)", 0, is_constructor);
Z_OBJ_P(return_value) = NULL;
return;
}
Expand All @@ -71,17 +71,17 @@ static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS)
if (variant <= 2) {
if (zend_parse_parameters(MIN(ZEND_NUM_ARGS(), 2),
"|z!s!", &tz_object, &locale, &locale_len) == FAILURE) {
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: bad arguments", 0);
intl_error_set_ex(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: bad arguments", 0, is_constructor);
Z_OBJ_P(return_value) = NULL;
return;
}
}
if (variant > 2 && zend_parse_parameters(ZEND_NUM_ARGS(),
"lll|lll", &largs[0], &largs[1], &largs[2], &largs[3], &largs[4],
&largs[5]) == FAILURE) {
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: bad arguments", 0);
intl_error_set_ex(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: bad arguments", 0, is_constructor);
Z_OBJ_P(return_value) = NULL;
return;
}
Expand All @@ -104,8 +104,8 @@ static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS)
gcal = new GregorianCalendar(tz, Locale::createFromName(locale),
status);
if (U_FAILURE(status)) {
intl_error_set(NULL, status, "intlgregcal_create_instance: error "
"creating ICU GregorianCalendar from time zone and locale", 0);
intl_error_set_ex(NULL, status, "intlgregcal_create_instance: error "
"creating ICU GregorianCalendar from time zone and locale", 0, is_constructor);
if (gcal) {
delete gcal;
}
Expand All @@ -117,9 +117,9 @@ static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS)
// From date/time (3, 5 or 6 arguments)
for (int i = 0; i < variant; i++) {
if (largs[i] < INT32_MIN || largs[i] > INT32_MAX) {
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
intl_error_set_ex(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: at least one of the arguments"
" has an absolute value that is too large", 0);
" has an absolute value that is too large", 0, is_constructor);
Z_OBJ_P(return_value) = NULL;
return;
}
Expand All @@ -137,8 +137,8 @@ static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS)
status);
}
if (U_FAILURE(status)) {
intl_error_set(NULL, status, "intlgregcal_create_instance: error "
"creating ICU GregorianCalendar from date", 0);
intl_error_set_ex(NULL, status, "intlgregcal_create_instance: error "
"creating ICU GregorianCalendar from date", 0, is_constructor);
if (gcal) {
delete gcal;
}
Expand All @@ -154,10 +154,10 @@ static void _php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAMETERS)
strlen(tzinfo->name), US_INV);
#endif
if (tzstr.isBogus()) {
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
intl_error_set_ex(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"intlgregcal_create_instance: could not create UTF-8 string "
"from PHP's default timezone name (see date_default_timezone_get())",
0);
0, is_constructor);
delete gcal;
Z_OBJ_P(return_value) = NULL;
return;
Expand All @@ -179,7 +179,7 @@ U_CFUNC PHP_FUNCTION(intlgregcal_create_instance)
object_init_ex(return_value, GregorianCalendar_ce_ptr);
ZVAL_COPY_VALUE(&orig, return_value);

_php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAM_PASSTHRU);
_php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAM_PASSTHRU, 0);

if (Z_TYPE_P(return_value) == IS_OBJECT && Z_OBJ_P(return_value) == NULL) {
zval_dtor(&orig);
Expand All @@ -194,7 +194,7 @@ U_CFUNC PHP_METHOD(IntlGregorianCalendar, __construct)

return_value = getThis();
//changes this to IS_NULL (without first destroying) if there's an error
_php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAM_PASSTHRU);
_php_intlgregcal_constructor_body(INTERNAL_FUNCTION_PARAM_PASSTHRU, 1);

if (Z_TYPE_P(return_value) == IS_OBJECT && Z_OBJ_P(return_value) == NULL) {
zend_object_store_ctor_failed(Z_OBJ(orig_this));
Expand Down
12 changes: 6 additions & 6 deletions ext/intl/collator/collator_create.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include "intl_data.h"

/* {{{ */
static void collator_ctor(INTERNAL_FUNCTION_PARAMETERS)
static void collator_ctor(INTERNAL_FUNCTION_PARAMETERS, zend_bool is_constructor)
{
const char* locale;
size_t locale_len = 0;
Expand All @@ -38,8 +38,8 @@ static void collator_ctor(INTERNAL_FUNCTION_PARAMETERS)
if( zend_parse_parameters( ZEND_NUM_ARGS(), "s",
&locale, &locale_len ) == FAILURE )
{
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"collator_create: unable to parse input params", 0 );
intl_error_set_ex( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"collator_create: unable to parse input params", 0, is_constructor );
zval_dtor(return_value);
RETURN_NULL();
}
Expand All @@ -53,7 +53,7 @@ static void collator_ctor(INTERNAL_FUNCTION_PARAMETERS)

/* Open ICU collator. */
co->ucoll = ucol_open( locale, COLLATOR_ERROR_CODE_P( co ) );
INTL_CTOR_CHECK_STATUS(co, "collator_create: unable to open ICU collator");
INTL_CTOR_CHECK_STATUS(co, "collator_create: unable to open ICU collator", is_constructor);
}
/* }}} */

Expand All @@ -63,7 +63,7 @@ static void collator_ctor(INTERNAL_FUNCTION_PARAMETERS)
PHP_FUNCTION( collator_create )
{
object_init_ex( return_value, Collator_ce_ptr );
collator_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
collator_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU, 0);
}
/* }}} */

Expand All @@ -75,7 +75,7 @@ PHP_METHOD( Collator, __construct )
zval orig_this = *getThis();

return_value = getThis();
collator_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
collator_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU, 1);

if (Z_TYPE_P(return_value) == IS_OBJECT && Z_OBJ_P(return_value) == NULL) {
zend_object_store_ctor_failed(Z_OBJ(orig_this));
Expand Down
11 changes: 9 additions & 2 deletions ext/intl/converter/converter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <unicode/ustring.h>

#include "../intl_error.h"
#include "../intl_common.h"

typedef struct _php_converter_object {
UConverter *src, *dest;
Expand Down Expand Up @@ -556,11 +557,17 @@ static PHP_METHOD(UConverter, __construct) {
size_t src_len = sizeof("utf-8") - 1;
char *dest = src;
size_t dest_len = src_len;
zend_error_handling zeh;
int rv;

intl_error_reset(NULL);

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|s!s!",
&dest, &dest_len, &src, &src_len) == FAILURE) {
zend_replace_error_handling(EH_THROW, IntlException_ce_ptr, &zeh TSRMLS_CC);
rv = zend_parse_parameters(ZEND_NUM_ARGS(), "|s!s!",
&dest, &dest_len, &src, &src_len);
zend_restore_error_handling(&zeh TSRMLS_CC);
Copy link
Member

Choose a reason for hiding this comment

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

indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, was spaces instead of tab.


if (rv == FAILURE) {
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
"UConverter::__construct(): bad arguments", 0);
return;
Expand Down
20 changes: 11 additions & 9 deletions ext/intl/dateformat/dateformat_create.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ extern "C" {
#include "dateformat_helpers.h"

/* {{{ */
static void datefmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
static void datefmt_ctor(INTERNAL_FUNCTION_PARAMETERS, zend_bool is_constructor)
{
zval *object;

Expand Down Expand Up @@ -64,8 +64,8 @@ static void datefmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
if (zend_parse_parameters(ZEND_NUM_ARGS(), "sll|zzs",
&locale_str, &locale_len, &date_type, &time_type, &timezone_zv,
&calendar_zv, &pattern_str, &pattern_str_len) == FAILURE) {
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR, "datefmt_create: "
"unable to parse input parameters", 0);
intl_error_set_ex( NULL, U_ILLEGAL_ARGUMENT_ERROR, "datefmt_create: "
"unable to parse input parameters", 0, is_constructor);
Z_OBJ_P(return_value) = NULL;
return;
}
Expand All @@ -79,6 +79,8 @@ static void datefmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
DATE_FORMAT_METHOD_FETCH_OBJECT_NO_CHECK;

if (DATE_FORMAT_OBJECT(dfo) != NULL) {
// This is __construct being called on an instance - it is not
// a constructor.
Copy link
Member

Choose a reason for hiding this comment

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

C++ comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

intl_errors_set(INTL_DATA_ERROR_P(dfo), U_ILLEGAL_ARGUMENT_ERROR,
"datefmt_create: cannot call constructor twice", 0);
return;
Expand Down Expand Up @@ -110,8 +112,8 @@ static void datefmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
pattern_str, pattern_str_len, &INTL_DATA_ERROR_CODE(dfo));
if (U_FAILURE(INTL_DATA_ERROR_CODE(dfo))) {
/* object construction -> only set global error */
intl_error_set(NULL, INTL_DATA_ERROR_CODE(dfo), "datefmt_create: "
"error converting pattern to UTF-16", 0);
intl_error_set_ex(NULL, INTL_DATA_ERROR_CODE(dfo), "datefmt_create: "
"error converting pattern to UTF-16", 0, is_constructor);
goto error;
}
}
Expand Down Expand Up @@ -139,8 +141,8 @@ static void datefmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
df->adoptTimeZone(timezone);
}
} else {
intl_error_set(NULL, INTL_DATA_ERROR_CODE(dfo), "datefmt_create: date "
"formatter creation failed", 0);
intl_error_set_ex(NULL, INTL_DATA_ERROR_CODE(dfo), "datefmt_create: date "
"formatter creation failed", 0, is_constructor);
goto error;
}

Expand Down Expand Up @@ -175,7 +177,7 @@ static void datefmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
U_CFUNC PHP_FUNCTION( datefmt_create )
{
object_init_ex( return_value, IntlDateFormatter_ce_ptr );
datefmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
datefmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU, 0);
if (Z_TYPE_P(return_value) == IS_OBJECT && Z_OBJ_P(return_value) == NULL) {
RETURN_NULL();
}
Expand All @@ -192,7 +194,7 @@ U_CFUNC PHP_METHOD( IntlDateFormatter, __construct )
/* return_value param is being changed, therefore we will always return
* NULL here */
return_value = getThis();
datefmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
datefmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU, 1);

if (Z_TYPE_P(return_value) == IS_OBJECT && Z_OBJ_P(return_value) == NULL) {
zend_object_store_ctor_failed(Z_OBJ(orig_this));
Expand Down
14 changes: 7 additions & 7 deletions ext/intl/formatter/formatter_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include "intl_convert.h"

/* {{{ */
static void numfmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
static void numfmt_ctor(INTERNAL_FUNCTION_PARAMETERS, zend_bool is_constructor)
{
const char* locale;
char* pattern = NULL;
Expand All @@ -39,8 +39,8 @@ static void numfmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
if( zend_parse_parameters( ZEND_NUM_ARGS(), "sl|s",
&locale, &locale_len, &style, &pattern, &pattern_len ) == FAILURE )
{
intl_error_set( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"numfmt_create: unable to parse input parameters", 0 );
intl_error_set_ex( NULL, U_ILLEGAL_ARGUMENT_ERROR,
"numfmt_create: unable to parse input parameters", 0, is_constructor );
Z_OBJ_P(return_value) = NULL;
return;
}
Expand All @@ -52,7 +52,7 @@ static void numfmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
/* Convert pattern (if specified) to UTF-16. */
if(pattern && pattern_len) {
intl_convert_utf8_to_utf16(&spattern, &spattern_len, pattern, pattern_len, &INTL_DATA_ERROR_CODE(nfo));
INTL_CTOR_CHECK_STATUS(nfo, "numfmt_create: error converting pattern to UTF-16");
INTL_CTOR_CHECK_STATUS(nfo, "numfmt_create: error converting pattern to UTF-16", is_constructor);
}

if(locale_len == 0) {
Expand All @@ -66,7 +66,7 @@ static void numfmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
efree(spattern);
}

INTL_CTOR_CHECK_STATUS(nfo, "numfmt_create: number formatter creation failed");
INTL_CTOR_CHECK_STATUS(nfo, "numfmt_create: number formatter creation failed", is_constructor);
}
/* }}} */

Expand All @@ -78,7 +78,7 @@ static void numfmt_ctor(INTERNAL_FUNCTION_PARAMETERS)
PHP_FUNCTION( numfmt_create )
{
object_init_ex( return_value, NumberFormatter_ce_ptr );
numfmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
numfmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU, 0);
if (Z_TYPE_P(return_value) == IS_OBJECT && Z_OBJ_P(return_value) == NULL) {
RETURN_NULL();
}
Expand All @@ -93,7 +93,7 @@ PHP_METHOD( NumberFormatter, __construct )
zval orig_this = *getThis();

return_value = getThis();
numfmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU);
numfmt_ctor(INTERNAL_FUNCTION_PARAM_PASSTHRU, 1);

if (Z_TYPE_P(return_value) == IS_OBJECT && Z_OBJ_P(return_value) == NULL) {
zend_object_store_ctor_failed(Z_OBJ(orig_this));
Expand Down
2 changes: 2 additions & 0 deletions ext/intl/intl_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,6 @@
#define INTL_Z_STRVAL_P(str) (UChar*) Z_STRVAL_P(str)
#define INTL_Z_STRLEN_P(str) UCHARS( Z_STRLEN_P(str) )

extern zend_class_entry *IntlException_ce_ptr;

#endif /* INTL_COMMON_H */
4 changes: 2 additions & 2 deletions ext/intl/intl_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ typedef struct _intl_data {
}

/* Check status, if error - destroy value and exit */
#define INTL_CTOR_CHECK_STATUS(obj, msg) \
#define INTL_CTOR_CHECK_STATUS(obj, msg, forceException) \
intl_error_set_code( NULL, INTL_DATA_ERROR_CODE((obj)) ); \
if( U_FAILURE( INTL_DATA_ERROR_CODE((obj)) ) ) \
{ \
intl_errors_set_custom_msg( INTL_DATA_ERROR_P((obj)), msg, 0 ); \
intl_errors_set_custom_msg_ex( INTL_DATA_ERROR_P((obj)), msg, 0, forceException ); \
/* yes, this is ugly, but it alreay is */ \
if (return_value != getThis()) { \
zval_dtor(return_value); \
Expand Down
Loading