Skip to content

Fix GH-16990 "dba_list() is now zero-indexed instead of using resourc… #17005

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
Closed
wants to merge 3 commits into from
Closed

Fix GH-16990 "dba_list() is now zero-indexed instead of using resourc… #17005

wants to merge 3 commits into from

Conversation

kocsismate
Copy link
Member

…e ids"

@kocsismate kocsismate force-pushed the dba_list-bug branch 3 times, most recently from b795a26 to 3dbcced Compare November 30, 2024 22:24
@kocsismate kocsismate marked this pull request as ready for review November 30, 2024 22:24
@kocsismate kocsismate requested a review from Girgias as a code owner November 30, 2024 22:24
@kocsismate kocsismate mentioned this pull request Nov 30, 2024
Closed
Girgias
Girgias approved these changes Dec 1, 2024
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.

MSTM, @alexandre-daubois can you confirm this fixes the issue?

alexandre-daubois
alexandre-daubois reviewed Dec 2, 2024
View reviewed changes
Copy link
Contributor

@alexandre-daubois alexandre-daubois 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.

If I understand correctly that std.handle points to the resource id, it does! Thank you Máté!

@kocsismate kocsismate requested a review from nielsdos December 2, 2024 08:50
@kocsismate
Copy link
Member Author

If I understand correctly that std.handle points to the resource id, it does!

Yes, basically that's the case (with the caveat that it's the object ID in fact, since the resource to object conversion)

@cmb69
Copy link
Member

cmb69 commented Dec 2, 2024

Small caveat: as far as I know, object IDs reusable (contrary to resource IDs). Might not matter in practice, though.

@Girgias
Copy link
Member

Girgias commented Dec 2, 2024

Small caveat: as far as I know, object IDs reusable (contrary to resource IDs). Might not matter in practice, though.

Yes they are indeed, see: https://3v4l.org/brMEt

cmb69
cmb69 reviewed Dec 2, 2024
View reviewed changes
ext/dba/tests/gh16990.phpt Outdated
Comment on lines 9 to 10
$handler = "flatfile";
require_once(__DIR__ .'/skipif.inc');
Copy link
Member

Choose a reason for hiding this comment

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

See #17011 (comment) f.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this out!

nielsdos
nielsdos approved these changes Dec 2, 2024
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Ok for me except the test thing that cmb pointed out

nielsdos
nielsdos reviewed Dec 2, 2024
View reviewed changes
ext/dba/tests/gh16990.phpt Outdated
$handler = "flatfile";
require_once(__DIR__ .'/skipif.inc');
require_once __DIR__ . '/setup/setup_dba_tests.inc';
check_skip('inifile');
Copy link
Member

Choose a reason for hiding this comment

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

Euh, flatfile or inifile? I suppose this needs to say flatfile

Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to blindly copy-paste the SKIPIF snippet.

Copy link
Member

Choose a reason for hiding this comment

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

How dare you! ;p

kocsismate
kocsismate commented Dec 2, 2024
View reviewed changes
@kocsismate kocsismate closed this in 50264b0 Dec 2, 2024
@kocsismate kocsismate deleted the dba_list-bug branch December 2, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexandre-daubois alexandre-daubois alexandre-daubois left review comments

@cmb69 cmb69 cmb69 left review comments

@nielsdos nielsdos nielsdos approved these changes

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
Extension: dba
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kocsismate @cmb69 @Girgias @alexandre-daubois @nielsdos