Skip to content

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Jun 17, 2021

Found partly when looking at #6106 again, but out handling of offsets is really a mess as the handling of Object (namely SPL), strings, and arrays all depend of their context which are all probably deserving of their own bug reports.

@Girgias Girgias requested a review from nikic June 17, 2021 22:07
@krakjoe krakjoe added the Bug label Jun 18, 2021
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

The first commit looks good.

For the second commit, if we want to make this change, I should it should happen in a more principled way, by reusing the zend_check_string_offset() logic. Now you're handling one of the error cases, but for example not the warning that occurs for float null/true/false/float offsets.

@Girgias
Copy link
Member Author

Girgias commented Jun 18, 2021

The first commit looks good.

The GC assertion on MacOS seems to be related to that one tho

For the second commit, if we want to make this change, I should it should happen in a more principled way, by reusing the zend_check_string_offset() logic. Now you're handling one of the error cases, but for example not the warning that occurs for float null/true/false/float offsets.

Right, that's actually a good idea, should also fix the leading numeric string issue.

@nikic
Copy link
Member

nikic commented Jun 18, 2021

The first might be a candidate for PHP-8.0, but I don't think the second one should go on a release branch.

@Girgias Girgias mentioned this pull request Jun 18, 2021
@Girgias Girgias changed the title Fix bugs in regards to offsets (81159 & 81160) Fix bug 81159 Jun 18, 2021
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LG if CI happy.

@Girgias Girgias closed this in f0fd592 Jun 18, 2021
@Girgias Girgias deleted the offset-bugs branch June 18, 2021 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants