Skip to content

Throwable #1274

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

Throwable #1274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e282290
Rename REGISTER_ITERATOR_INTERFACE macro to REGISTER_INTERFACE (as it…
sebastianbergmann May 13, 2015
85e87d7
Introduce Throwable interface
sebastianbergmann May 13, 2015
412843b
Let the default Exception class implement the Throwable interface
sebastianbergmann May 13, 2015
6efb8bf
* Rename ErrorException to Error
sebastianbergmann May 13, 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
46 changes: 33 additions & 13 deletions Zend/zend_exceptions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

static zend_class_entry *base_exception_ce;
static zend_class_entry *default_exception_ce;
static zend_class_entry *error_exception_ce;
static zend_class_entry *error_ce;
static zend_class_entry *engine_exception_ce;
static zend_class_entry *parse_exception_ce;
static zend_class_entry *type_exception_ce;
Expand Down Expand Up @@ -740,6 +740,7 @@ void zend_register_default_exception(void) /* {{{ */
base_exception_ce->create_object = NULL;
memcpy(&default_exception_handlers, zend_get_std_object_handlers(), sizeof(zend_object_handlers));
default_exception_handlers.clone_obj = NULL;
zend_class_implements(base_exception_ce, 1, zend_ce_throwable);
Copy link
Contributor

Choose a reason for hiding this comment

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

indent

Copy link
Contributor

Choose a reason for hiding this comment

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

its spaces, wheras the code needs to be with tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be great if we could focus on conceptual problems and implementation issues before critiquing whitespace.


zend_declare_property_string(base_exception_ce, "message", sizeof("message")-1, "", ZEND_ACC_PROTECTED);
zend_declare_property_string(base_exception_ce, "string", sizeof("string")-1, "", ZEND_ACC_PRIVATE);
Expand Down Expand Up @@ -772,21 +773,40 @@ void zend_register_default_exception(void) /* {{{ */
}
} ZEND_HASH_FOREACH_END();

INIT_CLASS_ENTRY(ce, "ErrorException", error_exception_functions);
error_exception_ce = zend_register_internal_class_ex(&ce, default_exception_ce);
error_exception_ce->create_object = zend_error_exception_new;
zend_declare_property_long(error_exception_ce, "severity", sizeof("severity")-1, E_ERROR, ZEND_ACC_PROTECTED);
INIT_CLASS_ENTRY(ce, "Error", NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Im' not sure at all about this name.

This will break BC as many people may have an Error class already, and that's why it is named ErrorException.
I think this kind of break needs consensus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

EngineError? PHPError?

I wouldn't call it ErrorException since many people's expectation would be that it inherits from Exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record Java is similar to this proposal.
They have Error an Exception, both of those are descending from Throwable:
http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/Error.html
I could be convinced that having a generic name like Error in the global namespace would be ok to use to have a decently named implementation, and a quick look did not turn up anything wide-spread plus spotting and renaming a conflicting classname is one of the easier task when upgrading.

Copy link
Contributor

Choose a reason for hiding this comment

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

and a quick look did not turn up anything wide-spread plus spotting and renaming a conflicting classname is one of the easier task when upgrading.

Plus the world has (hopefully) been using namespaces long enough for it not to matter much.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but take Symfony2 for example.
You can find such code :

use Foo\Bar\String;

It gives : "Fatal error : class String is reserved." when it gets compiled, because the compiler checks for reserved words when use alias are resolved, at compile time.

So I guess some people may have written : use Foo\Bar\Error; and this will break as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpauli use Foo\Bar\Error won't break, because this is a namespaced class. So you can still reference the base Error via \Error, just like any built-in class. The String reservation was because string is not a namespaced class, but a type identifier (so having use Foo as String would make little sense).

error_ce = zend_register_internal_class_ex(&ce, base_exception_ce);
error_ce->create_object = zend_default_exception_new;
zend_declare_property_long(error_ce, "severity", sizeof("severity")-1, E_ERROR, ZEND_ACC_PROTECTED);

INIT_CLASS_ENTRY(ce, "EngineException", NULL);
engine_exception_ce = zend_register_internal_class_ex(&ce, base_exception_ce);
/* A trick, to make visible private properties of BaseException */
ZEND_HASH_FOREACH_PTR(&error_ce->properties_info, prop) {
if (prop->flags & ZEND_ACC_SHADOW) {
if (prop->name->len == sizeof("\0BaseException\0string")-1) {
prop->flags &= ~ZEND_ACC_SHADOW;
prop->flags |= ZEND_ACC_PRIVATE;
prop->ce = error_ce;
} else if (prop->name->len == sizeof("\0BaseException\0trace")-1) {
prop->flags &= ~ZEND_ACC_SHADOW;
prop->flags |= ZEND_ACC_PRIVATE;
prop->ce = error_ce;
} else if (prop->name->len == sizeof("\0BaseException\0previous")-1) {
prop->flags &= ~ZEND_ACC_SHADOW;
prop->flags |= ZEND_ACC_PRIVATE;
prop->ce = error_ce;
}
}
} ZEND_HASH_FOREACH_END();

INIT_CLASS_ENTRY(ce, "EngineError", NULL);
engine_exception_ce = zend_register_internal_class_ex(&ce, error_ce);
engine_exception_ce->create_object = zend_default_exception_new;

INIT_CLASS_ENTRY(ce, "ParseException", NULL);
parse_exception_ce = zend_register_internal_class_ex(&ce, base_exception_ce);
INIT_CLASS_ENTRY(ce, "ParseError", NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Why name an exception with the "Error" wording instead of "Exception" ? It will be thrown as an exception. That may confuse people

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

parse_exception_ce = zend_register_internal_class_ex(&ce, error_ce);
parse_exception_ce->create_object = zend_default_exception_new;

INIT_CLASS_ENTRY(ce, "TypeException", NULL);
type_exception_ce = zend_register_internal_class_ex(&ce, engine_exception_ce);
INIT_CLASS_ENTRY(ce, "TypeError", NULL);
type_exception_ce = zend_register_internal_class_ex(&ce, error_ce);
type_exception_ce->create_object = zend_default_exception_new;
}
/* }}} */
Expand All @@ -803,9 +823,9 @@ ZEND_API zend_class_entry *zend_exception_get_default(void) /* {{{ */
}
/* }}} */

