FAQ
Hi!

First of all, I would like to politely ask everybody on the list to
change subject if, well, subject of the discussion changes. I was
totally under impression that this topic still discusses libidn2
extension in PECL and might miss discussion about intl IDNA patch if
David didn't point it to me (thanks!). Let's use subjects for their
intended purpose :)

Now about the patch:
0. I consider it a bugfix, so I am OK with getting it in anytime it's
ready. Thanks for making the quick patch, Gustavo!

1. I'm not sure I understand why we need two options fields. We already
have one options field, won't that be enough? We can combine options and
space them in a way that old ones work fine with new ones, can't we?
And have default variant so one of them is always true (probably 2003).

2. I think optional by-ref parameter to receive IDNA-specific error
codes is right. Generic intl error reporting is about reporting, well,
generic errors and is not meant to have granularity enough to store
whole UIDNAInfo stuff.

3. I'm not sure I understand the code in php_intl_bad_args() - it
composes error message into the buff but then only sets it if msg !=
NULL. It doesn't check msg before using it in sprintf. Looks like
something's missing there: I don't see how msg can be NULL there at all
but if we check it, let's check it right :) Another place has the same
code.

4. Also we'd want some tests with that ;)

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Search Discussions

  • Gustavo Lopes at Nov 23, 2011 at 9:03 am

    On Wed, 23 Nov 2011 07:03:26 -0000, Stas Malyshev wrote:

    1. I'm not sure I understand why we need two options fields. We already
    have one options field, won't that be enough? We can combine options and
    space them in a way that old ones work fine with new ones, can't we?
    And have default variant so one of them is always true (probably 2003).
    Technically, yes, it is possible. But is it desirable? It would require
    breaking the abstraction and looking at the actual values of the flags,
    choosing one of the unused bits (possibly a high one) and hope it'll never
    be used in future. Plus, in the current patch, the value of the variant
    completely changes the return value (from string to array) so if it's more
    appropriate to single it out.
    2. I think optional by-ref parameter to receive IDNA-specific error
    codes is right. Generic intl error reporting is about reporting, well,
    generic errors and is not meant to have granularity enough to store
    whole UIDNAInfo stuff.
    That's what they've must felt lately too, because the IDNA2003
    implementation uses the generic intl error reporting.
    I don't really care or way or the other about pass-by-ref vs mixed return,
    but I don't think your position on that issue is clear here, as you only
    affirm that trying to force generic intl error reporting is not an option
    (which I've also argued). If there are no objections, I'll advance with
    mixed return, as Pierre has announced.
    3. I'm not sure I understand the code in php_intl_bad_args() - it
    composes error message into the buff but then only sets it if msg !=
    NULL. It doesn't check msg before using it in sprintf. Looks like
    something's missing there: I don't see how msg can be NULL there at all
    but if we check it, let's check it right :) Another place has the same
    code.
    Ah yes, nice catch. Not that it hurts because the functions are never
    called with NULL and they're static, but that part is nonsensical.
    4. Also we'd want some tests with that ;)
    Sure. I will also test both against ICU 4.2 and 4.8.

    --
    Gustavo Lopes
  • Stas Malyshev at Nov 23, 2011 at 9:26 am
    Hi!
    Technically, yes, it is possible. But is it desirable? It would require
    breaking the abstraction and looking at the actual values of the flags,
    choosing one of the unused bits (possibly a high one) and hope it'll never
    be used in future. Plus, in the current patch, the value of the variant
    completely changes the return value (from string to array) so if it's more
    appropriate to single it out.
    I think yes, it's better to have one options set than "options" and
    "another options which aren't first options but different options". I
    don't think there's breaking of abstraction if we use more options that
    ICU has, and probability of ICU using all 32 bits seems quite low for me
    now.

    I don't think we should return array there - it makes using the function
    much more awkward since you need to check options before you know what
    it returns. I think it's better to have normal return always a string,
    abnormal can be handled by a special value if the user cares.
    I don't really care or way or the other about pass-by-ref vs mixed return,
    but I don't think your position on that issue is clear here, as you only
    affirm that trying to force generic intl error reporting is not an option
    (which I've also argued). If there are no objections, I'll advance with
    mixed return, as Pierre has announced.
    Actually, that's what I was objecting to. I think mixed return (i.e.
    function returning array or string depending on option) is not the best
    option. I think for most use cases people would just use the string and
    if the return is false they can either say "bad domain name" and be done
    with it, or parse the error codes and create special handling for each
    of them if they like. But I don't think there's a need to add so much
    complication by making the same function return two types.

    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
  • Ferenc Kovacs at Nov 23, 2011 at 9:31 am

    On Wed, Nov 23, 2011 at 10:26 AM, Stas Malyshev wrote:

    Hi!

    Technically, yes, it is possible. But is it desirable? It would require
    breaking the abstraction and looking at the actual values of the flags,
    choosing one of the unused bits (possibly a high one) and hope it'll never
    be used in future. Plus, in the current patch, the value of the variant
    completely changes the return value (from string to array) so if it's more
    appropriate to single it out.
    I think yes, it's better to have one options set than "options" and
    "another options which aren't first options but different options". I don't
    think there's breaking of abstraction if we use more options that ICU has,
    and probability of ICU using all 32 bits seems quite low for me now.
    Yeah, albeit low, there is still a chance that in the future we got some
    clashing between their options and ours, so I think it would be better to
    separate those.

    I don't think we should return array there - it makes using the function
    much more awkward since you need to check options before you know what it
    returns. I think it's better to have normal return always a string,
    abnormal can be handled by a special value if the user cares.

    I don't really care or way or the other about pass-by-ref vs mixed return,
    but I don't think your position on that issue is clear here, as you only
    affirm that trying to force generic intl error reporting is not an option
    (which I've also argued). If there are no objections, I'll advance with
    mixed return, as Pierre has announced.
    Actually, that's what I was objecting to. I think mixed return (i.e.
    function returning array or string depending on option) is not the best
    option. I think for most use cases people would just use the string and if
    the return is false they can either say "bad domain name" and be done with
    it, or parse the error codes and create special handling for each of them
    if they like. But I don't think there's a need to add so much complication
    by making the same function return two types.

    I thought that we already agreed using an output argument for getting the
    specific error instead of returning either a string or an array.

    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Stas Malyshev at Nov 23, 2011 at 9:37 am
    Hi!
    I thought that we already agreed using an output argument for getting
    the specific error instead of returning either a string or an array.
    That's what I was thinking too, but Gustavo seems to plan to do mixed
    return, which I think is a worse option.
    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
  • Ferenc Kovacs at Nov 23, 2011 at 9:47 am

    On Wed, Nov 23, 2011 at 10:36 AM, Stas Malyshev wrote:

    Hi!


    I thought that we already agreed using an output argument for getting
    the specific error instead of returning either a string or an array.
    That's what I was thinking too, but Gustavo seems to plan to do mixed
    return, which I think is a worse option.

    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
    oh, I skipped "I'll advance with mixed return, as Pierre has announced."
    when reading the mail.
    I think that is only a typo, as Pierre and Gustavo talked about this on
    irc, and they both agreed that the output argument is the way to go.

    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Ferenc Kovacs at Nov 23, 2011 at 9:49 am
    On Wed, Nov 23, 2011 at 10:47 AM, Ferenc Kovacs wrote:
    On Wed, Nov 23, 2011 at 10:36 AM, Stas Malyshev wrote:

    Hi!


    I thought that we already agreed using an output argument for getting
    the specific error instead of returning either a string or an array.
    That's what I was thinking too, but Gustavo seems to plan to do mixed
    return, which I think is a worse option.

    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
    oh, I skipped "I'll advance with mixed return, as Pierre has announced."
    when reading the mail.
    I think that is only a typo, as Pierre and Gustavo talked about this on
    irc, and they both agreed that the output argument is the way to go.
    see the Pierre's announcement
    http://www.mail-archive.com/internals@lists.php.net/msg54542.html
    the first option was the output argument.

    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Pierre Joye at Nov 23, 2011 at 10:27 am

    On Wed, Nov 23, 2011 at 10:26 AM, Stas Malyshev wrote:

    I think yes, it's better to have one options set than "options" and "another
    options which aren't first options but different options". I don't think
    there's breaking of abstraction if we use more options that ICU has, and
    probability of ICU using all 32 bits seems quite low for me now.

    I don't think we should return array there - it makes using the function
    much more awkward since you need to check options before you know what it
    returns. I think it's better to have normal return always a string, abnormal
    can be handled by a special value if the user cares.
    yes, see my reply. We agreed that these functions should always return
    a string or false (on error).

    Which kind of action will be take is defined by the new argument, and
    possible errors can be returned using the last new argument (array by
    ref). It is however important to give the caller a way to deal with
    the possible warnings or errors.
    Actually, that's what I was objecting to. I think mixed return (i.e.
    function returning array or string depending on option) is not the best
    option.
    That's not what we agreed on. I did not check the last version of the
    patch but we agreed on string|false only as return value :)


    Cheers,
  • Gustavo Lopes at Nov 24, 2011 at 5:58 pm
    I've committed support for UTS #46 to 5.4 and trunk.

    See http://svn.php.net/viewvc?view=revision&revision=319770

    --
    Gustavo Lopes
  • Pierre Joye at Nov 24, 2011 at 6:26 pm
    hi Gustavo!

    Thanks!

    can you add a note to UPGRADING and in the bug report please?

    Cheers,
    On Thu, Nov 24, 2011 at 6:58 PM, Gustavo Lopes wrote:
    I've committed support for UTS #46 to 5.4 and trunk.

    See http://svn.php.net/viewvc?view=revision&revision=319770

    --
    Gustavo Lopes

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
  • Stas Malyshev at Nov 24, 2011 at 9:44 pm
    Hi!
    I've committed support for UTS #46 to 5.4 and trunk.

    See http://svn.php.net/viewvc?view=revision&revision=319770
    Could you please also fix the protos on the functions? And updating the
    docs would be ideal :)

    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedNov 23, '11 at 7:03a
activeNov 24, '11 at 9:44p
posts11
users4
websitephp.net

People

Translate

site design / logo © 2022 Grokbase