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 inheritance 
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áté
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 inheritance
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 involvedEven 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áté
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 adding 
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 as 
the minimum version requirement. And given PHP's general error handling 
story, deprecation warnings are not exactly graceful...
Regards, 
Nikita
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 inheritance
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 involvedEven 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áté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 adding
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 as
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?
Hi Nikita and Nicolas,
After a month of pause, I came back to this topic.
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 as
the minimum version requirement. And given PHP's general error handling
story, deprecation warnings are not exactly graceful...
Thanks for pointing out this possible consequence. I do agree that it's a 
valid concern, so we should try to do something about it. I could come up 
with a few possible mitigations so far:
- instead of E_DEPRECATE, we could emit E_STRICTwarnings so that people
 can separately tackle this class of errors from the rest
- we could add support for a SuppressReturnTypeDeclaration annotation that 
 could suppress these notices altogether
- we could postpone the tentative declaration of those method return types 
 which can't be expressed on PHP 7.4 to a later version (e.g. PHP 8.4) when
 the 7.x series will have become EOL for a while
Actually, I counted how many of the return types in question can't be 
expressed on PHP 7.4. I found ~270 results out of all the ~1440 methods 
which don't yet have a return type declared.
Do you think these ideas would sufficiently answer your concerns?
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 adding
return types in userland libraries.
Inspired by Symfony's strategy, a #[ReturnTypeDeclaration("int|string", 
"Library Foo v2.0 will declare int|string as return type")] attribute could 
be added to methods in order to gradually 
let any child classes know that a future version of library Foo will 
declare the int|string return type for the method in question. Then 
overriding methods should either declare some kind of 
return type (unfortunately, even a wrong one would suffice, since it would 
be cumbersome to store exact type information in an attribute, if I'm not 
mistaken) or add the very same 
attribute - but at least the first argument should be exactly the same, 
from character to character - if people want to avoid the deprecation 
notice.
It has just come to my mind that in case of internal functions, we could 
combine the annotation based approach described above together with the 
behavior I originally proposed:
- if an overriding method declares a return type then the regular 
 compatibility checks would kick in, and a notice would be emitted in case
 of failure
- if an overriding method doesn't have a return type, but 
 a ReturnTypeDeclaration attribute, then a notice would be emitted if the
 attribute doesn't have the exact same type as the
 one we added via PHPDoc in the stubs
- otherwise a notice is emitted
To be honest, this migration path seems to be slightly overcomplicated for 
me. So personally, I would be just fine with the ideas I collected in the 
beginning of my message for reducing impact. 
However, if you think that it makes some sense, I'm ok with going with 
something along the lines of it.
Regards: 
Máté