# intl IDNA patch (was: Re: [PECL-DEV] libidn2 extension for php) - Thread: https://externals.io/message/56482 - Original archive: https://news-web.php.net/php.internals/56482 - Started by Stas Malyshev on 2011-11-23T07:03:26+00:00 - Messages: 11 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #56482 — Stas Malyshev — 2011-11-23T07:03:26+00:00 URL: https://externals.io/message/56482#56482 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 --- ## #56485 — Gustavo Lopes — 2011-11-23T09:03:29+00:00 URL: https://externals.io/message/56482#56485 · In reply to: #56482 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 --- ## #56488 — Stas Malyshev — 2011-11-23T09:26:15+00:00 URL: https://externals.io/message/56482#56488 · In reply to: #56485 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 --- ## #56490 — Ferenc Kovacs — 2011-11-23T09:31:48+00:00 URL: https://externals.io/message/56482#56490 · In reply to: #56488 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 > > […] > > 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, > > […] > > 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 --- ## #56491 — Stas Malyshev — 2011-11-23T09:36:55+00:00 URL: https://externals.io/message/56482#56491 · In reply to: #56490 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 --- ## #56492 — Ferenc Kovacs — 2011-11-23T09:47:20+00:00 URL: https://externals.io/message/56482#56492 · In reply to: #56491 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 > > […] > > 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 --- ## #56493 — Ferenc Kovacs — 2011-11-23T09:49:51+00:00 URL: https://externals.io/message/56482#56493 · In reply to: #56492 > > > On Wed, Nov 23, 2011 at 10:36 AM, Stas Malyshev wrote: > > > […] > > 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 --- ## #56496 — Pierre Joye — 2011-11-23T10:27:11+00:00 URL: https://externals.io/message/56482#56496 · In reply to: #56488 > 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 --- ## #56578 — Gustavo Lopes — 2011-11-24T17:58:02+00:00 URL: https://externals.io/message/56482#56578 · In reply to: #56488 I've committed support for UTS #46 to 5.4 and trunk. See http://svn.php.net/viewvc?view=revision&revision=319770 -- Gustavo Lopes --- ## #56579 — Pierre Joye — 2011-11-24T18:26:43+00:00 URL: https://externals.io/message/56482#56579 · In reply to: #56578 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 --- ## #56592 — Stas Malyshev — 2011-11-24T21:44:28+00:00 URL: https://externals.io/message/56482#56592 · In reply to: #56578 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