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

Migrate SOAP table resource to array #14174

Merged
merged 1 commit into from
May 8, 2024

Conversation

kocsismate
Copy link
Member

I was not sure if we really have to convert the SOAP table resource as an object, I rather chose an array. It could also be readonly, but that would mean the constructor cannot be called twice, so I opted for not adding this modifier for now.

Related to https://wiki.php.net/rfc/resource_to_object_conversion and php/php-tasks#6

@Girgias
Copy link
Member

Girgias commented May 8, 2024

An array does seem to make sense here, rather than an object.

The readonly modifier would be nice but I suppose this can always be discussed on the internals ML.

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.

Makes a lot of sense to me. Thanks! I like that the number of lines even decreases.
I wonder why it wasn't simply an array in the first place. I guess it wasn't because maybe this predates private properties?

@kocsismate
Copy link
Member Author

I wonder why it wasn't simply an array in the first place. I guess it wasn't because maybe this predates private properties?

Yeah, I was also wondering about it.. My guess was that it had been implemented like that to achieve immutability (that's why I was thinking about adding the readonly modifier) 🤔 But I don't know..

Thank you all for the quick reviews! :) One less extension uses resources!

@kocsismate kocsismate merged commit 9205910 into php:master May 8, 2024
10 checks passed
@kocsismate kocsismate deleted the soap-resource2 branch May 8, 2024 20:48
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.

None yet

3 participants