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:
-
I consider it a bugfix, so I am OK with getting it in anytime it's
ready. Thanks for making the quick patch, Gustavo! -
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). -
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. -
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 beNULLthere at all
but if we check it, let's check it right :) Another place has the same
code. -
Also we'd want some tests with that ;)
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
On Wed, 23 Nov 2011 07:03:26 -0000, Stas Malyshev smalyshev@sugarcrm.com
wrote:
- 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.
- 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.
- 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 beNULLthere 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.
- Also we'd want some tests with that ;)
Sure. I will also test both against ICU 4.2 and 4.8.
--
Gustavo Lopes
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
On Wed, Nov 23, 2011 at 10:26 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
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
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
On Wed, Nov 23, 2011 at 10:36 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
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
On Wed, Nov 23, 2011 at 10:36 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
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. 227oh, 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
internals@lists.php.net/msg54542.html" rel="nofollow" target="_blank">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
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,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
I've committed support for UTS #46 to 5.4 and trunk.
See http://svn.php.net/viewvc?view=revision&revision=319770
--
Gustavo Lopes
hi Gustavo!
Thanks!
can you add a note to UPGRADING and in the bug report please?
Cheers,
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
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Hi!
I've committed support for UTS #46 to 5.4 and trunk.
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