Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112785
MIME-Version: 1.0
References: <CAH5C8xUhozPLq=q6RE0D7pF-qB-2D+pCwyazGRmyzJ1KRVU2cw@mail.gmail.com>
 <CAF+90c-Q9EfS=kpTkRdkApD8Dk6_VqXXL5zbL19sCtKVntKY6w@mail.gmail.com>
In-Reply-To: <CAF+90c-Q9EfS=kpTkRdkApD8Dk6_VqXXL5zbL19sCtKVntKY6w@mail.gmail.com>
Date: Wed, 6 Jan 2021 17:54:53 +0100
Message-ID: <CAOWwgpn5My3XCwa5pQcf-96tgmd4XEmud3d+Ryhs8RZycVm-SQ@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: =?UTF-8?B?TcOhdMOpIEtvY3Npcw==?= <kocsismate90@gmail.com>, 
	PHP Internals List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000005555ff05b83e2fb3"
Subject: Re: [PHP-DEV] [RFC] Adding return types to internal methods (PHP 8)
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--0000000000005555ff05b83e2fb3
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Dec 28, 2020 at 7:54 PM M=C3=A1t=C3=A9 Kocsis <kocsismate90@gmail.c=
om> wrote:
>
> > Hi Internals,
> >
> > Last year, Nikita started a discussion about adding return types
> > to non-final internal methods: https://externals.io/message/106539 .
> >
> > I'd like to restart the conversation, since I've just created an
> > implementation
> > for the first step of the migration:
> > https://github.com/php/php-src/pull/6548
> > (I had to start a new email thread because I don't have the original
> one).
> >
> > My implementation currently emits a deprecation notice during inheritan=
ce
> > validation
> > for each method which omits the return type of its parent. This means
> > that the parent return type is not yet enforced when the child omits it=
,
> > neither during
> > inheritance, nor at run-time. However, as soon as the child class
> declares
> > return types,
> > everything will behave as usual, so variance and run-time return value
> > checks
> > both apply. Finally, in a couple of years, PHP 9.0 could make the
> > declaration of
> > return types required.
> >
> > If there are concerns about methods which are already declared with the
> > wrong return
> > type, we could lax the restrictions even further, and only emit a
> > deprecation notice
> > instead of a fatal error in this case as well.
> >
> > I chose the above approach over inheriting the return type implicitly
> > (which was suggested
> > by Sara) because I believe it has a few advantages:
> > - It causes less BC break: methods not declaring a return type will
> surely
> > continue to work
> > as before, and gradual migration it also supported
> > - It has negligible run-time impact because diagnostics is emitted
> maximum
> > once per
> > any method which misses the return type. If return types were inherited
> > implicitly,
> > incorrect return values would potentially trigger lots of errors.
> > - It is more straightforward behavior than implicit inheritance, no
> "magic"
> > is involved
> >
> > Even though I prefer the current implementation (or its more lax
> variant),
> > I'm also not
> > opposed to going with implicit inheritance if that's what we settle on.
> >
> > I appreciate any input, especially about the possible impact of the
> > different approaches.
> >
> > Regards:
> > M=C3=A1t=C3=A9
> >
>
> Is there any way we could make this mechanism more broadly available than
> just for internal methods? This is a problem that also shows up when addi=
ng
> return types in userland libraries.
>
> A concern I have is that for methods with union return types, there
> wouldn't be any way to avoid a deprecation warning without having PHP 8 a=
s
> the minimum version requirement. And given PHP's general error handling
> story, deprecation warnings are not exactly graceful...
>

We have the same issue in Symfony. We have a special class loader that
triggers a deprecation when a parent class has an `@return` annotation but
the child class doesn't have a return type. Also, we don't trigger a
deprecation when the child class also has the `@return` annotation. The
reason for this is to allow child classes to be deprecation-free.
By duplicating the annotation from the parent, child classes tell they know
a return type should be added in their next major, which should happen
before the next major of their parent class.

PHP could adopt the same strategy using an attribute. When a method has
that attribute, it'd mean the same: skip the deprecation regarding it's
parent classes, and trigger a deprecation when child classes either 1.
don't have an explicit or 2. don't have the same annotation.

WDYT?

--0000000000005555ff05b83e2fb3--