Promote Intl warnings to standard ValueError by Girgias · Pull Request #5669 · php/php-src

Girgias · 2020-06-05T20:34:56Z

As Intl is rather large I think I will split this up into multiple PRs

Girgias · 2020-07-20T23:41:28Z

If there are no objections I'll merge this at the end of the week

nikic

This looks pretty good, but I think is missing some stub updates for IntlCalendar. E.g. fieldDifference can no longer return false, right?

nikic · 2020-07-23T12:51:37Z

Should be possible, e.g. via newInstanceWithoutConstructor.

Ah yeah, does are usually Errors right?

Yeah, we use Error for uninitialized objects.

Girgias · 2020-07-24T07:49:59Z

I promoted a couple more to ValueError, however in regards to the stubs I'm very confused as to why there are two different ones for the calendar proceduaral functions? I thought those were taken care by the aliasing of the Class methods?

nikic · 2020-07-24T07:55:21Z

Can you please avoid rebasing and amending commits at the same time? This means the whole PR has to be reviewed from scratch every time, because it's not possible to see what changed.

I promoted a couple more to ValueError, however in regards to the stubs I'm very confused as to why there are two different ones for the calendar proceduaral functions? I thought those were taken care by the aliasing of the Class methods?

The @alias only means that the implementation is aliased (FALIAS), the signature is specified separately.

Girgias · 2020-07-24T08:02:32Z

Can you please avoid rebasing and amending commits at the same time? This means the whole PR has to be reviewed from scratch every time, because it's not possible to see what changed.

Okay will do that next time

I promoted a couple more to ValueError, however in regards to the stubs I'm very confused as to why there are two different ones for the calendar proceduaral functions? I thought those were taken care by the aliasing of the Class methods?
The @alias only means that the implementation is aliased (FALIAS), the signature is specified separately.

Ah, understood

nikic

LGTM

nikic · 2020-07-29T13:34:15Z

I'm not sure the error message this generates will make sense.

Is this better?
Suggested change
zend_argument_error(NULL, 2, "object is not fully initialized");
zend_argument_error(NULL, 2, "must be constructed");

Or maybe?
Suggested change
zend_argument_error(NULL, 2, "object is not fully initialized");
zend_argument_error(NULL, 2, "is uninitialized");

That's better :D
Will go with that and merge it directly then. :)

Girgias · 2020-07-29T16:14:58Z

Actually before I merge it, @nikic should I do that in this PR or just leave the TODO comment?

Looks ok to change.

…lueError

Seems no test are covering these cases

This was fixed in ICU 50.1 which is required as of PHP 7.4

Girgias force-pushed the intl-error-promotion branch 2 times, most recently from c8ff4b1 to e1a1aebCompare June 22, 2020 13:31

carusogabriel added the Warning promotion label Jun 22, 2020

Girgias force-pushed the intl-error-promotion branch from e1a1aeb to df9a403Compare July 13, 2020 11:46

Girgias force-pushed the intl-error-promotion branch from df9a403 to 8c05241Compare July 20, 2020 16:53

nikic reviewed Jul 23, 2020
View reviewed changes

Girgias force-pushed the intl-error-promotion branch from 8c05241 to 3d2d808Compare July 24, 2020 07:49

nikic approved these changes Jul 24, 2020
View reviewed changes

Comment threadext/intl/calendar/gregoriancalendar_methods.cpp Outdated
Comment threadext/intl/calendar/gregoriancalendar_methods.cpp Outdated

Girgias force-pushed the intl-error-promotion branch from 3d2d808 to 76ddff9Compare July 24, 2020 18:27

nikic reviewed Jul 29, 2020
View reviewed changes

Girgias commented Jul 29, 2020
View reviewed changes

Girgias added 8 commits July 29, 2020 17:15

Promote some IntlBreakIterator internal error handling to standard Va…
660e9fe
…lueError

Convert some internal IntlCalendar warnigns to Errors
65f2db6

Promote IntlGregorianCalendar warnings to Error
7d2e249
Seems no test are covering these cases

Add some todo comments
4f66834

Drop workaround due to ICU bug 9194
da13108
This was fixed in ICU 50.1 which is required as of PHP 7.4

Nits
dd3bbb4

Fix error message
cacb143

Drop bubble up todo with warning, albeit can't seem to hit this
ed93fb8

Girgias force-pushed the intl-error-promotion branch from 76ddff9 to ed93fb8Compare July 29, 2020 17:09

php-pulls closed this in f78a091Jul 31, 2020

Girgias deleted the intl-error-promotion branch July 31, 2020 12:30

Girgias mentioned this pull request Aug 13, 2020
Warning to Error (TypeError/ValueError) in extensions php/php-tasks#18
Closed
71 tasks

	zend_argument_error(NULL, 2, "object is not fully initialized");
	zend_argument_error(NULL, 2, "must be constructed");

	zend_argument_error(NULL, 2, "object is not fully initialized");
	zend_argument_error(NULL, 2, "is uninitialized");

Uh oh!

Conversation

Girgias commented Jun 5, 2020

Uh oh!

Girgias commented Jul 20, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Girgias commented Jul 24, 2020

Uh oh!

nikic commented Jul 24, 2020

Uh oh!

Girgias commented Jul 24, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants