Skip to content

Fix GH-9769: Improve error message for array unpacking in constant expression #9776

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

Closed
wants to merge 4 commits into from

Conversation

jhdxr
Copy link
Member

@jhdxr jhdxr commented Oct 18, 2022

I didn't attach a test script for this as it's only changing the error message. But I can add one if it's required.
(fix #9769 )

@iluuu1994
Copy link
Member

I didn't attach a test script for this as it's only changing the error message. But I can add one if it's required.

Yes please 🙂 They're not required per-se but are certainly welcome.

@jhdxr
Copy link
Member Author

jhdxr commented Oct 18, 2022

I didn't attach a test script for this as it's only changing the error message. But I can add one if it's required.

Yes please 🙂 They're not required per-se but are certainly welcome.

If this is the case I prefer to explore to bring Traversables to const expr, but this definately needs to change the target branch to 8.2 or master. Just adding a test case for error message seems to be kinds of overkilling IMHO.

@iluuu1994
Copy link
Member

@jhdxr Since this would involve calling user code from constant expressions at least a mail to the mailing list would be warranted. This also likely has suffers from the same caching issue as this RFC did.

@jhdxr
Copy link
Member Author

jhdxr commented Oct 19, 2022

As said in #9769 I'll jus leave this PR open for RMs to decide.
Test case will NOT be added since error messages are subjected to change.

Exploration on Traversables in constant expression is another topic and I'll send an email to interals if I have progress on that.

@iluuu1994
Copy link
Member

Test case will NOT be added since error messages are subjected to change.

I don't think user code should depend on error messages. It's fine for us to depend on them though, as there's no other way to test them. For bulk-changes we have the --bless parameter.

@cmb69
Copy link
Member

cmb69 commented Oct 25, 2022

@ramsey is fine with changing the error message, so this can go into PHP-8.1.

However, I think we should have a test for this; not because of the error message, but rather because this code path is apparently not tested so far (otherwise the changed message should have triggered a test failure). @jhdxr, could you please add a simple test (basically, just const EXAMPLE = [...new ArrayObject()]; for instance).

@jhdxr
Copy link
Member Author

jhdxr commented Nov 16, 2022

@cmb69 @ramsey test added. sorry for the long waiting.

@jhdxr
Copy link
Member Author

jhdxr commented Nov 17, 2022

the AppVeyor failure seems to be unrelated, but I can rerun it if necessary.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Besides the minor issue pointed out below, this looks good. And yeah, that AppVeyor failure is unrelated; I've retriggred the build nonetheless.

@cmb69 cmb69 closed this in 93592ea Dec 2, 2022
@cmb69
Copy link
Member

cmb69 commented Dec 2, 2022

Thank you!

@jhdxr jhdxr deleted the patch-3 branch December 26, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants