Skip to content

[RFC] User-defined object comparison #3339

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

Closed
wants to merge 40 commits into from

Conversation

rtheunissen
Copy link
Contributor

@rtheunissen rtheunissen changed the title WIP: Draft implementation of the user-defined object comparison RFC [RFC] Draft implementation of user-defined object comparison Jun 26, 2018
@Majkl578
Copy link
Contributor

I was playing with this recently (based on Java): https://gist.github.com/Majkl578/e71828d2a146a5cad34ccb893171f6ab

Copy link
Contributor

@carusogabriel carusogabriel left a comment

Choose a reason for hiding this comment

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

Great RFC :)

Nit: some tests don't need EXPECTF, we can use EXPECT

@rtheunissen rtheunissen force-pushed the rt-compare-to-magic-method branch from 4cdb098 to 5da5218 Compare June 27, 2018 23:16
@rtheunissen
Copy link
Contributor Author

@carusogabriel Thanks for the suggestion! Fixed them all in 7f40bf2.

@rtheunissen
Copy link
Contributor Author

Can someone help me debug what's happening with inheritance here?

When ENABLE_DEBUG is 0 on a non-debug build, running the same script as Travis results in some very strange behaviour around inheritance:

./sapi/cli/php run-tests.php -p `pwd`/sapi/cli/php $(if [ $ENABLE_DEBUG == 0 ]; then echo "-d opcache.enable_cli=1 -d zend_extension=`pwd`/modules/opcache.so"; fi) -g "FAIL,XFAIL,BORK,WARN,LEAK,SKIP,PASS" --offline --show-diff --show-slow 1000 --set-timeout 120 Zend/tests/comparisons/*.phpt
Fatal error: Uncaught Error: Cannot access parent:: when current class scope has no parent in .../Zend/tests/comparisons/compare-inheritance.php:18

There are multiple failures all over the place but this is a good one to inspect.

That test file is here, and the included file is here. it's worth noting that taking out that include and embedded the contents in that .phpt file has no effect on the output.

@nikic
Copy link
Member

nikic commented Jun 28, 2018

@rtheunissen The debug=0 build is with opcache. You're missing an update to https://github.com/php/php-src/blob/master/ext/opcache/zend_accelerator_util_funcs.c#L425 -- pointers to methods need to be rewired to point into the CG arena.

@nikic nikic added the RFC label Jun 28, 2018
@Majkl578
Copy link
Contributor

I'm slightly worried about the behavior of weak comparison after this change.

Given the discussion in #3297 (comment), this may make things more confusing.
I'd like to come up with RFC for declare(strict_comparisons=1) which would affect ==, !=, but also <, >, <=, >=, <=>, <>, targeting PHP-next (whichever it is - after 7.3). This would make ==/!= behave the same way as ===/!==.

I'm afraid that if this RFC was implemented sooner, it would make things more confusing.

@rtheunissen rtheunissen changed the title [RFC] Draft implementation of user-defined object comparison [RFC] User-defined object comparison Jun 28, 2018
@rtheunissen
Copy link
Contributor Author

I'm slightly worried about the behavior of weak comparison after this change.

@Majkl578 see #3297 (comment)

@rtheunissen rtheunissen force-pushed the rt-compare-to-magic-method branch 2 times, most recently from 95519dc to 20a1efc Compare July 2, 2018 12:34
@rtheunissen rtheunissen force-pushed the rt-compare-to-magic-method branch from e490eba to 7bbae20 Compare July 4, 2018 18:18
@rtheunissen
Copy link
Contributor Author

Closing due to declined RFC.

@dakur
Copy link

dakur commented Jul 19, 2022

@rtheunissen Is it possible to find any information about why it was declined? I need object equality quite often in tests so I'm curious what led to declining this.

I've searched mailing lists but without any success, maybe because I can't work with them, I don't know..

@WebDevTmas
Copy link

@rtheunissen Is it possible to find any information about why it was declined? I need object equality quite often in tests so I'm curious what led to declining this.

I've searched mailing lists but without any success, maybe because I can't work with them, I don't know..

I was just doing the same, haven't found it yet either. Seems like the vote went through without any discussion. Now I often write my own compare function in a class (typescript)

$a->equals($b); but doesn't support sorting, still have to use usort in case.

@rtheunissen
Copy link
Contributor Author

I think maybe I just didn't build enough consensus before the vote happened. The most common objection I heard was that a comparator should always be external, provided, never implicit. I don't agree with that.. if you want to override implicit behavior or compare in a non-standard way you can provide a comparator, but the object itself should know how it compares to others in a standard way. Other languages support this.. Python, C++, etc. I still think this is a good idea.

@yorksgomez
Copy link

Is there anyway to re do this voting/rfc? PHP kinda feels unnecessarily rigid because of this

@rtheunissen
Copy link
Contributor Author

Is there anyway to re do this voting/rfc? PHP kinda feels unnecessarily rigid because of this

I'm coming back to some extension work and I am once again wishing this was approved. I remember being rushed into a vote because the next version of PHP at the time was coming up quickly and I didn't want to wait for the next version. I still believe that this is a good strategy for PHP. What is the process of potentially re-opening the discussion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants