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.

Sign up for GitHub

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

Jump to bottom

Inform type in messsage: 'Illegal offset type' #10504

Merged
merged 4 commits into from
Feb 8, 2023
Merged

Inform type in messsage: 'Illegal offset type' #10504

merged 4 commits into from
Feb 8, 2023

Conversation

marcosmarcolin
Copy link
Contributor

#10432

@Girgias @nielsdos @devnexen

I apologize for my ignorance on #10450, it was not something I used in my day to day (git rebase).

I appreciate all of your help.

nielsdos
nielsdos reviewed Feb 4, 2023
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I had a quick scroll through the changed C code and it looks generally good, thanks.
Just some minor comments.

ext/opcache/jit/zend_jit_helpers.c Outdated Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
ext/spl/spl_array.c Outdated Show resolved Hide resolved
Girgias
Girgias approved these changes Feb 5, 2023
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Other than the typo looks good.

I'll do a follow up PR to extract the offset functions into a common zend_API one as it could also fold the string offset access error into it.

@Girgias
Copy link
Member

Girgias commented Feb 5, 2023

After addressing the comments please do not forget to regenerate the vm by running Zend/zend_vm_gen.php

@Girgias Girgias linked an issue Feb 5, 2023 that may be closed by this pull request
Illegal offset type exception should mention the type #10432
Closed
@nielsdos
Copy link
Member

nielsdos commented Feb 6, 2023

Lgtm, thanks!

@marcosmarcolin
Copy link
Contributor Author

marcosmarcolin commented Feb 7, 2023 •
edited
Loading

After addressing the comments please do not forget to regenerate the vm by running Zend/zend_vm_gen.php

Done, I think that's it. I await you.

Thanks.

@marcosmarcolin
Copy link
Contributor Author

About the failure in the CI, I noticed the same behavior in #10483.

Appears to be cached.

@marcosmarcolin
Copy link
Contributor Author

Well, now it's over, it was the same cache. The AppVeyor failure I believe is related to this change.

@Girgias
Copy link
Member

Girgias commented Feb 8, 2023

Well, now it's over, it was the same cache. The AppVeyor failure I believe is related to this change.

The failed file is file_get_contents_variation7-win32.phpt and it doesn't access any offsets.

@Girgias Girgias merged commit 641fe23 into php:master Feb 8, 2023
@Girgias
Copy link
Member

Girgias commented Feb 8, 2023

Thank you!

@marcosmarcolin
Copy link
Contributor Author

The failed file is file_get_contents_variation7-win32.phpt and it doesn't access any offsets.

Okay, that was a typo, it was supposed to say it had nothing to do with this PR changes.

Thanks for all your help, regards.

@Girgias Girgias mentioned this pull request Feb 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos left review comments

@devnexen devnexen devnexen left review comments

@mvorisek mvorisek mvorisek left review comments

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
Category: Engine Extension: opcache Extension: spl Extension: standard
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Illegal offset type exception should mention the type
5 participants
@marcosmarcolin @Girgias @nielsdos @mvorisek @devnexen