Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yield without value in reference generator should create notice #16882

Open
wants to merge 4 commits into
base: PHP-8.2
Choose a base branch
from

Conversation

Bellangelo
Copy link

Fixes #16761

@dstogov
Copy link
Member

dstogov commented Nov 25, 2024

I'm not sure if this is necessary.
@cmb69 @nielsdos please make a decision

@cmb69
Copy link
Member

cmb69 commented Nov 25, 2024

php-src/Zend/zend_vm_def.h

Lines 8138 to 8139 in 173175b

/* Constants and temporary variables aren't yieldable by reference,
* but we still allow them with a notice. */

I think the question is whether this behavior is supposed to be deprecated/removed at some point in the future. If so, having the notice in the else branch appears to be prudent. If not, we may drop the other notices instead; that would make the behavior consistent.

@Bellangelo
Copy link
Author

If not, we may drop the other notices instead; that would make the behavior consistent.

@cmb69 If we agree on this, it might be a good idea to do this in a new version. I don't think that we should change the behavior of the pre-existing versions.

@nielsdos
Copy link
Member

I think the question is whether this behavior is supposed to be deprecated/removed at some point in the future. If so, having the notice in the else branch appears to be prudent. If not, we may drop the other notices instead; that would make the behavior consistent.

I would agree. I also agree with @Bellangelo but would make it stronger in the sense that perhaps, since this is a behaviour change, whatever we choose has to go in master anyway. Perhaps we should consult the mailing list to ask for opinion on whether the notice should be added or not.

@cmb69
Copy link
Member

cmb69 commented Nov 26, 2024

Perhaps we should consult the mailing list to ask for opinion on whether the notice should be added or not.

@Bellangelo, would you want to start this discussion?

@Bellangelo
Copy link
Author

@Bellangelo, would you want to start this discussion?

Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants