Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120564
MIME-Version: 1.0
References: <CAH5C8xXtCVHaxwzmAW_eBxp_D6p7kww9svND=3AEeC9X6--YXg@mail.gmail.com>
 <eaaa805e-86b2-ce8c-5a24-c301260d16ac@php.net>
In-Reply-To: <eaaa805e-86b2-ce8c-5a24-c301260d16ac@php.net>
Date: Tue, 13 Jun 2023 17:59:47 +0200
Message-ID: <CAOWwgpn3PY=4DQ7s2PxjET7__fx09wYqQOJR76L2owa9PX9tFA@mail.gmail.com>
To: PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000618f2b05fe04ed51"
Subject: Re: [PHP-DEV] [RFC] [Discussion] Deprecate functions with overloaded signatures
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--000000000000618f2b05fe04ed51
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

>
> > As you have possibly already experienced, overloaded signatures cause
> > various smaller and bigger issues, while the concept is not natively
> > supported by PHP. That's why I drafted an RFC which intends to phase
> > out the majority of overloaded function/method signatures and also
> > forbid the introduction of such functions in the future:
> > https://wiki.php.net/rfc/deprecate_functions_with_overloaded_signatures
>
> I'm going to nitpick on the newly suggested names and argument order for
> the
> DatePeriod factory methods =E2=80=94 althoughI do agree that they need to=
 get
> created:
>
> createFromInterval(DateTimeInterface $start, DateInterval $interval,
> DateTimeInterface $end, int $options =3D 0)
> =E2=86=92 createWithRange(DateTimeInterface $begin, DateTimeInterface $en=
d,
> DateTimeInterface $int, int $options =3D 0)
>
> createFromRecurrences(DateTimeInterface $start, DateInterval $interval,
> int $recurrences, int $options =3D 0)
> =E2=86=92 createWithRecurrences(DateInterval $begin, int $recurrences,
> DateInterval $interval, int $options =3D 0)
>
> We also should fix the argument names. Either $start/$finish, or
> $begin/$end. I
> prefer the latter.
>
> createFromIso8601(string $specification, int $options =3D 0)
> -> createFromISO8601String
>
> I am open to bike shedding about this :-)
>

On my side, I'd very much prefer keeping the constructor of DatePeriod and
thus making it non-overloaded with this signature:

public function __construct(DateTimeInterface $start, DateInterval
$interval, DateTimeInterface|int $end, int $options =3D 0) {}

That'd help a lot with the migration/BC path and this signature also just
makes sense.

Unrelated: I don't think adding "ReflectionMethod::createFromMethodName" is
useful. Using explode is just fine for those that need this style. And
since this is the recommended migration path, better not give a reason to
change twice the code.

I like everything else in the RFC. I'm going to go with the "short path" of
deprecating things because the migration paths look smooth enough, thanks
for taking the time to explain them all (I just have a concern with the
DatePeriod migration path, which would be fixed with my suggestion bove.)

Thanks!
Nicolas

--000000000000618f2b05fe04ed51--