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

Don't overload ArrayObject get_properties #3574

Closed
wants to merge 2 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Oct 4, 2018

Change to fix #3573 (comment).

The first commit lays groundwork to allow proper overloading of (array) casts. The second commit explicitly overloads (array) for ArrayObject and removes the get_properties overload.

The BC breaks are noted in UPGRADING. I think that array_key_exists() is the only problematic one. However, as we already plan to deprecate array_key_exists with objects. As this will make the operation generate a warning (rather than silently change behavior) I'm not so concerned.

Previously this was only possible by changing get_properties, which
also impacts other behaviors.

This change required an adjustment in compare_function, which
compares objects and non-objects by casting the object to the type
of the non-object. However, as an exception, arrays are treated
as null.
Instead explicitly overload the (array) cast.
@nikic
Copy link
Member Author

nikic commented Oct 4, 2018

Overall I'm not really happy with this change, as it has potential impact on too many places.

I think a better solution might be to introduce a get_properties_for handler, which accepts a purpose flag. We also need this to solve long standing bugs in DateTime and other classes that want to overload get_properties only for some specific functionality, but end up overloading it for everything.

@dstogov
Copy link
Member

dstogov commented Oct 4, 2018

This is definitely not a simple bug fix :(
I'll try to think about a better solution.

@dstogov
Copy link
Member

dstogov commented Oct 4, 2018

@nikic SPL is over-designed, and provides more functionality that actually can do :(
Ideally, I would drop things that are not implemented properly and can't be fixed.

If you decide to proceed with get_properties_for(), it may make sense to just change prototype of get_properties(). Object handlers are going to be broken by typed properties, anyway.

@nikic
Copy link
Member Author

nikic commented Oct 10, 2018

Closing in favor of #3579, which has landed.

@nikic nikic closed this Oct 10, 2018
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.

None yet

2 participants