Skip to content

Promote Intl warnings to standard ValueError #5669

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Promote Intl warnings to standard ValueError #5669

wants to merge 8 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jun 5, 2020

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

@Girgias Girgias force-pushed the intl-error-promotion branch 2 times, most recently from c8ff4b1 to e1a1aeb Compare June 22, 2020 13:31
@Girgias Girgias force-pushed the intl-error-promotion branch from e1a1aeb to df9a403 Compare July 13, 2020 11:46
@Girgias Girgias force-pushed the intl-error-promotion branch from df9a403 to 8c05241 Compare July 20, 2020 16:53
@Girgias
Copy link
Member Author

Girgias commented Jul 20, 2020

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

nikic
nikic reviewed Jul 23, 2020
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

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

ext/intl/calendar/calendar_methods.cpp Outdated
@@ -355,6 +362,7 @@ static void _php_intlcal_before_after(
CALENDAR_METHOD_FETCH_OBJECT;

when_co = Z_INTL_CALENDAR_P(when_object);
/* Can this ever happen ? */
Copy link
Member

Choose a reason for hiding this comment

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

Should be possible, e.g. via newInstanceWithoutConstructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, does are usually Errors right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we use Error for uninitialized objects.

@Girgias Girgias force-pushed the intl-error-promotion branch from 8c05241 to 3d2d808 Compare July 24, 2020 07:49
@Girgias
Copy link
Member Author

Girgias commented Jul 24, 2020

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
Copy link
Member

nikic commented Jul 24, 2020

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
Copy link
Member Author

Girgias commented Jul 24, 2020

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
nikic approved these changes Jul 24, 2020
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@Girgias Girgias force-pushed the intl-error-promotion branch from 3d2d808 to 76ddff9 Compare July 24, 2020 18:27
nikic
nikic reviewed Jul 29, 2020
View reviewed changes
ext/intl/calendar/calendar_methods.cpp Outdated
intl_errors_set(&co->err, U_ILLEGAL_ARGUMENT_ERROR,
"intlcal_before/after: Other IntlCalendar was unconstructed", 0);
RETURN_FALSE;
zend_argument_error(NULL, 2, "object is not fully initialized");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this better?

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

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Girgias
Girgias commented Jul 29, 2020
View reviewed changes
ext/intl/calendar/calendar_methods.cpp Outdated
@@ -1085,6 +1079,7 @@ U_CFUNC PHP_FUNCTION(intlcal_to_date_time)
object_init_ex(return_value, php_date_get_date_ce());
zend_call_known_instance_method_with_2_params(
Z_OBJCE_P(return_value)->constructor, Z_OBJ_P(return_value), NULL, &ts_zval, timezone_zval);
// TODO Bubble up exception?
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks ok to change.

@Girgias Girgias force-pushed the intl-error-promotion branch from 76ddff9 to ed93fb8 Compare July 29, 2020 17:09
@php-pulls php-pulls closed this in f78a091 Jul 31, 2020
@Girgias Girgias deleted the intl-error-promotion branch July 31, 2020 12:30
@Girgias Girgias mentioned this pull request Aug 13, 2020
Closed
71 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
Warning promotion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Girgias @nikic @carusogabriel