Skip to content

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 14, 2024

This is a follow-up to PR #3858, based on a comment by @arnaud-lb[1].

[1] php/php-src#16371 (comment)

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Thank you @cmb69! The change looks good to me. I think we could change the class description as well, to reflect the intended use-case more.

Copy link
Member

@jimwins jimwins left a comment

Choose a reason for hiding this comment

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

LGTM with one word change.

@cmb69
Copy link
Member Author

cmb69 commented Oct 16, 2024

@arnaud-lb, it seems that more should be changed than the class description. E.g. https://www.php.net/manual/en/arrayobject.count.php claims:

Get the number of public properties in the ArrayObject

The whole documentation of this class should be revised. I'll try to come up with a more complete PR soonish, but I'm going to merge this now so users are not discouraged to wrap arrays (an interesting use-case I've never considered before).

@cmb69 cmb69 merged commit eff2271 into php:master Oct 16, 2024
2 checks passed
@cmb69 cmb69 deleted the cmb/arrayobject-objects branch October 16, 2024 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants