Skip to content

ext/spl: Add ArrayObject test with property hooks #15005

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

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jul 18, 2024

As expected, ArrayObject is cursed.

@Crell maybe you could provide more useful test cases? But in any case ArrayObject just, does not care about property hooks.

As expected, ArrayObject is cursed
@Girgias Girgias requested a review from iluuu1994 July 18, 2024 01:36
@iluuu1994
Copy link
Member

That's pretty much what I expected. Essentially, ArrayAccess writes directly to the properties table. If you cast it to an array, or use get_mangled_object_vars(), you'd probably see the dynamic properties created for virtual properties.

@Girgias
Copy link
Member Author

Girgias commented Jul 18, 2024

In that case, except if @Crell wants to write more edge cases, I'll merge this when I get back to working on the container RFC to be able to check how ArrayObject behaves after my changes.

@Girgias Girgias merged commit efe4e6d into php:master Jul 18, 2024
11 checks passed
@Girgias Girgias deleted the array-object-property-hooks branch July 18, 2024 12:10
@Crell
Copy link
Contributor

Crell commented Jul 18, 2024

If you're going to tag me, you'll need to give me more than 12 hours while I'm asleep to respond. 😛

In any case, I think the only time I've used ArrayObject was in one slide in my talk on why to not use arrays. So, I don't really care what happens here and I'm happy to defer to whatever @iluuu1994 feels is best. Especially if your goal is to destroy ArrayObject eventually anyway.

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.

3 participants