[RFC][VOTE] Constructor behaviour of internal classes

9 years ago by Michael Wallner — view source

unread

Hi List,

The 'Constructor behaviour of internal classes' RFC is now in voting.
Please note, it's the coding standard that is being voted on. If
anyone thinks I've implemented the changes in a way that is less
awesome then there is no reason the changes couldn't be improved.

Additionally, while writing the change I noticed some things that were
already present in the code, that are outside the scope of the RFC but
ought to be fixed for the release of PHP 7.

Multiple examples of a generic Exception being thrown rather than a
specific exception being thrown.

Code generating an error notice and throwing an exception. It should
be one or the other, not both.

The text of exceptions in Intl not always being as informative as
the error message, which could be improved.

But as I said, the vote is on whether the standard behaviour of either
returning a working instance or throwing an exception, is the standard
behaviour we want in PHP.

For the lazy people, like me:
https://wiki.php.net/rfc/internal_constructor_behaviour#voting <https://wiki.php.net/rfc/internal_constructor_behaviour#voting

9 years ago by Dmitry Stogov — view source

unread

Hi Dan,

Your RFC is going to be accepted and I support the idea, however the patch
looks a bit surprising to me.

Actually, returning NULL from constructors wasn't simple :)
We have special function to do this - zend_ctor_make_null() and some tricks
in the VM.
I made just a quick look over your patch but didn't find any references to
them.

I'll try to improve the patch on next week. Please don't commit it yet.
Nikita, could you also take a quick look.

Thanks. Dmitry.

Hi List,

The 'Constructor behaviour of internal classes' RFC is now in voting.
Please note, it's the coding standard that is being voted on. If
anyone thinks I've implemented the changes in a way that is less
awesome then there is no reason the changes couldn't be improved.

Additionally, while writing the change I noticed some things that were
already present in the code, that are outside the scope of the RFC but
ought to be fixed for the release of PHP 7.

Multiple examples of a generic Exception being thrown rather than a
specific exception being thrown.

Code generating an error notice and throwing an exception. It should
be one or the other, not both.

The text of exceptions in Intl not always being as informative as
the error message, which could be improved.

But as I said, the vote is on whether the standard behaviour of either
returning a working instance or throwing an exception, is the standard
behaviour we want in PHP.

cheers
Dan
Ack

9 years ago by Dan Ackroyd — view source

unread

Hi Dmitry,

however the patch looks a bit surprising to me.
We have special function to do this - zend_ctor_make_null() and some tricks in the VM.
I made just a quick look over your patch but didn't find any references to them.

Surprising is usually not good, so let me see if I can explain.

I touched the minimal amount of code needed to achieve the desired
behaviour. For the intl classes, the exception is being thrown by
telling the intl error handling code to use an exception, no matter
what the intl.use_exceptions setting is, if the error was emitted
during a constructor:
https://github.com/Danack/php-src/blob/InternalClassClean/ext/intl/intl_error.c#L114

I didn't touch any of the code zend_ctor_make_null. I guess if it's
only used in these special cases, and they are going away it could
also be removed? But it sounds like that would be a job for someone
who understands that bit.

Please don't commit it yet.

That won't be difficult, I don't have commit rights, and don't
particularly want them.

Nikita, could you also take a quick look.

Nikita pointed out that I may have missed a couple of classes. I'll
try to get those updated before you have a look.

cheers
Dan

9 years ago by Dmitry Stogov — view source

unread

Hi Dan,

The updated patch is at https://github.com/php/php-src/pull/1205

The main difference is in ext/intl.
If you don't see any problems I can commit it.

I didn't think about the classes you missed.

Thanks. Dmitry.

Hi Dmitry,

however the patch looks a bit surprising to me.
We have special function to do this - zend_ctor_make_null() and some
tricks in the VM.
I made just a quick look over your patch but didn't find any references
to them.

Surprising is usually not good, so let me see if I can explain.

I touched the minimal amount of code needed to achieve the desired
behaviour. For the intl classes, the exception is being thrown by
telling the intl error handling code to use an exception, no matter
what the intl.use_exceptions setting is, if the error was emitted
during a constructor:

https://github.com/Danack/php-src/blob/InternalClassClean/ext/intl/intl_error.c#L114

I didn't touch any of the code zend_ctor_make_null. I guess if it's
only used in these special cases, and they are going away it could
also be removed? But it sounds like that would be a job for someone
who understands that bit.

Please don't commit it yet.

That won't be difficult, I don't have commit rights, and don't
particularly want them.

Nikita, could you also take a quick look.

