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

Fix GH-17317: ResourceBundle crash on undefined iterator key #17323

Open
wants to merge 1 commit into
base: PHP-8.3
Choose a base branch
from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jan 2, 2025

The next() and rewind() calls should be the ones fetching the data. We skip holes in the resource bundle in the same way that holes in arrays are skipped.

The next() and rewind() calls should be the ones fetching the data.
We skip holes in the resource bundle in the same way that holes in
arrays are skipped.
@nielsdos nielsdos marked this pull request as ready for review January 2, 2025 15:03
@nielsdos nielsdos requested a review from devnexen as a code owner January 2, 2025 15:03
@nielsdos nielsdos requested a review from cmb69 January 2, 2025 15:03
@nielsdos nielsdos linked an issue Jan 2, 2025 that may be closed by this pull request
@cmb69
Copy link
Member

cmb69 commented Jan 2, 2025

Thank you! I haven't checked very thoroughly, but this looks like a sensible solution. There is, however, an issue regarding the ::count() method:

var_dump((new ResourceBundle('', NULL))->get('calendar')->get('buddhist')->count());

reports 12, but traversal only shows 10 items. We can't change ::count() for BC reasons (would break "manual" for iterations using ::get()). If we want to avoid this dichotomy (at least for stable branches), we could return null for key and value for missing elements (I was missing that resourcebundle_iterator_current() can simply return NULL), but that still would leave a difference regarding ::getError*() (see middle part of #17318 (comment)).

Probably not a big deal, so I'm fine merging this PR as is into PHP-8.3.

@nielsdos
Copy link
Member Author

nielsdos commented Jan 2, 2025

There is, however, an issue regarding the ::count() method:

Indeed, this one is unfortunate but we should preserve BC.

we could return null for key and value for missing elements

Sounds like a typical PHP solution, and inconsistent with how arrays work imo.

I was missing that resourcebundle_iterator_current() can simply return NULL

You can only do this if the iterator is invalid, so it's not a valid alternative solution (if you're talking about returning a NULL pointer).

but that still would leave a difference regarding ::getError*() (see middle part of #17318 (comment)).

This is consistent with how arrays work:

<?php
$array = [0 => "a", 2 => "b"];
$array[1]; // warning & returns NULL
foreach ($array as $k=>$v) { var_dump($k, $v); } // skips 1

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.

Iterating on ResourceBundle (using keys) causes segmentation fault
2 participants