Hello everyone,
In this issue #16761 https://github.com/php/php-src/issues/16761 it was
verified that in the above code it should throw a notice for both yields:
<?phperror_reporting(E_ALL);function &y()
{
yield null; // warning
yield; // no warning, agreed to be a bug
}foreach (y() as &$y);
I have created this PR https://github.com/php/php-src/pull/16882
which adds the "missing" notice but we aren't sure on
how to continue on this.
There are 2 main questions:
- Should we continue with adding the missing notice and merge it
also in master? - Should we deprecate / remove the usage of yield without value in
reference generator functions?
- Aggelos Bellos
Le 26 nov. 2024 à 01:49, aggelos bellos aggelosbellos7@gmail.com a écrit :
Hello everyone,
In this issue #16761 https://github.com/php/php-src/issues/16761 it was verified that in the above code it should throw a notice for both yields:
<?php error_reporting(E_ALL); function &y() { yield null; // warning yield; // no warning, agreed to be a bug } foreach (y() as &$y);I have created this PR https://github.com/php/php-src/pull/16882 which adds the "missing" notice but we aren't sure on
how to continue on this.
There are 2 main questions:
Should we continue with adding the missing notice and merge it also in master?
Should we deprecate / remove the usage of yield without value in reference generator functions?
- Aggelos Bellos
Hi,
A notice would be consistent with what happens with: function &f() { return; }; f();. But don’t add new notices in patch releases, otherwise you may break without warning a non-trivial amount of code that conflates notices with fatal errors. The bug is not serious enough to warrant such a risk.
Whether it should be deprecated, is a separate question. You should also consider what to do with: function &g() { yield null; }, function &g() { if (false) yield; }, function &f() { return; }, etc.
—Claude
Le 26 nov. 2024 à 01:49, aggelos bellos aggelosbellos7@gmail.com a écrit :
In this issue #16761 https://github.com/php/php-src/issues/16761 it was verified that in the above code it should throw a notice for both yields:
<?php error_reporting(E_ALL); function &y() { yield null; // warning yield; // no warning, agreed to be a bug } foreach (y() as &$y);I have created this PR https://github.com/php/php-src/pull/16882 which adds the "missing" notice but we aren't sure on
how to continue on this.
There are 2 main questions:
Should we continue with adding the missing notice and merge it also in master?
Should we deprecate / remove the usage of yield without value in reference generator functions?A notice would be consistent with what happens with:
function &f() { return; }; f();. But don’t add new notices in patch releases, otherwise you may break without warning a non-trivial amount of code that conflates notices with fatal errors. The bug is not serious enough to warrant such a risk.Whether it should be deprecated, is a separate question. You should also consider what to do with:
function &g() { yield null; },function &g() { if (false) yield; },function &f() { return; }, etc.
In my opinion, whether it should be deprecated is not a separate
question, but the one we should answer first. If we're not going to
deprecate/remove the auto-conversion to a reference, we may as well drop
the notice altogether, instead of adding a further notice.
Christoph
A notice would be consistent with what happens with:
function &f() { return; }; f();. But don’t add new notices in patch releases, otherwise
you may break without warning a non-trivial amount of code that conflates
notices with fatal errors. The bug is not serious enough to warrant such a
risk.
But isn't a bug that the notice is missing in the first place?
In my opinion, whether it should be deprecated is not a separate
question, but the one we should answer first. If we're not going to
deprecate/remove the auto-conversion to a reference, we may as well drop
the notice altogether, instead of adding a further notice.
Hmm I am not exactly sure if I agree with you. Whatever we decide, it will
happen in a new version. It feels right to fix the current supported
versions.
Regarding the deprecation decision, I checked the RFC
https://wiki.php.net/rfc/generators and I found this line:
*Only generators specifying the & modifier can be iterated by ref. If you
try to iterate a non-ref generator by-ref an
E_ERRORis thrown.*
Correct me if I am wrong, but this phrase refers to our case. If I
understand this correctly it should normally throw an E_ERROR so we should
continue perhaps with the deprecation? Other than this, I don't have a
strong opinion on if we should deprecate or not this case.