Nikita pointed out that I may have missed a couple of classes. I'll
try to get those updated before you have a look.

cheers
Dan

9 years ago by Dmitry Stogov — view source

unread

Hi Dan,

any update? should I commit it?
or do you see any problems?

Thanks. Dmitry.

Hi Dan,

The updated patch is at https://github.com/php/php-src/pull/1205

The main difference is in ext/intl.
If you don't see any problems I can commit it.

I didn't think about the classes you missed.

Thanks. Dmitry.

On Fri, Mar 27, 2015 at 7:32 PM, Dan Ackroyd danack@basereality.com
wrote:

Hi Dmitry,

however the patch looks a bit surprising to me.
We have special function to do this - zend_ctor_make_null() and some
tricks in the VM.
I made just a quick look over your patch but didn't find any references
to them.

Surprising is usually not good, so let me see if I can explain.

I touched the minimal amount of code needed to achieve the desired
behaviour. For the intl classes, the exception is being thrown by
telling the intl error handling code to use an exception, no matter
what the intl.use_exceptions setting is, if the error was emitted
during a constructor:

https://github.com/Danack/php-src/blob/InternalClassClean/ext/intl/intl_error.c#L114

I didn't touch any of the code zend_ctor_make_null. I guess if it's
only used in these special cases, and they are going away it could
also be removed? But it sounds like that would be a job for someone
who understands that bit.

Please don't commit it yet.

That won't be difficult, I don't have commit rights, and don't
particularly want them.

Nikita, could you also take a quick look.

Nikita pointed out that I may have missed a couple of classes. I'll
try to get those updated before you have a look.

cheers
Dan

9 years ago by Nikita Popov — view source

unread

Hi Dan,

The updated patch is at https://github.com/php/php-src/pull/1205

The main difference is in ext/intl.
If you don't see any problems I can commit it.

I didn't think about the classes you missed.

Thanks. Dmitry.

I'm wondering, if we implement throwing zpp by using EH_THROW mode, which
relies on converting warnings to exceptions, does that mean that the same
code using strict_types=1 mode will start throwing TypeException instead of
whatever exception type is passed to replace_error_handling? If so, that
would be pretty weird. Maybe we should just always throw TypeException for
this? Could also add a zpp flag for doing that as all the code for throwing
TypeExceptions is already present, we just need to trigger it.

Nikita

9 years ago by Dmitry Stogov — view source

unread

hi Nikita,

I don't care about this a lot. I reworked this patch just because it missed
few details, and then found and fixed mistake in ext/intl.
If you think TypeException is better (I think this makes sense), please
implement it on top and commit.

Thanks. Dmitry.

Hi Dan,

The updated patch is at https://github.com/php/php-src/pull/1205

The main difference is in ext/intl.
If you don't see any problems I can commit it.

I didn't think about the classes you missed.

Thanks. Dmitry.

I'm wondering, if we implement throwing zpp by using EH_THROW mode, which
relies on converting warnings to exceptions, does that mean that the same
code using strict_types=1 mode will start throwing TypeException instead of
whatever exception type is passed to replace_error_handling? If so, that
would be pretty weird. Maybe we should just always throw TypeException for
this? Could also add a zpp flag for doing that as all the code for throwing
TypeExceptions is already present, we just need to trigger it.

Nikita

9 years ago by Dan Ackroyd — view source

unread

Hi Dmitry,

Your approach is definitely a better one, and I have no objection to
it whatsoever.

Sorry, I was too busy to look deeply at each class but I can't see any problems.

Nikita Popov wrote:

does that mean that the same code using strict_types=1 mode will start
throwing TypeException instead of whatever exception type is passed
to replace_error_handling?

That's going to affect everything isn't it - not just this RFC?

However I think that is the correct behaviour. It more closely
resembles the behaviour that will be seen in userland code, and is
more 'semantically' meaningful i.e. it allows people to tell the cases
between the two types of errors apart.

It probably bears more thinking about though.

cheers
Dan

$formatInfoList = [
["en_US", "{0,number,integer} monkeys on {1,number,integer} trees
make {2,number} monkeys per tree"],
["en_US", '{this was made intentionally incorrect}'], //valid type
but wrong value
["en_US", new StdClass] //wrong type
]

foreach ($formatInfoList as $formatInfo) {
list($locale, $pattern) = formatInfo;
try {
$mf = new MessageFormatter($locale, $pattern);
echo $mf->format(array(4560, 123, 4560/123));
}
catch(IntlException $ie) {
// A strict type-est person would not want this to catch the case where
// the wrong type has been passed, as it's not an exception
related to Intl
// it's an exception related to the type being wrong.
}
}

9 years ago by Dmitry Stogov — view source

unread

Hi Dmitry,

Your approach is definitely a better one, and I have no objection to
it whatsoever.

Your patch was quite good. You just didn't know what this NULL return
required a hack in the PHP core :)

