Re: svn: /php /php-src/trunk/ NEWS ext/intl/config.m4 ext /intl/config.w32 ext/intl/php intl.c ex t/intl/spoofchecker/spoofchecker.c ext/intl/spo
ofchecker/spoofchecker.h ext/intl/spoofchecker/ spoofchecker class.c ext/intl/spoofchecker/ ...

On Jan 12, 2011, at 11:11 PM, Stas Malyshev wrote: > Hi! > > Some of questions: > 1. Why clone is not defined? uspoof_clone exists. Done > 2. > /* {{{ proto void Spoofchecker::areConfusable( string $str1, string $str2[, int $error_code ] ) > + * Checks if a given text contains any confusable characters > + */ > > It says void but code returns bool. Same with isSuspicious. > Also, is 'issued_found' a typo? > Why wouldn't this function just return the error code? Yeah, it's meant to return bool. Just a typo in the prototype. > > 3. + if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) { > + zend_throw_exception(zend_exception_get_default(TSRMLS_C), > u_errorName(SPOOFCHECKER_ERROR_CODE(co)), SPOOFCHECKER_ERROR_CODE(co) TSRMLS_CC); > > None of intl parts throw exceptions on errors. Please bring the code in sync with the rest of > the module. what value would it return instead of an exception? false? It returns a bool already, so false means its not suspicious. So if some random error occurred it would imply there is no security issue. > > 4. + SPOOFCHECKER_METHOD_FETCH_OBJECT > Please put ; at the end of the line. I know it's not required, but the line without ; at > the end makes the code less readable - each time you look at it you think "is there a problem > here?" Done, but surely we should kill ; from the macro then so it becomes required. > > 5. Why setAllowedLocales but no setAllowedChars? Couldn't see an immediate use, but i'll add it shortly. - Scott