Grokbase › Groups › PHP › php-internals › February 2009
FAQ

Ilia Alshanetsky wrote:
I've discussed this issue with Andrei at least a month ago (if not
longer) when the patch was originally added, and I believe that the
introduced behavior is the correct one.
IMO correct or not depends on the context where the function is used.

At least, as array_unique() was not capable of dealing with objects
before the Andrei's patch, every existing code should use it with
strings, not objects.

If SORT_REGULAR could handle objects as well as strings in the same
manner as SORT_STRING I wouldn't see any problem, although it cannot.

Moriyoshi

On 14-Feb-09, at 9:12 PM, Moriyoshi Koizumi wrote:

So, what are RM's thoughts on this? My points are:

1. Making SORT_REGULAR default *actually* broke existing code.
2. Adding the second argument addressed the problem enough that the
elements are treated indifferently when used with objects.

Regards,
Moriyoshi

Moriyoshi Koizumi wrote:
Whatever reasoning, I don't think it's a good idea to revert someone
else's patch before discussing anything.

Aside from this, I agree with you the old behavior is that stupid, but
BC should always be honored.

Moriyoshi

Andrei Zmievski wrote:
Don't do this please. Why did you feel the need to go back and
change my
patch including the NEWS entry? I knew what I was doing when I set the
default behavior to SORT_REGULAR and this was discussed with both 5.3
and 5.2 RMs. With your change it'l back to the stupid old behavior of:

$array = array(new stdClass(), new stdClass(), new Foo());
$array = array_unique($array);

And now $array has only 1 element. I really hate having tell PHP not to
be stupid, rather than having it default to being smart.

I'm going to revert this.

-Andrei

Moriyoshi Koizumi wrote:
moriyoshi Thu Feb 12 18:29:15 2009 UTC

Modified files: /php-src/ext/standard array.c
Log:
* Fix bug #47370 (BC breakage of array_unique())

http://cvs.php.net/viewvc.cgi/php-src/ext/standard/array.c?r1=1.471&r2=1.472&diff_format=u


Index: php-src/ext/standard/array.c
diff -u php-src/ext/standard/array.c:1.471
php-src/ext/standard/array.c:1.472
--- php-src/ext/standard/array.c:1.471 Mon Feb 9 10:47:19 2009
+++ php-src/ext/standard/array.c Thu Feb 12 18:29:15 2009
@@ -21,7 +21,7 @@

+----------------------------------------------------------------------+

*/

-/* $Id: array.c,v 1.471 2009/02/09 10:47:19 dmitry Exp $ */
+/* $Id: array.c,v 1.472 2009/02/12 18:29:15 moriyoshi Exp $ */

