Skip to content

Conversation

@sudam802
Copy link

This PR implements the grapheme_levenshtein() function for:

  • symfony/polyfill-intl-grapheme
  • symfony/polyfill-php85 (when intl is loaded)

Includes:
✔ Function implementation
✔ ICU-compatible segmentation
✔ Test file: tests/Intl/Grapheme/grapheme_levenshtein.phpt


\define('SYMFONY_GRAPHEME_CLUSTER_RX', ((float) \PCRE_VERSION < 10 ? (float) \PCRE_VERSION >= 8.32 : (float) \PCRE_VERSION >= 10.39) ? '\X' : Grapheme::GRAPHEME_CLUSTER_RX);
\define('SYMFONY_GRAPHEME_CLUSTER_RX',
(false !== @preg_match('/\X/u', "a"))
Copy link
Member

Choose a reason for hiding this comment

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

This change looks suspicious to me. We were explicitly checking the PCRE version because older versions of PCRE also have a broken implementation of \X (there is even an open discussion mentioning that we might need to increase the bound from which we use the native feature)

*/
public static function grapheme_levenshtein($string1, $string2, $insertion_cost = 1, $replacement_cost = 1, $deletion_cost = 1)
{
// Cast (PHP does this)
Copy link
Member

Choose a reason for hiding this comment

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

use native parameter types, so that PHP actually does it.

if (\extension_loaded('intl') && !function_exists('grapheme_levenshtein')) {
function grapheme_levenshtein($string1, $string2, $insertion_cost = 1, $replacement_cost = 1, $deletion_cost = 1)
{
return Grapheme::grapheme_levenshtein($string1, $string2, $insertion_cost, $replacement_cost, $deletion_cost);
Copy link
Member

Choose a reason for hiding this comment

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

The implementation must be duplicated in the Php85 package if we want to provide it here, not call the other package.


use Symfony\Polyfill\Intl\Grapheme\Grapheme;

return [
Copy link
Member

Choose a reason for hiding this comment

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

this file does not make sense. It does not provide a phpt test, and we use PHPUnit tests anyway.

Copy link
Author

Choose a reason for hiding this comment

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

I just wanted to make test file I'll look more deeper in this implementation

Copy link
Member

Choose a reason for hiding this comment

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

the file you added is not a test file at all. It is not testing anything.

Copy link
Author

Choose a reason for hiding this comment

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

@stof I want to know is this function is needed to be implemented in here grapheme and php85

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants