I've done some fuzzy tests:

I have a Very Bad News, and a Good news.

🔶 The Good news:

Here we are just checking the $to_encoding. I think that is 98% OK. Probably to be 100% OK we should also check the $from_encoding but that would slow down things without any clear indication of bad usage around. So I would say: just check "$to", and, on the next reported crash, let's also eventually fix the "from". Premising that usually the "from" should be already assumed as meaningful and usually not user-provided, so I do not expect a crash there, so I do not expect changes.

→ So I would propose, let's keep your current "just check the 'from'" logic.

🔴 Bad news:

PHP is really more evil than thought: the damn mb_list_encodings() just returns few possible encodings. To verify if an encoding exists, we need to also check against $aliases = mb_encoding_aliases($existing).

https://www.php.net/manual/en/function.mb-encoding-aliases.php

So our nightmare is similar to the following one (that also needs strtolower stuff):

function is_existing_encoding($encoding) {
  foreach(mb_list_encodings() as $candidate) {
    // check against the canonical version
    if($candidate === $encoding) {
      return true;
    }

    // also check against any related alias
    $aliases = mb_encoding_aliases($candidate);
    if(in_array($encoding, $aliases, true)) {
      return true;
    }
  }
  return false;
}

With strtolower() extra normalization it becomes something like:

function is_existing_encoding($encoding) {
  $encoding = strtolower($encoding);

  foreach(mb_list_encodings() as $candidate) {
    // check against the canonical version
    if(strtolower($candidate) === $encoding) {
      return true;
    }

    // also check against any related alias
    $aliases = mb_encoding_aliases($candidate);
    foreach($aliases as $alias_candidate) {
      // check against the canonical version
      if(strtolower($alias_candidate) === $encoding) {
        return true;
      }
    }
  }

  return false;
}

This is becoming really overkill. Probably this deserves a dedicated function.

Thinking deeply about this, probably we can try a... try catch structure. That would do the job, without introducing terrible giant overhead (like in my terrible code proposal)

In D25249#7806, @valerio.bozzolan wrote:

Thinking deeply about this, probably we can try a... try catch structure. That would do the job, without introducing terrible giant overhead (like in my terrible code proposal)

Thanks for this change but just to clarify this comment, I was just suggesting something like this:

try {
  $result = mb_convert_encoding($string, $to_encoding, $from_encoding);
} catch(Throwable $e) {
  $message = $e->getMessage();
}

(Untested) or something like that, since the function mb_convert_encoding() seems to already have all the necessary validity checks, and probably we do not need to rewrite that business logic: we can just catch its errors.

I suggest that, also because this is supposed to be a low-level phutil utility and probably should be as fast and simple as possible

I suggest that, also because this is supposed to be a low-level phutil utility and probably should be as fast and simple as possible

Indeed, that works and is way more simple.

Uh! Very good news! I tried this also in PHP 5.5 using:

$ cd arcanist
$ docker run -it --rm --name my-running-script -v "$PWD":/usr/src/myapp -w /usr/src/myapp php:5.5-cli php -a
$ require 'support/init/init-script.php';
$ var_dump(phutil_utf8_convert('asd', 'lol', 'dsa'));

And SURPRISINGLY this still works in PHP 5.5 and still generates an exception! 💌 I was very skeptical since only PHP >= 8.0 generates a ValueError, and just an E_WARNING before.

But.. it works because Arcanist elevates warnings to exceptions! We hate this because of PHP 8.1 warnings were massive, but, here that is useful! ✨

I'm 99% sure that this works. But let's see if anybody else is OK with this. Just to have more eyes on this.

Something like this, though I'm not sure if this is actually correct - in PHP8 using @ will still throw a ValueError if passed an invalid encoding. But I'm not sure This @ thing is working as expected in php8, or I don't have a valid test case.

$message = null;
try {
  $result = @mb_convert_encoding($string, $to_encoding, $from_encoding);

  if ($result === false) {
    $message = error_get_last();
    if ($message) {
      $message = idx($message, 'message', pht('Unknown error.'));
    }
  }
} catch (Throwable $ex) {
  $message = $ex->getMessage();
}

if ($message !== null) {
  throw new Exception(...
}

In D25249#8543, @speck wrote:

The reason that @ is used instead of try/catch -- the comment on the function is saying if you have a string in e.g. Latin-1 and $to_encoding is set to something different-yet-valid such as Korean (e.g. ISO-2022-KR) -- then the encoding conversion will silently fail, not throwing an exception, but still populating the result with incorrect garbage.

My interpretation of

This function assumes that the input is in the given source encoding. [...] We can detect failures caused by invalid encoding names, but mb_convert_encoding() fails silently if the encoding name identifies a real encoding but the string is not actually encoded with that encoding.

is slightly different. If $from_encoding doesn't match the actual encoding used for the string, then mb_convert_encoding() will produce garbage, but there will be no way at all to determine that this happened. In particular, using @ will have no impact.

In D25249#8543, @speck wrote:

$test = @mb_convert_encoding('😄', 'UTF8', 'LATIN1');
$message = error_get_last();

Will show that $message['message'] is non-null.

I can't reproduce this with any version of PHP (5.6, 7.4, 8.0, 8.1, or 8.2). With or without the @ (and even if the encodings are swapped), it reports no errors and throws no exceptions, because both encodings are valid.

In D25249#8543, @speck wrote:

In testing on PHP 8.2 I think the solution is to keep the existing code to use @ and error_get_last() but to try/catch around it in order to capture invalid encodings

As far as I can tell, the only reason to keep the @ in addition to the try/catch would be to support both PHP <8 and PHP >=8. Prior to PHP 8, mb_convert_encoding() used to emit E_WARNING for invalid encodings, which can be handled using @ and error_get_last(). Starting from PHP 8, it throws a ValueError for invalid encodings, which must be caught instead. I think that's consistent with this observation:

In D25249#8546, @speck wrote:

in PHP8 using @ will still throw a ValueError if passed an invalid encoding

Despite the API change for mb_convert_encoding() in PHP 8, we might not need to include both versions of error handling:

In D25249#8505, @valerio.bozzolan wrote:

And SURPRISINGLY this still works in PHP 5.5 and still generates an exception! [...] it works because Arcanist elevates warnings to exceptions!

This suggests that there's actually no need for @/error_get_last() here, even with old PHP. However, does the generated exception still have the "String conversion from encoding" part of the message, or does it only include the "Unknown encoding" part? If the latter, do we care? Is it fine to rely on the elevation of warnings to exceptions to deal with this, or should it still be handled explicitly in case that policy changes?

The use of error_get_last() in this very code has been unhelpful and misleading to me as it led to displaying messages entirely unrelated to invalid encodings (e.g. about libxml_disable_entity_loader having been deprecated).