[CORE][TemporaryFile] Add option to specify attempts and better handle when reaching the attemp limit without being able to create a file

src/Core/I18n/I18n.php

@@ -36,11 +36,9 @@
 
 namespace App\Core\I18n;
 
-use App\Util\Common;
 use App\Util\Exception\ServerException;
 use App\Util\Formatting;
-use Exception;
+use InvalidArgumentException;
-use Symfony\Component\Translation\Exception\InvalidArgumentException;
 use Symfony\Contracts\Translation\TranslatorInterface;
 
 // Locale category constants are usually predefined, but may not be
@@ -100,19 +98,19 @@ abstract class I18n
     }
 
     /**
-     * Content negotiation for language codes
+     * Content negotiation for language codes. Gets our highest rated translation language that the client accepts
      *
      * @param string $http_accept_lang_header HTTP Accept-Language header
      *
      * @return string language code for best language match, false otherwise
      */
-    public static function clientPreferredLanguage(string $http_accept_lang_header): string
+    public static function clientPreferredLanguage(string $http_accept_lang_header): string | bool
     {
         $client_langs  = [];
-        $all_languages = Common::config('site', 'languages');
+        $all_languages = self::getAllLanguages();
 
         preg_match_all('"(((\S\S)-?(\S\S)?)(;q=([0-9.]+))?)\s*(,\s*|$)"',
-            strtolower($http_accept_lang_header), $http_langs);
+                       mb_strtolower($http_accept_lang_header), $http_langs);
 
         for ($i = 0; $i < count($http_langs); ++$i) {
             if (!empty($http_langs[2][$i])) {
@@ -143,7 +141,7 @@ abstract class I18n
     public static function getNiceLanguageList(): array
     {
         $nice_lang     = [];
-        $all_languages = Common::config('site', 'languages');
+        $all_languages = self::getAllLanguages();
 
         foreach ($all_languages as $lang) {
             $nice_lang[$lang['lang']] = $lang['name'];
@@ -158,9 +156,9 @@ abstract class I18n
      *
      * @return bool true if language is rtl
      */
-    public static function isRtl(string $lang_value): bool
+    public static function isRTL(string $lang_value): bool
     {
-        foreach (Common::config('site', 'languages') as $code => $info) {
+        foreach (self::getAllLanguages() as $code => $info) {
             if ($lang_value == $info['lang']) {
                 return $info['direction'] == 'rtl';
             }
@@ -263,7 +261,7 @@ abstract class I18n
                 $pref = '';
                 $op   = 'select';
             } else {
-                throw new ServerException('Invalid variable type. (int|string) only');
+                throw new InvalidArgumentException('Invalid variable type. (int|string) only');
             }
 
             $res = "{$var}, {$op}, ";
@@ -281,7 +279,7 @@ abstract class I18n
                 } elseif (is_string($m)) {
                     $res .= " {{$m}} ";
                 } else {
-                    throw new Exception('Invalid message array');
+                    throw new InvalidArgumentException('Invalid message array');
                 }
                 ++$i;
             }
@@ -320,14 +318,16 @@ function _m(...$args): string
     // and only 2 frames (this and previous)
     $domain = I18n::_mdomain(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS)[0]['file'], 2);
     switch (count($args)) {
-        case 1:
+    case 1:
-            // Empty parameters, simple message
+        // Empty parameters, simple message
-            return I18n::$translator->trans($args[0], [], $domain);
+        return I18n::$translator->trans($args[0], [], $domain);
-        case 3:
+    case 3:
-            if (is_int($args[2])) {
+        // @codeCoverageIgnoreStart
-                throw new Exception('Calling `_m()` with an explicit number is deprecated, ' .
+        if (is_int($args[2])) {
-                    'use an explicit parameter');
+            throw new InvalidArgumentException('Calling `_m()` with a number for pluralization is deprecated, ' .
-            }
+                                                'use an explicit parameter');
+        }
+        // @codeCoverageIgnoreEnd
         // Falthrough
         // no break
         case 2:
@@ -342,7 +342,9 @@ function _m(...$args): string
             }
         // Fallthrough
         // no break
-        default:
+    default:
-            throw new InvalidArgumentException('Bad parameters to `_m()`');
+        // @codeCoverageIgnoreStart
+        throw new InvalidArgumentException("Bad parameters to `_m()` for domain {$domain}");
+        // @codeCoverageIgnoreEnd
     }
 }

src/Core/I18n/TransExtractor.php

@@ -45,6 +45,12 @@ use Symfony\Component\Translation\Extractor\ExtractorInterface;
 use Symfony\Component\Translation\Extractor\PhpStringTokenParser;
 use Symfony\Component\Translation\MessageCatalogue;
 
+/**
+ * Since this happens outside the normal request life-cycle (through a
+ * command, usually), it unfeasible to test this
+ *
+ * @codeCoverageIgnore
+ */
 class TransExtractor extends AbstractFileExtractor implements ExtractorInterface
 {
     /**

src/Util/TemporaryFile.php

@@ -35,36 +35,37 @@ use Symfony\Component\Mime\MimeTypes;
  */
 class TemporaryFile extends \SplFileInfo
 {
+    // Cannot type annotate currently. `resource` is the expected type, but it's not a builtin type
     protected $resource;
 
     /**
-     * @param array $options - ['prefix' => ?string, 'suffix' => ?string, 'mode' => ?string, 'directory' => ?string]
+     * @param array $options - ['prefix' => ?string, 'suffix' => ?string, 'mode' => ?string, 'directory' => ?string, 'attempts' => ?int]
      *                       Description of options:
      *                       > prefix: The file name will begin with that prefix, default is 'gs-php'
      *                       > suffix: The file name will end with that suffix, default is ''
      *                       > mode: Operation mode, default is 'w+b'
      *                       > directory: Directory where the file will be used, default is the system's temporary
+     *                       > attempts: Default 16, how many times to attempt to find a unique file
      *
      * @throws TemporaryFileException
      */
     public function __construct(array $options = [])
     {
-        $attempts = 16;
+        // todo options permission
-        $filename = uniqid(($options['directory'] ?? (sys_get_temp_dir() . '/')) . ($options['prefix'] ?? 'gs-php')) . ($options['suffix'] ?? '');
+        $attempts = $options['attempts'] ?? 16;
+        $filepath = uniqid(($options['directory'] ?? (sys_get_temp_dir() . '/')) . ($options['prefix'] ?? 'gs-php')) . ($options['suffix'] ?? '');
         for ($count = 0; $count < $attempts; ++$count) {
-            $this->resource = @fopen($filename, $options['mode'] ?? 'w+b');
+            $this->resource = @fopen($filepath, $options['mode'] ?? 'w+b');
             if ($this->resource !== false) {
                 break;
             }
         }
-        if ($count == $attempts && $this->resource !== false) {
+        if ($this->resource === false) {
-            // @codeCoverageIgnoreStart
             $this->cleanup();
-            throw new TemporaryFileException('Could not open file: ' . $filename);
+            throw new TemporaryFileException('Could not open file: ' . $filepath);
-            // @codeCoverageIgnoreEnd
         }
 
-        parent::__construct($filename);
+        parent::__construct($filepath);
     }
 
     public function __destruct()
@@ -99,7 +100,7 @@ class TemporaryFile extends \SplFileInfo
     protected function close(): bool
     {
         $ret = true;
-        if (!is_null($this->resource)) {
+        if (!is_null($this->resource) && $this->resource !== false) {
             $ret = fclose($this->resource);
         }
         if ($ret) {
@@ -115,10 +116,12 @@ class TemporaryFile extends \SplFileInfo
      */
     protected function cleanup(): void
     {
-        $path = $this->getRealPath();
+        if ($this->resource !== false) {
-        $this->close();
+            $path = $this->getRealPath();
-        if (file_exists($path)) {
+            $this->close();
-            @unlink($path);
+            if (file_exists($path)) {
+                @unlink($path);
+            }
         }
     }
 
@@ -182,7 +185,7 @@ class TemporaryFile extends \SplFileInfo
         }
 
         // Memorise if the file was there and see if there is access
-        $exists = file_exists($destpath);
+        $existed = file_exists($destpath);
 
         if (!$this->close()) {
             // @codeCoverageIgnoreStart
@@ -192,10 +195,10 @@ class TemporaryFile extends \SplFileInfo
 
         set_error_handler(function ($type, $msg) use (&$error) { $error = $msg; });
         $renamed = rename($this->getPathname(), $destpath);
+        $chmoded = chmod($destpath, $filemode);
         restore_error_handler();
-        chmod($destpath, $filemode);
+        if (!$renamed || !$chmoded) {
-        if (!$renamed) {
+            if (!$existed && file_exists($destpath)) {
-            if (!$exists) {
                 // If the file wasn't there, clean it up in case of a later failure
                 unlink($destpath);
             }

tests/Core/I18n/I18nTest.php

@@ -21,13 +21,12 @@ namespace App\Tests\Core\I18n;
 
 use function App\Core\I18n\_m;
 use App\Core\I18n\I18n;
+use Jchook\AssertThrows\AssertThrows;
 use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
 
-// use Jchook\AssertThrows\AssertThrows;
-
 class I18nTest extends KernelTestCase
 {
-    // use AssertThrows;
+    use AssertThrows;
 
     public function testM()
     {
@@ -62,8 +61,7 @@ class I18nTest extends KernelTestCase
         static::assertSame('her apple',   _m($pronouns, ['pronoun' => 'she']));
         static::assertSame('his apple',   _m($pronouns, ['pronoun' => 'he']));
         static::assertSame('their apple', _m($pronouns, ['pronoun' => 'they']));
-        // $this->assertThrows(\Exception::class,
+        static::assertSame('their apple', _m($pronouns, ['pronoun' => 'unkown'])); // a bit odd, not sure if we want this
-        //                     function () use ($pronouns) { _m($pronouns, ['pronoun' => 'unknown']); });
 
         $pronouns = ['she' => 'her apple', 'he' => 'his apple', 'they' => 'their apple', 'someone\'s apple'];
         static::assertSame('someone\'s apple', _m($pronouns, ['pronoun' => 'unknown']));
@@ -92,5 +90,30 @@ class I18nTest extends KernelTestCase
         static::assertSame('her 42 apples',  _m($complex, ['pronoun' => 'she',  'count' => 42]));
         static::assertSame('their apple',    _m($complex, ['pronoun' => 'they', 'count' => 1]));
         static::assertSame('their 3 apples', _m($complex, ['pronoun' => 'they', 'count' => 3]));
+
+        static::assertThrows(\InvalidArgumentException::class, fn () => _m($apples, ['count' => []]));
+        static::assertThrows(\InvalidArgumentException::class, fn () => _m([1], ['foo' => 'bar']));
+    }
+
+    public function testIsRTL()
+    {
+        static::assertFalse(I18n::isRTL('af'));
+        static::assertTrue(I18n::isRTL('ar'));
+        static::assertThrows(\InvalidArgumentException::class, fn () => I18n::isRTL(''));
+        static::assertThrows(\InvalidArgumentException::class, fn () => I18n::isRTL('not a language'));
+    }
+
+    public function testGetNiceList()
+    {
+        static::assertIsArray(I18n::getNiceLanguageList());
+    }
+
+    public function testClientPreferredLanguage()
+    {
+        static::assertSame('fr', I18n::clientPreferredLanguage('Accept-Language: fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5'));
+        static::assertSame('de', I18n::clientPreferredLanguage('Accept-Language: de'));
+        static::assertSame('de', I18n::clientPreferredLanguage('Accept-Language: de-CH'));
+        static::assertSame('en', I18n::clientPreferredLanguage('Accept-Language: en-US,en;q=0.5'));
+        static::assertFalse(I18n::clientPreferredLanguage('Accept-Language: some-language'));
     }
 }