#include "php.h"
#include "php_ini.h"
@@ -2924,7 +2924,7 @@
};
struct bucketindex *arTmp, *cmpdata, *lastkept;
unsigned int i;
- long sort_type = PHP_SORT_REGULAR;
+ long sort_type = PHP_SORT_STRING;

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "a|l",
&array, &sort_type) == FAILURE) {
return;

Ilia Alshanetsky

Search Discussions

  • Moriyoshi Koizumi at Feb 17, 2009 at 2:20 pm ⇧
    Anyone's thoughts on this?

    I'm thinking of re-reverting in 48 hours without further objections.

    Moriyoshi
    On Sun, Feb 15, 2009 at 11:48 PM, Moriyoshi Koizumi wrote:
    Ilia Alshanetsky wrote:
    I've discussed this issue with Andrei at least a month ago (if not
    longer) when the patch was originally added, and I believe that the
    introduced behavior is the correct one.
    IMO correct or not depends on the context where the function is used.

    At least, as array_unique() was not capable of dealing with objects
    before the Andrei's patch, every existing code should use it with
    strings, not objects.

    If SORT_REGULAR could handle objects as well as strings in the same
    manner as SORT_STRING I wouldn't see any problem, although it cannot.

    Moriyoshi

    On 14-Feb-09, at 9:12 PM, Moriyoshi Koizumi wrote:

    So, what are RM's thoughts on this? My points are:

    1. Making SORT_REGULAR default *actually* broke existing code.
    2. Adding the second argument addressed the problem enough that the
    elements are treated indifferently when used with objects.

    Regards,
    Moriyoshi

    Moriyoshi Koizumi wrote:
    Whatever reasoning, I don't think it's a good idea to revert someone
    else's patch before discussing anything.

    Aside from this, I agree with you the old behavior is that stupid, but
    BC should always be honored.

    Moriyoshi

    Andrei Zmievski wrote:
    Don't do this please. Why did you feel the need to go back and
    change my
    patch including the NEWS entry? I knew what I was doing when I set the
    default behavior to SORT_REGULAR and this was discussed with both 5.3
    and 5.2 RMs. With your change it'l back to the stupid old behavior of:

    $array = array(new stdClass(), new stdClass(), new Foo());
    $array = array_unique($array);

    And now $array has only 1 element. I really hate having tell PHP not to
    be stupid, rather than having it default to being smart.

    I'm going to revert this.

    -Andrei

    Moriyoshi Koizumi wrote:
    moriyoshi Thu Feb 12 18:29:15 2009 UTC

    Modified files: /php-src/ext/standard array.c
    Log:
    * Fix bug #47370 (BC breakage of array_unique())

    http://cvs.php.net/viewvc.cgi/php-src/ext/standard/array.c?r1=1.471&r2=1.472&diff_format=u


    Index: php-src/ext/standard/array.c
    diff -u php-src/ext/standard/array.c:1.471
    php-src/ext/standard/array.c:1.472
    --- php-src/ext/standard/array.c:1.471 Mon Feb 9 10:47:19 2009
    +++ php-src/ext/standard/array.c Thu Feb 12 18:29:15 2009
    @@ -21,7 +21,7 @@

    +----------------------------------------------------------------------+

    */

    -/* $Id: array.c,v 1.471 2009/02/09 10:47:19 dmitry Exp $ */
    +/* $Id: array.c,v 1.472 2009/02/12 18:29:15 moriyoshi Exp $ */

    #include "php.h"
    #include "php_ini.h"
    @@ -2924,7 +2924,7 @@
    };
    struct bucketindex *arTmp, *cmpdata, *lastkept;
    unsigned int i;
    - long sort_type = PHP_SORT_REGULAR;
    + long sort_type = PHP_SORT_STRING;

    if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "a|l",
    &array, &sort_type) == FAILURE) {
    return;

    Ilia Alshanetsky


  • Andrei Zmievski at Feb 17, 2009 at 6:11 pm ⇧

    Moriyoshi Koizumi wrote:
    Ilia Alshanetsky wrote:
    I've discussed this issue with Andrei at least a month ago (if not
    longer) when the patch was originally added, and I believe that the
    introduced behavior is the correct one.
    IMO correct or not depends on the context where the function is used.

    At least, as array_unique() was not capable of dealing with objects
    before the Andrei's patch, every existing code should use it with
    strings, not objects.

    If SORT_REGULAR could handle objects as well as strings in the same
    manner as SORT_STRING I wouldn't see any problem, although it cannot.
    SORT_STRING can only reliably deal with strings - its behavior on non-string type is
    basically broken. Unless we agree that PHP is Tcl (strings are the only type), then
    SORT_REGULAR makes much more sense to me, and evidently others.

    If you really have a huge problem with BC, perhaps we could leave the default behavior as
    SORT_STRING for 5.2.x, but it definitely needs to be SORT_REGULAR for 5.3/6.

    -Andrei
  • Moriyoshi Koizumi at Feb 18, 2009 at 2:43 am ⇧

    On Wed, Feb 18, 2009 at 3:11 AM, Andrei Zmievski wrote:

    SORT_STRING can only reliably deal with strings - its behavior on non-string
    type is basically broken. Unless we agree that PHP is Tcl (strings are the
    only type), then SORT_REGULAR makes much more sense to me, and evidently
    others.

    If you really have a huge problem with BC, perhaps we could leave the
    default behavior as SORT_STRING for 5.2.x, but it definitely needs to be
    SORT_REGULAR for 5.3/6.
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.

    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.

    Moriyoshi
    -Andrei
  • Moriyoshi Koizumi at Feb 18, 2009 at 2:54 am ⇧
    In addition, we should look at similar comparison-involved array
    functions such as array_intersect, array_diff and so on, otherwise
    it's gonna be a mess.

    Moriyoshi
    On Wed, Feb 18, 2009 at 11:43 AM, Moriyoshi Koizumi wrote:
    On Wed, Feb 18, 2009 at 3:11 AM, Andrei Zmievski wrote:

    SORT_STRING can only reliably deal with strings - its behavior on non-string
    type is basically broken. Unless we agree that PHP is Tcl (strings are the
    only type), then SORT_REGULAR makes much more sense to me, and evidently
    others.

    If you really have a huge problem with BC, perhaps we could leave the
    default behavior as SORT_STRING for 5.2.x, but it definitely needs to be
    SORT_REGULAR for 5.3/6.
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.

    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.

    Moriyoshi
    -Andrei
  • Andrei Zmievski at Feb 18, 2009 at 7:56 pm ⇧
    Yes, those should be fixed too, but it's more difficult to do because they accept varargs,
    so not clear where the flag should go.

    -Andrei

    Moriyoshi Koizumi wrote:
    In addition, we should look at similar comparison-involved array
    functions such as array_intersect, array_diff and so on, otherwise
    it's gonna be a mess.

    Moriyoshi
    On Wed, Feb 18, 2009 at 11:43 AM, Moriyoshi Koizumi wrote:
    On Wed, Feb 18, 2009 at 3:11 AM, Andrei Zmievski wrote:

    SORT_STRING can only reliably deal with strings - its behavior on non-string
    type is basically broken. Unless we agree that PHP is Tcl (strings are the
    only type), then SORT_REGULAR makes much more sense to me, and evidently
    others.

    If you really have a huge problem with BC, perhaps we could leave the
    default behavior as SORT_STRING for 5.2.x, but it definitely needs to be
    SORT_REGULAR for 5.3/6.
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.

    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.

    Moriyoshi
    -Andrei
  • Andrei Zmievski at Feb 18, 2009 at 7:51 pm ⇧

    Moriyoshi Koizumi wrote:
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.
    Should we have array_unique_for_non_strings() then or something?
    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.
    What sort of notices do you propose? At runtime or in the docs?

    -Andrei
  • Moriyoshi Koizumi at Feb 19, 2009 at 2:52 am ⇧

    On Thu, Feb 19, 2009 at 4:51 AM, Andrei Zmievski wrote:
    Moriyoshi Koizumi wrote:
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.
    Should we have array_unique_for_non_strings() then or something?
    That'd be stupid. What is reasonable is to make it default to
    SORT_STRING... Or is there any strong reason to make the default
    SORT_REGULAR now that you can specify SORT_REGULAR to array_unique()
    throught the second argument?
    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.
    What sort of notices do you propose? At runtime or in the docs?
    You even didn't mention that the behavior would be changed starting
    with 5.2.9 in the document, instead you simply added the description
    for the second optional argument that defaults to SORT_REGULAR, as if
    it was the default long before. That's absolutely the thing we should
    not do.

    Eitherway, if we were to make such change, I think we should at least
    make the second argument mandatory.

    Moriyoshi
    -Andrei
  • Ian Eure at Feb 19, 2009 at 6:15 am ⇧

    On Feb 18, 2009, at 6:52 PM, Moriyoshi Koizumi wrote:

    On Thu, Feb 19, 2009 at 4:51 AM, Andrei Zmievski <andrei@gravitonic.com
    wrote:
    Moriyoshi Koizumi wrote:
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any
    change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.
    Should we have array_unique_for_non_strings() then or something?
    That'd be stupid. What is reasonable is to make it default to
    SORT_STRING... Or is there any strong reason to make the default
    SORT_REGULAR now that you can specify SORT_REGULAR to array_unique()
    throught the second argument?
    Because it's bad to require an argument to make a function do the
    right thing; it should do the right thing by default.

    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.
    What sort of notices do you propose? At runtime or in the docs?
    You even didn't mention that the behavior would be changed starting
    with 5.2.9 in the document, instead you simply added the description
    for the second optional argument that defaults to SORT_REGULAR, as if
    it was the default long before. That's absolutely the thing we should
    not do.
    Why would it be necessary? SORT_REGULAR is a superset of SORT_STRING,
    yes? If the function should "only be used with strings" (prior to this
    patch), as you claim, that's not a BC break. Unless you're arguing
    that we should preserve BC for code calling functions in unsupported
    ways.

    Eitherway, if we were to make such change, I think we should at least
    make the second argument mandatory.
    Wouldn't this be an even bigger BC break than fixing the existing
    (broken) behavior?

    - Ian
  • Moriyoshi Koizumi at Feb 19, 2009 at 7:26 am ⇧

    On Thu, Feb 19, 2009 at 3:14 PM, Ian Eure wrote:
    On Feb 18, 2009, at 6:52 PM, Moriyoshi Koizumi wrote:

    On Thu, Feb 19, 2009 at 4:51 AM, Andrei Zmievski <andrei@gravitonic.com>
    wrote:
    Moriyoshi Koizumi wrote:
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.
    Should we have array_unique_for_non_strings() then or something?
    That'd be stupid. What is reasonable is to make it default to
    SORT_STRING... Or is there any strong reason to make the default
    SORT_REGULAR now that you can specify SORT_REGULAR to array_unique()
    throught the second argument?
    Because it's bad to require an argument to make a function do the right
    thing; it should do the right thing by default.
    As I said, whether it is right or not is pretty much a context-depndent matter.
    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.
    What sort of notices do you propose? At runtime or in the docs?
    You even didn't mention that the behavior would be changed starting
    with 5.2.9 in the document, instead you simply added the description
    for the second optional argument that defaults to SORT_REGULAR, as if
    it was the default long before. That's absolutely the thing we should
    not do.
    Why would it be necessary? SORT_REGULAR is a superset of SORT_STRING, yes?
    If the function should "only be used with strings" (prior to this patch), as
    you claim, that's not a BC break. Unless you're arguing that we should
    preserve BC for code calling functions in unsupported ways.
    If SORT_REGULAR was a superset of SORT_STRING, there would have been
    no problem in the first place. See the original bug report to see what
    is to be broken.
    Eitherway, if we were to make such change, I think we should at least
    make the second argument mandatory.
    Wouldn't this be an even bigger BC break than fixing the existing (broken)
    behavior?
    A little sarcasm, the point here is that it'd be better off making the
    *incompatible* application not work at all than still allowing them to
    silently malfunction, but such a decision is just a bit less absurd
    than changing the default, as you pointed out.

    Anyway that's basically not a fix IMO.

    Moriyoshi
    - Ian

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
  • Moriyoshi Koizumi at Feb 25, 2009 at 8:26 pm ⇧
    Last call: any more objections?

    Moriyoshi
    On Thu, Feb 19, 2009 at 11:52 AM, Moriyoshi Koizumi wrote:
    On Thu, Feb 19, 2009 at 4:51 AM, Andrei Zmievski wrote:
    Moriyoshi Koizumi wrote:
    As I said earlier, the function is never supposed to be used with
    objects. Therefore, we cannot declare it to be broken, and any change
    to the behavior anyway leads to a huge BC break. I got a report that
    claims the reporter's real-world application behaves strangely with
    the latest release candidate.
    Should we have array_unique_for_non_strings() then or something?
    That'd be stupid. What is reasonable is to make it default to
    SORT_STRING... Or is there any strong reason to make the default
    SORT_REGULAR now that you can specify SORT_REGULAR to array_unique()
    throught the second argument?
    That said, I'm not really against making SORT_REGULAR default for
    later versions than 5.2.x as long as *appropriate notices* are
    provided, while I strongly disagree for 5.2.x.
    What sort of notices do you propose? At runtime or in the docs?
    You even didn't mention that the behavior would be changed starting
    with 5.2.9 in the document, instead you simply added the description
    for the second optional argument that defaults to SORT_REGULAR, as if
    it was the default long before. That's absolutely the thing we should
    not do.

    Eitherway, if we were to make such change, I think we should at least
    make the second argument mandatory.

    Moriyoshi
    -Andrei

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedFeb 15, '09 at 2:38p
activeFeb 25, '09 at 8:26p
posts11
users3
websitephp.net

3 users in discussion

Moriyoshi Koizumi: 7 posts Andrei Zmievski: 3 posts Ian Eure: 1 post

Content

People

Support

Translate

site design / logo © 2022 Grokbase