Skip to content

ext/intl: introduce SpoofChecker::areBidiConfusable. #13469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ext/intl: introduce SpoofChecker::areBidiConfusable. #13469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ext/intl/spoofchecker/spoofchecker.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ class Spoofchecker
/** @cvalue USPOOF_HIDDEN_OVERLAY */
public const int HIDDEN_OVERLAY = UNKNOWN;
#endif
#if U_ICU_VERSION_MAJOR_NUM >= 74
/** @cvalue UBIDI_LTR */
public const int UBIDI_LTR = UNKNOWN;
/** @cvalue UBIDI_RTL */
public const int UBIDI_RTL = UNKNOWN;
/** @cvalue UBIDI_MIXED */
public const int UBIDI_MIXED = UNKNOWN;
/** @cvalue UBIDI_NEUTRAL */
public const int UBIDI_NEUTRAL = UNKNOWN;
#endif

public function __construct() {}

Expand All @@ -64,4 +74,10 @@ public function setChecks(int $checks): void {}
/** @tentative-return-type */
public function setRestrictionLevel(int $level): void {}
#endif
#if U_ICU_VERSION_MAJOR_NUM >= 74
/**
* @param int $errorCode
*/
public function areBidiConfusable(int $direction, string $string1, string $string2, &$errorCode = null): bool {}
#endif
}
49 changes: 48 additions & 1 deletion ext/intl/spoofchecker/spoofchecker_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 39 additions & 0 deletions ext/intl/spoofchecker/spoofchecker_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,42 @@ PHP_METHOD(Spoofchecker, setRestrictionLevel)
}
/* }}} */
#endif

#if U_ICU_VERSION_MAJOR_NUM >= 74
PHP_METHOD(Spoofchecker, areBidiConfusable)
{
int ret;
char *id1, *id2;
size_t length1, length2;
zend_long direction;
zval *error_code = NULL;
SPOOFCHECKER_METHOD_INIT_VARS;

if (FAILURE == zend_parse_parameters(ZEND_NUM_ARGS(), "lss|z", &direction, &id1, &length1,
&id2, &length2, &error_code)) {
RETURN_THROWS();
}

SPOOFCHECKER_METHOD_FETCH_OBJECT;
if (direction != UBIDI_LTR && direction != UBIDI_RTL) {
zend_argument_value_error(1, "must be either SpoofChecker::UBIDI_LTR or SpoofChecker::UBIDI_RTL");
RETURN_THROWS();
}
if(length1 > INT32_MAX || length2 > INT32_MAX) {
SPOOFCHECKER_ERROR_CODE(co) = U_BUFFER_OVERFLOW_ERROR;
} else {
ret = uspoof_areBidiConfusableUTF8(co->uspoof, (UBiDiDirection)direction, id1, (int32_t)length1, id2, (int32_t)length2, SPOOFCHECKER_ERROR_CODE_P(co));
}
if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) {
php_error_docref(NULL, E_WARNING, "(%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));
RETURN_TRUE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we return true in case on failures? Is this behavior OK? Shouldn't we rather throw?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was being consistent with the existing areConfusable method. I may adjust spoofchecker class behavior in a separate commit like I m doing with IntlTimeZone

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the failures are related to programmatic errors, then it's surely better to throw. The one error case I can immediately see is however related to string length, which seems like a user-related one, but I guess it should be quite rare to exceed the limit. that's why I'm not really sure what to do. If we threw an exception, then the ugly reference param could also be avoided, which would be a nice advantage.

cc. @Girgias Do you have any opinion about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehhhh not really, but shouldn't this go through the "normal" intl extension error handling mechanism so user can set if they want it to be silent/warning/error?

}

if (error_code) {
zval_ptr_dtor(error_code);
ZVAL_LONG(Z_REFVAL_P(error_code), ret);
Z_TRY_ADDREF_P(error_code);
}
RETVAL_BOOL(ret != 0);
}
#endif
21 changes: 21 additions & 0 deletions ext/intl/tests/spoofchecker_ubidi.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
spoofchecker checks if strings are confusable in a given direction.
--EXTENSIONS--
intl
--SKIPIF--
<?php if(!class_exists("Spoofchecker")) print 'skip'; ?>
<?php if (version_compare(INTL_ICU_VERSION, '74.0') < 0)die('skip for ICU >= 74'); ?>
--FILE--
<?php
$s = new SpoofChecker();
try {
$s->areBidiConfusable(SpoofChecker::UBIDI_RTL + 1, "a", "a");
} catch (ValueError $e) {
echo $e->getMessage() . PHP_EOL;
}
var_dump($s->areBidiConfusable(SpoofChecker::UBIDI_LTR, "\x73\x63", "sc"));
var_dump($s->areBidiConfusable(SpoofChecker::UBIDI_RTL, "sc", "\x73\x63"));
--EXPECT--
Spoofchecker::areBidiConfusable(): Argument #1 ($direction) must be either SpoofChecker::UBIDI_LTR or SpoofChecker::UBIDI_RTL
bool(true)
bool(true)