Skip to content

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

Merged
Girgias merged 4 commits into
php:masterfrom
marcosmarcolin:change-msg-illegal-offset-type
Feb 8, 2023
Merged

Inform type in messsage: 'Illegal offset type'#10504
Girgias merged 4 commits into
php:masterfrom
marcosmarcolin:change-msg-illegal-offset-type

Conversation

@marcosmarcolin

Copy link
Copy Markdown
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.

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

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.

Comment thread ext/opcache/jit/zend_jit_helpers.c Outdated
Comment thread Zend/zend_execute.c Outdated
Comment thread ext/spl/spl_array.c Outdated
Comment thread Zend/tests/036.phpt

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

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

Girgias commented Feb 5, 2023

Copy link
Copy Markdown
Member

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
@ndossche

ndossche commented Feb 6, 2023

Copy link
Copy Markdown
Member

Lgtm, thanks!

@marcosmarcolin

marcosmarcolin commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

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

Appears to be cached.

@marcosmarcolin

Copy link
Copy Markdown
Contributor Author

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

@Girgias

Girgias commented Feb 8, 2023

Copy link
Copy Markdown
Member

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

Girgias commented Feb 8, 2023

Copy link
Copy Markdown
Member

Thank you!

@marcosmarcolin

Copy link
Copy Markdown
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.

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.

Illegal offset type exception should mention the type

5 participants