Hi Máté,
> Le 30 mai 2023 à 01:41, Máté Kocsis <kocsismat...@gmail.com> a écrit :
>
> Hi Claude,
>
>> The replacement methods for IntlCalendar::set() (namely
>> IntlCalendar::setDate() and IntlCalendar::setDateTime()) must not have a
>> return type of `void`, but `true`, like the original method, for the two
>> following reasons:
>>
>> 1. By changing the returned value, you are introducing an unnecessary hazard
>> when migrating code.
>>
>> 2. Per https://www.php.net/IntlCalendar, all modification methods of that
>> class (clear(), roll(), setTime(), etc.) consistently return `true` on
>> success and `false` on failure, even when the method is infallible (and thus
>> would always return `true`). Don’t break consistency.
>
> 1. I don't see much risk with this change, since we clearly indicate at the
> very beginning that the new methods return void, so programmers migrating
> their code
> should pay attention to the return types. IDEs and static analysers can
> surely warn them in case of inattention.
The very risk is that the programmer must be aware that the return convention
has changed. Recall that not everyone run static analysers, especially for such
a trivial migration, and that not every code is statically analysable (e.g..
`$foo->$bar()`).
Of course, the benefice of the change might be worth the risk; but that leads
to the second point:
>
> 2. Some of the methods you mentioned have a "true" return type for a few
> weeks now. The biggest motivation for introducing these was to at least have
> some chance to
> make them void in the future. Doing so is a risky move indeed, so should be
> carried out very carefully, if it's possible at all... That's why I consider
> it beneficial to spare the
> effort and start with a clean slate by adding the new methods in question
> with their correct return type.
Concretely, if you change the return value (from `true` to `void/null`), you
will break the following pattern:
<?php
if (!@$foo->setBar(...)) {
// error handling (might be dead code, it doesn’t matter)
}
?>
Any change that introduce a BC break or a migration burden must be justified.
(There has been enough rant around justified BC breaks, so let’s not introduce
unjustified ones.)
What is the purpose of changing the return convention of IntlCalendar methods,
and is it worth?
—Claude