ZEND_API zend_class_entry *zend_get_error_exception(void) /* {{{ */
ZEND_API zend_class_entry *zend_get_error(void) /* {{{ */
{
return error_exception_ce;
return error_ce;
}
/* }}} */

Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_exceptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ void zend_register_default_exception(void);

ZEND_API zend_class_entry *zend_exception_get_base(void);
ZEND_API zend_class_entry *zend_exception_get_default(void);
ZEND_API zend_class_entry *zend_get_error_exception(void);
ZEND_API zend_class_entry *zend_get_error(void);
ZEND_API zend_class_entry *zend_get_engine_exception(void);
ZEND_API zend_class_entry *zend_get_parse_exception(void);
ZEND_API zend_class_entry *zend_get_type_exception(void);
Expand Down
24 changes: 18 additions & 6 deletions Zend/zend_interfaces.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ ZEND_API zend_class_entry *zend_ce_aggregate;
ZEND_API zend_class_entry *zend_ce_iterator;
ZEND_API zend_class_entry *zend_ce_arrayaccess;
ZEND_API zend_class_entry *zend_ce_serializable;
ZEND_API zend_class_entry *zend_ce_throwable;

/* {{{ zend_call_method
Only returns the returned zval if retval_ptr != NULL */
Expand Down Expand Up @@ -502,6 +503,13 @@ static int zend_implement_serializable(zend_class_entry *interface, zend_class_e
}
/* }}}*/

/* {{{ zend_implement_throwable */
static int zend_implement_throwable(zend_class_entry *interface, zend_class_entry *class_type)
{
return SUCCESS;
}
/* }}}*/

/* {{{ function tables */
const zend_function_entry zend_funcs_aggregate[] = {
ZEND_ABSTRACT_ME(iterator, getIterator, NULL)
Expand Down Expand Up @@ -549,9 +557,11 @@ const zend_function_entry zend_funcs_serializable[] = {
ZEND_FENTRY(unserialize, NULL, arginfo_serializable_serialize, ZEND_ACC_PUBLIC|ZEND_ACC_ABSTRACT|ZEND_ACC_CTOR)
ZEND_FE_END
};

const zend_function_entry *zend_funcs_throwable = NULL;
/* }}} */

#define REGISTER_ITERATOR_INTERFACE(class_name, class_name_str) \
#define REGISTER_INTERFACE(class_name, class_name_str) \
Copy link
Member

Choose a reason for hiding this comment

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

While I find this logical, don't forget that this will break some custom extensions that used the old macro.
I'm not against renaming it, but this must be documented into UPGRADING.INTERNALS , and perhaps for 7.0, keep an alias macro for compat ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

{\
zend_class_entry ce;\
INIT_CLASS_ENTRY(ce, # class_name_str, zend_funcs_ ## class_name) \
Expand All @@ -565,17 +575,19 @@ const zend_function_entry zend_funcs_serializable[] = {
/* {{{ zend_register_interfaces */
ZEND_API void zend_register_interfaces(void)
{
REGISTER_ITERATOR_INTERFACE(traversable, Traversable);
REGISTER_INTERFACE(traversable, Traversable);

REGISTER_ITERATOR_INTERFACE(aggregate, IteratorAggregate);
REGISTER_INTERFACE(aggregate, IteratorAggregate);
REGISTER_ITERATOR_IMPLEMENT(aggregate, traversable);

REGISTER_ITERATOR_INTERFACE(iterator, Iterator);
REGISTER_INTERFACE(iterator, Iterator);
REGISTER_ITERATOR_IMPLEMENT(iterator, traversable);

REGISTER_ITERATOR_INTERFACE(arrayaccess, ArrayAccess);
REGISTER_INTERFACE(arrayaccess, ArrayAccess);

REGISTER_INTERFACE(serializable, Serializable)

REGISTER_ITERATOR_INTERFACE(serializable, Serializable)
REGISTER_INTERFACE(throwable, Throwable)
}
/* }}} */

Expand Down
1 change: 1 addition & 0 deletions Zend/zend_interfaces.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ extern ZEND_API zend_class_entry *zend_ce_aggregate;
extern ZEND_API zend_class_entry *zend_ce_iterator;
extern ZEND_API zend_class_entry *zend_ce_arrayaccess;
extern ZEND_API zend_class_entry *zend_ce_serializable;
extern ZEND_API zend_class_entry *zend_ce_throwable;

typedef struct _zend_user_iterator {
zend_object_iterator it;
Expand Down