Sorry, I was too busy to look deeply at each class but I can't see any
problems.

Great.

Nikita, do you like me to commit this PR or will you take care about it?

Thanks. Dmitry.

Nikita Popov wrote:

does that mean that the same code using strict_types=1 mode will start
throwing TypeException instead of whatever exception type is passed
to replace_error_handling?

That's going to affect everything isn't it - not just this RFC?

However I think that is the correct behaviour. It more closely
resembles the behaviour that will be seen in userland code, and is
more 'semantically' meaningful i.e. it allows people to tell the cases
between the two types of errors apart.

It probably bears more thinking about though.

cheers
Dan

$formatInfoList = [
["en_US", "{0,number,integer} monkeys on {1,number,integer} trees
make {2,number} monkeys per tree"],
["en_US", '{this was made intentionally incorrect}'], //valid type
but wrong value
["en_US", new StdClass] //wrong type
]

foreach ($formatInfoList as $formatInfo) {
list($locale, $pattern) = formatInfo;
try {
$mf = new MessageFormatter($locale, $pattern);
echo $mf->format(array(4560, 123, 4560/123));
}
catch(IntlException $ie) {
// A strict type-est person would not want this to catch the case
where
// the wrong type has been passed, as it's not an exception
related to Intl
// it's an exception related to the type being wrong.
}
}

9 years ago by Dmitry Stogov — view source

unread

committed into master.
Dan, please update the RFC accordingly.

Thanks. Dmitry.

On Wed, Apr 1, 2015 at 4:31 AM, Dan Ackroyd danack@basereality.com
wrote:

Hi Dmitry,

Your approach is definitely a better one, and I have no objection to
it whatsoever.

Your patch was quite good. You just didn't know what this NULL return
required a hack in the PHP core :)

Sorry, I was too busy to look deeply at each class but I can't see any
problems.

Great.

Nikita, do you like me to commit this PR or will you take care about it?

Thanks. Dmitry.

Nikita Popov wrote:

does that mean that the same code using strict_types=1 mode will start
throwing TypeException instead of whatever exception type is passed
to replace_error_handling?

That's going to affect everything isn't it - not just this RFC?

However I think that is the correct behaviour. It more closely
resembles the behaviour that will be seen in userland code, and is
more 'semantically' meaningful i.e. it allows people to tell the cases
between the two types of errors apart.

It probably bears more thinking about though.

cheers
Dan

