# [RFC] ICU UConverter implementation for ext/intl - Thread: https://externals.io/message/63710 - Original archive: https://news-web.php.net/php.internals/63710 - Started by Sara Golemon on 2012-10-30T01:57:31+00:00 - Messages: 19 - 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 --- ## #63710 — Sara Golemon — 2012-10-30T01:57:31+00:00 URL: https://externals.io/message/63710#63710 http://wiki.php.net/rfc/uconverter Discuss! --- ## #63711 — Pierre Joye — 2012-10-30T06:27:37+00:00 URL: https://externals.io/message/63710#63711 · In reply to: #63710 hi Sara! > http://wiki.php.net/rfc/uconverter > > Discuss! Nice job! Some comments, raw :) : - the UCNV prefix is not necessary, we are in the UConverter class already, I would simply use LATIN_1, or ILLEGAL for exampe. - It is OO, we should or could use exception. Warning are not nice to deal with and errors are quite common during conversion - I would add ERROR_ to the error related constants (UNASSIGNED & co), or? Some questions, is 'latin1' the same than const UCNV_LATIN_1? or is there any relation between the two? Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #63712 — Pierre Joye — 2012-10-30T06:29:45+00:00 URL: https://externals.io/message/63710#63712 · In reply to: #63710 btw, can you add the changes to config.w32 too pls? Pretty much the same than in .m4, so I can add it to our CI and valid the RFC patch in a more handy manner :) > http://wiki.php.net/rfc/uconverter > > Discuss! > > -- > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #63713 — Ivan Enderlin @ Hoa — 2012-10-30T06:39:35+00:00 URL: https://externals.io/message/63710#63713 · In reply to: #63710 Very nice work. Bravo! > http://wiki.php.net/rfc/uconverter > > Discuss! > -- Ivan Enderlin Developer of Hoa http://hoa.42/ or http://hoa-project.net/ PhD. student at DISC/Femto-ST (Vesontio) and INRIA (Cassis) http://disc.univ-fcomte.fr/ and http://www.inria.fr/ Member of HTML and WebApps Working Group of W3C http://w3.org/ --- ## #63715 — Gustavo Lopes — 2012-10-30T08:42:17+00:00 URL: https://externals.io/message/63710#63715 · In reply to: #63710 Em 2012-10-30 2:57, Sara Golemon escreveu: > http://wiki.php.net/rfc/uconverter > > Discuss! Good work. I do, however, have some concerns: * Error handling is done in a different way from the rest of the extension. Error handling in ext/intl is, admittedly, a little strange. Errors are both stored globally (to be retrieved by intl_get_error_code() and intl_get_error_message()) and in the object that raised them (through ::getErrorCode() and ::getErrorMessage()). The mechanism in ICU where one can do several calls in succession and check the error code only at the end is broken; the error codes are reset at the start of each call. The errors may raise any kind of PHP error (depending on the value of a INI setting, intl.error_level) or an exception (activated globally via intl.use_exceptions). On failure, functions/methods return false (constructors and factory methods return null). Unusual as this may be, it would be a bad idea to introduce inconsistency here. See the implementations in the other modules. * The tests have poor coverage. -- Gustavo Lopes --- ## #63717 — Sara Golemon — 2012-10-30T16:11:11+00:00 URL: https://externals.io/message/63710#63717 · In reply to: #63711 > - the UCNV prefix is not necessary, we are in the UConverter class > already, I would simply use LATIN_1, or ILLEGAL for exampe. > - I would add ERROR_ to the error related constants (UNASSIGNED & co), or? > For the UConverterType enum I could maybe get behind that. For UConverterCallbackReason I think we need something, but ERROR_ isn't universally applicable since three are just status incidents (UCNV_RESET, UCNV_CLOSE, UCNV_CLONE). Perhaps: REASON_* for these? In that vein, I'd be tempted to make the UConverterType values be TYPE_* > - It is OO, we should or could use exception. Warning are not nice to > deal with and errors are quite common during conversion > We are. UConverterException is thrown everywhere but one place. That one exception is the static method I added to give a non-oop-like API for the most core functionality. Given its non-oopness, I left it non-exceptiony. See Gustvo's notes about error handling below. > Some questions, is 'latin1' the same than const UCNV_LATIN_1? or is > there any relation between the two? > The UCNV_LATIN1 constant is a numeric value, so they're not the same, no. The relationship is that when you instantiate a UConverter with 'latin1' (or 'iso-8859-1', or any of its aliases), $conv->getSourceType() (or getDestinationType() as appropriate) will return UConverter::UCNV_LATIN_1. Each type in the UConverterType set uses a different algorithm approach to do the character conversion. Sets like latin1, utf*, ascii, etc... can be done faster than sets like 'koi8-r' because the conversions are simple bitshifts (very simple in the case of ascii/latin1). This just surfaces some info on what the converter is doing internally and isn't much use beyond a bit of logging, in practice. > btw, can you add the changes to config.w32 too pls? > Ah, right sorry, had meant to do that at the last minute. Will add that in a bit, > Error handling is done in a different way from the rest of the extension. > Unusual as this may be, it would be a bad idea to introduce inconsistency > here. See the implementations in the other modules. > I do agree, I was hoping I could slip that one by. :p I'll take another look at the rest of ext/intl and see about adapting my error handling. > The tests have poor coverage. > The important bits are hit, but yeah, it could stand to have better testing. I'll add some. --- ## #63718 — Stas Malyshev — 2012-10-30T16:56:58+00:00 URL: https://externals.io/message/63710#63718 · In reply to: #63710 Hi! > http://wiki.php.net/rfc/uconverter > > Discuss! Looks nice. Some points: 1. transcode() accepts options, but there's no comparable way to set options to the object. I think these APIs should be synchronized. Imagine code keeping options in array/config object - it's be really annoying to have two separate procedures to feed these to object and to transcode(). Also, description of options would be helpful. 2. Shouldn't "Enumeration and lookup" methods be static? They look like independent from encodings and don't use the object. 3. For "Advanced Use", I think "no error" condition should be the default and not requiring explicit action. 4. I think error reporting should match other intl functions. It'd not really be good if intl submodules would be all different in error reporting. 5. What is $source parameter for callbacks? 6. Why toUCallback returns string but fromUCallback gets codepoint as long? Shouldn't those be the same - i.e., if toU returns unicode codepoint, it should be long? Or it can return multiple codepoints? In which case it becomes confusing as we represent codepoints as both string and long in the same API. 7. Link to ICU API from the RFC would be helpful for reviewers and later docs, I think. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #63719 — Sara Golemon — 2012-10-30T17:26:36+00:00 URL: https://externals.io/message/63710#63719 · In reply to: #63718 > 1. transcode() accepts options, but there's no comparable way to set > options to the object. I think these APIs should be synchronized. > Imagine code keeping options in array/config object - it's be really > annoying to have two separate procedures to feed these to object and to > transcode(). > transcode() having an $options parameter is to make up for the instance version (convert()) being able to set those via instance functions (setSubstChars()). I don't picture a given app using both convert() and transcode(), the latter only exists to placate those who are objectophobic. > Also, description of options would be helpful. > They're covered in the RFC: to_subst and from_subst under "Simple Use" > 2. Shouldn't "Enumeration and lookup" methods be static? They look like > independent from encodings and don't use the object. > They are in the patch, I just forgot to note that in the RFC. Updated. > 3. For "Advanced Use", I think "no error" condition should be the > default and not requiring explicit action. > If you take no action at all, then an error still exists. This is consistent with the underlying API. > 4. I think error reporting should match other intl functions. It'd not > really be good if intl submodules would be all different in error > reporting. > Mentioned in previous feedback, I plan to look at this again. > 5. What is $source parameter for callbacks? > It's context for where in the conversion we are. $codeunit/$codepoint is the specific element causing the problem, $source is the string from that point forward. > 6. Why toUCallback returns string but fromUCallback gets codepoint as > long? Shouldn't those be the same - i.e., if toU returns unicode > codepoint, it should be long? Or it can return multiple codepoints? In > which case it becomes confusing as we represent codepoints as both > string and long in the same API. > Actually (I left this out of the RFC), they both can return a large number of types. In the case of toUCallback, you can return a utf-8 string (most reasonable Unicode representation to be returned as a char* string) and the callback mechanism will make that into UChars to put into the target string. You can return a long and it'll be treated as a single Unicode codepoint (One UChar for BMP, 2 for higher planes). You can also return an array of either of these types to specify a string in a readable, but unicode friendly format, e.g. array("Espa", 0x00F1 /* LATIN SMALL LETTER N WITH TILDE */, "ol") would be equivalent to "Espa\xC3\xB1ol". The same is true for fromUCallback() with the exception that the values being returned are assumed to be in the target encoding. For longs this means a single byte unsigned char which is appended to the target as-is. Similarly strings are appended as-is. As for input parameters: for toUCallback, $source and $codeUnits are still in their original encoding and presented as-is for that encoding. For fromUCallback(), the $source/$codePoint are in Unicode (UChar/UTF16 internall) and can't be directly offered to PHP without running into endianness issues. So the codepoint is provided as a single UChar32 (avoiding the surrogate problem in the process), and source is given as a series of UChar32 codepoints in a numerically indexed array. I'll add a section about callback input/return types to clarify this. > 7. Link to ICU API from the RFC would be helpful for reviewers and later > docs, I think. > Added! --- ## #63725 — Sara Golemon — 2012-10-30T22:18:25+00:00 URL: https://externals.io/message/63710#63725 · In reply to: #63719 With the exception of renaming the UConverter::UCNV_* constants to remove the UCNV_ prefix, I believe I've addressed the concerns thus far. ((Waiting to hear if anyone else wants to weigh in on the contants)) The RFC has been updated accordingly.+ --- ## #63733 — Ilia Alshanetsky — 2012-10-31T20:59:12+00:00 URL: https://externals.io/message/63710#63733 · In reply to: #63725 After the updates it looks really good, very handy functionality to have. > With the exception of renaming the UConverter::UCNV_* constants to > remove the UCNV_ prefix, I believe I've addressed the concerns thus > far. ((Waiting to hear if anyone else wants to weigh in on the > contants)) The RFC has been updated accordingly.+ --- ## #63734 — Adam Harvey — 2012-11-01T01:40:06+00:00 URL: https://externals.io/message/63710#63734 · In reply to: #63725 > With the exception of renaming the UConverter::UCNV_* constants to > remove the UCNV_ prefix, I believe I've addressed the concerns thus > far. ((Waiting to hear if anyone else wants to weigh in on the > contants)) The RFC has been updated accordingly.+ I would say that unprefixing makes sense in general, particularly since that's what happens in other intl classes (such as Collator). Prefixing the callback reason constants with REASON_* seems like a good compromise there — as you said upthread, they are different to the other constants defined on UConverter. Beyond that, I think the type constants would do fine as direct UConverter constants (UConverter::UTF8, for instance, makes sense to me — in general, you're using a converter because you want to deal with encoding types). I'm +1 on the functionality in general. The thought of another encoding conversion API in PHP doesn't fill me with great joy given we already have mbstring and iconv, but it does provide features neither of those libraries provide: combined with the existing intl functionality and the ever-increasing need for internationalisation support, I'd like to think we might look at nudging intl towards being the usual way of providing that functionality in PHP. Thanks, Adam, who is apparently in a run-on sentence kind of mood today. --- ## #63735 — Sara Golemon — 2012-11-01T18:37:51+00:00 URL: https://externals.io/message/63710#63735 · In reply to: #63734 Okay, I can be convinced. :) Diff and RFC updated. And I agree on the slight sense of unease at adding a third encoding converter, but ICU is so far ahead of the other two that I really feel we should encourage its use over iconv and mbstring. -Sara > > > […] > > I would say that unprefixing makes sense in general, particularly > since that's what happens in other intl classes (such as Collator). > Prefixing the callback reason constants with REASON_* seems like a > good compromise there — as you said upthread, they are different to > the other constants defined on UConverter. Beyond that, I think the > type constants would do fine as direct UConverter constants > (UConverter::UTF8, for instance, makes sense to me — in general, > you're using a converter because you want to deal with encoding > types). > > I'm +1 on the functionality in general. The thought of another > encoding conversion API in PHP doesn't fill me with great joy given we > already have mbstring and iconv, but it does provide features neither > of those libraries provide: combined with the existing intl > functionality and the ever-increasing need for internationalisation > support, I'd like to think we might look at nudging intl towards being > the usual way of providing that functionality in PHP. > > Thanks, > > Adam, who is apparently in a run-on sentence kind of mood today. --- ## #63748 — Sara Golemon — 2012-11-05T17:26:58+00:00 URL: https://externals.io/message/63710#63748 · In reply to: #63735 The requisite one week minimum has passed and there have been no further comments since my last update (which addressed all comments thereto) so I'm opening the voting process on https://wiki.php.net/rfc/uconverter Cheers, -Sara --- ## #63749 — Pierre Joye — 2012-11-05T18:25:28+00:00 URL: https://externals.io/message/63710#63749 · In reply to: #63748 Just for the record, it is one week minimum, not maximum :) If you think that the implementation details are solved and ready to be voted on, then heh, go ahead :) > The requisite one week minimum has passed and there have been no > further comments since my last update (which addressed all comments > thereto) so I'm opening the voting process on > https://wiki.php.net/rfc/uconverter > > Cheers, > -Sara > > -- > > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #63755 — Sara Golemon — 2012-11-06T03:21:20+00:00 URL: https://externals.io/message/63710#63755 · In reply to: #63749 There doesn't seem to be any further discussion. I figured if someone has more objections during the week this stays open for voting they can be addressed, and if need be the voting can be reset and resumed. -Sara > Just for the record, it is one week minimum, not maximum :) > > If you think that the implementation details are solved and ready to be > voted on, then heh, go ahead :) > > > > > […] > > > > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #63757 — Lester Caine — 2012-11-06T08:48:58+00:00 URL: https://externals.io/message/63710#63757 · In reply to: #63755 Sara Golemon wrote: > There doesn't seem to be any further discussion. I figured if someone > has more objections during the week this stays open for voting they > can be addressed, and if need be the voting can be reset and resumed. This may already be covered elsewhere, but is there a return from the class that indicates easily the number of codepoint errors? It would be helpful if the likes of the error handling single line linked through to the relevant material? I've got back to the intl documentation, but it's not clear there what is returned if a codepoint error is detected. -- Lester Caine - G8HFL ----------------------------- Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk --- ## #63762 — Sara Golemon — 2012-11-06T16:26:47+00:00 URL: https://externals.io/message/63710#63762 · In reply to: #63757 As far as what's returned by convert() in the case of codepoint errors, I thought that was explained in the usage sections, but I'll put some content into the error handling section as well (and link to the existing intl functions related to error surfacing, As to determining how many invalid codepoints were encountered, no. There's not an API to surface that at ICU's level. I could add a counter to the callback and always enable it, even when not subclassing, but that's adding complexity where it in all likelihood won't be used for something which can be done in userspace, so I'm inclined against it. -Sara > Sara Golemon wrote: > > […] > > > This may already be covered elsewhere, but is there a return from the class > that indicates easily the number of codepoint errors? > It would be helpful if the likes of the error handling single line linked > through to the relevant material? I've got back to the intl documentation, > but it's not clear there what is returned if a codepoint error is detected. > > -- > Lester Caine - G8HFL > ----------------------------- > Contact - http://lsces.co.uk/wiki/?page=contact > L.S.Caine Electronic Services - http://lsces.co.uk > EnquirySolve - http://enquirysolve.com/ > Model Engineers Digital Workshop - http://medw.co.uk > Rainbow Digital Media - http://rainbowdigitalmedia.co.uk --- ## #63763 — Lester Caine — 2012-11-06T16:55:44+00:00 URL: https://externals.io/message/63710#63763 · In reply to: #63762 Sara Golemon wrote: > As far as what's returned by convert() in the case of codepoint > errors, I thought that was explained in the usage sections, but I'll > put some content into the error handling section as well (and link to > the existing intl functions related to error surfacing, The problem with the wiki is then cross linking to the rest of the documentation. After 10 minutes I was still going around in circles ... once you know WHERE to look finding things is easier. > As to determining how many invalid codepoints were encountered, no. > There's not an API to surface that at ICU's level. I could add a > counter to the callback and always enable it, even when not > subclassing, but that's adding complexity where it in all likelihood > won't be used for something which can be done in userspace, so I'm > inclined against it. I thought that was the case, but again cross checking gets difficult. I'm more or less UFT8 based now content wise, and on the whole can just bounce UTF8 back, but identifying the number of 'extended' characters would be useful to pick up strings that would become unreadable. The odd character missed can be acceptable, but whole words are a different matter? Almost looking for a 'readability' scale rather than number of errors? > -Sara > > > > […] -- Lester Caine - G8HFL ----------------------------- Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk --- ## #64053 — Sara Golemon — 2012-11-26T21:20:29+00:00 URL: https://externals.io/message/63710#64053 · In reply to: #63748 It's been a few weeks since voting opened and it's unanimous so far. If no one objects I'll plan on pushing into git on Friday. > The requisite one week minimum has passed and there have been no > further comments since my last update (which addressed all comments > thereto) so I'm opening the voting process on > https://wiki.php.net/rfc/uconverter > > Cheers, > -Sara