$formatInfoList = [
["en_US", "{0,number,integer} monkeys on {1,number,integer} trees
make {2,number} monkeys per tree"],
["en_US", '{this was made intentionally incorrect}'], //valid type
but wrong value
["en_US", new StdClass] //wrong type
]

foreach ($formatInfoList as $formatInfo) {
list($locale, $pattern) = formatInfo;
try {
$mf = new MessageFormatter($locale, $pattern);
echo $mf->format(array(4560, 123, 4560/123));
}
catch(IntlException $ie) {
// A strict type-est person would not want this to catch the
case where
// the wrong type has been passed, as it's not an exception
related to Intl
// it's an exception related to the type being wrong.
}
}

9 years ago by Nikita Popov — view source

unread

Hi Dmitry,

Your approach is definitely a better one, and I have no objection to
it whatsoever.

Sorry, I was too busy to look deeply at each class but I can't see any
problems.

Nikita Popov wrote:

does that mean that the same code using strict_types=1 mode will start
throwing TypeException instead of whatever exception type is passed
to replace_error_handling?

That's going to affect everything isn't it - not just this RFC?

However I think that is the correct behaviour. It more closely
resembles the behaviour that will be seen in userland code, and is
more 'semantically' meaningful i.e. it allows people to tell the cases
between the two types of errors apart.

It probably bears more thinking about though.

cheers
Dan

$formatInfoList = [
["en_US", "{0,number,integer} monkeys on {1,number,integer} trees
make {2,number} monkeys per tree"],
["en_US", '{this was made intentionally incorrect}'], //valid type
but wrong value
["en_US", new StdClass] //wrong type
]

foreach ($formatInfoList as $formatInfo) {
list($locale, $pattern) = formatInfo;
try {
$mf = new MessageFormatter($locale, $pattern);
echo $mf->format(array(4560, 123, 4560/123));
}
catch(IntlException $ie) {
// A strict type-est person would not want this to catch the case
where
// the wrong type has been passed, as it's not an exception
related to Intl
// it's an exception related to the type being wrong.
}
}

PR for throwing TypeException (independently of strict mode) for zpp
failures in constructors is here: https://github.com/php/php-src/pull/1215
Will apply it if nobody sees a problem with it.

Nikita

9 years ago by Dmitry Stogov — view source

unread

I don' t see any problems.

Thanks. Dmitry.

On Wed, Apr 1, 2015 at 3:31 AM, Dan Ackroyd danack@basereality.com
wrote:

Hi Dmitry,

Your approach is definitely a better one, and I have no objection to
it whatsoever.

Sorry, I was too busy to look deeply at each class but I can't see any
problems.

Nikita Popov wrote:

does that mean that the same code using strict_types=1 mode will start
throwing TypeException instead of whatever exception type is passed
to replace_error_handling?

That's going to affect everything isn't it - not just this RFC?

However I think that is the correct behaviour. It more closely
resembles the behaviour that will be seen in userland code, and is
more 'semantically' meaningful i.e. it allows people to tell the cases
between the two types of errors apart.

It probably bears more thinking about though.

cheers
Dan

$formatInfoList = [
["en_US", "{0,number,integer} monkeys on {1,number,integer} trees
make {2,number} monkeys per tree"],
["en_US", '{this was made intentionally incorrect}'], //valid type
but wrong value
["en_US", new StdClass] //wrong type
]

foreach ($formatInfoList as $formatInfo) {
list($locale, $pattern) = formatInfo;
try {
$mf = new MessageFormatter($locale, $pattern);
echo $mf->format(array(4560, 123, 4560/123));
}
catch(IntlException $ie) {
// A strict type-est person would not want this to catch the
case where
// the wrong type has been passed, as it's not an exception
related to Intl
// it's an exception related to the type being wrong.
}
}

PR for throwing TypeException (independently of strict mode) for zpp
failures in constructors is here: https://github.com/php/php-src/pull/1215
Will apply it if nobody sees a problem with it.

Nikita

9 years ago by Marc Bennewitz — view source

unread

Hi Nikita, Dmitry,

I think it's a mistake to throw a TypeException on invalid argument
count as in this case it has nothing to do with a type.

Marc

Am 03.04.2015 um 20:48 schrieb Dmitry Stogov:

I don' t see any problems.

Thanks. Dmitry.

On Wed, Apr 1, 2015 at 3:31 AM, Dan Ackroyd danack@basereality.com
wrote:

Hi Dmitry,

Your approach is definitely a better one, and I have no objection to
it whatsoever.

Sorry, I was too busy to look deeply at each class but I can't see any
problems.

Nikita Popov wrote:

does that mean that the same code using strict_types=1 mode will start
throwing TypeException instead of whatever exception type is passed
to replace_error_handling?
That's going to affect everything isn't it - not just this RFC?

However I think that is the correct behaviour. It more closely
resembles the behaviour that will be seen in userland code, and is
more 'semantically' meaningful i.e. it allows people to tell the cases
between the two types of errors apart.

It probably bears more thinking about though.

cheers
Dan

$formatInfoList = [
["en_US", "{0,number,integer} monkeys on {1,number,integer} trees
make {2,number} monkeys per tree"],
["en_US", '{this was made intentionally incorrect}'], //valid type
but wrong value
["en_US", new StdClass] //wrong type
]

foreach ($formatInfoList as $formatInfo) {
list($locale, $pattern) = formatInfo;
try {
$mf = new MessageFormatter($locale, $pattern);
echo $mf->format(array(4560, 123, 4560/123));
}
catch(IntlException $ie) {
// A strict type-est person would not want this to catch the
case where
// the wrong type has been passed, as it's not an exception
related to Intl
// it's an exception related to the type being wrong.
}
}

PR for throwing TypeException (independently of strict mode) for zpp
failures in constructors is here: https://github.com/php/php-src/pull/1215
Will apply it if nobody sees a problem with it.

Nikita

9 years ago by Pascal Martin — view source

unread

Le 15/03/2015 17:09, Dan Ackroyd a écrit :

The 'Constructor behaviour of internal classes' RFC is now in voting.
Please note, it's the coding standard that is being voted on. If
anyone thinks I've implemented the changes in a way that is less
awesome then there is no reason the changes couldn't be improved.

Hi,

We at AFUP are +1 on this.

Basically, more consistency is a good thing.

Thanks!

--
Pascal MARTIN, AFUP - French UG
http://php-internals.afup.org/