From 7a2574eafbde5b188e7f22b7b00f648f5e15a2b6 Mon Sep 17 00:00:00 2001 From: Hugo Sales Date: Wed, 28 Jul 2021 21:16:18 +0000 Subject: [PATCH] [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 | 46 +++++++++++++++++--------------- src/Core/I18n/TransExtractor.php | 6 +++++ src/Util/TemporaryFile.php | 39 ++++++++++++++------------- tests/Core/I18n/I18nTest.php | 33 +++++++++++++++++++---- 4 files changed, 79 insertions(+), 45 deletions(-) diff --git a/src/Core/I18n/I18n.php b/src/Core/I18n/I18n.php index bee6ebce0d..14b88def3f 100644 --- a/src/Core/I18n/I18n.php +++ b/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 Symfony\Component\Translation\Exception\InvalidArgumentException; +use 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: - // Empty parameters, simple message - return I18n::$translator->trans($args[0], [], $domain); - case 3: - if (is_int($args[2])) { - throw new Exception('Calling `_m()` with an explicit number is deprecated, ' . - 'use an explicit parameter'); - } + case 1: + // Empty parameters, simple message + return I18n::$translator->trans($args[0], [], $domain); + case 3: + // @codeCoverageIgnoreStart + if (is_int($args[2])) { + 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: - throw new InvalidArgumentException('Bad parameters to `_m()`'); + default: + // @codeCoverageIgnoreStart + throw new InvalidArgumentException("Bad parameters to `_m()` for domain {$domain}"); + // @codeCoverageIgnoreEnd } } diff --git a/src/Core/I18n/TransExtractor.php b/src/Core/I18n/TransExtractor.php index ae74e249c3..e855849dc7 100644 --- a/src/Core/I18n/TransExtractor.php +++ b/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 { /** diff --git a/src/Util/TemporaryFile.php b/src/Util/TemporaryFile.php index c9bf64e31b..795163a46d 100644 --- a/src/Util/TemporaryFile.php +++ b/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; - $filename = uniqid(($options['directory'] ?? (sys_get_temp_dir() . '/')) . ($options['prefix'] ?? 'gs-php')) . ($options['suffix'] ?? ''); + // todo options permission + $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) { - // @codeCoverageIgnoreStart + if ($this->resource === false) { $this->cleanup(); - throw new TemporaryFileException('Could not open file: ' . $filename); - // @codeCoverageIgnoreEnd + throw new TemporaryFileException('Could not open file: ' . $filepath); } - 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(); - $this->close(); - if (file_exists($path)) { - @unlink($path); + if ($this->resource !== false) { + $path = $this->getRealPath(); + $this->close(); + 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) { - if (!$exists) { + if (!$renamed || !$chmoded) { + if (!$existed && file_exists($destpath)) { // If the file wasn't there, clean it up in case of a later failure unlink($destpath); } diff --git a/tests/Core/I18n/I18nTest.php b/tests/Core/I18n/I18nTest.php index 3bfa0eb033..59d859c3a5 100644 --- a/tests/Core/I18n/I18nTest.php +++ b/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, - // function () use ($pronouns) { _m($pronouns, ['pronoun' => 'unknown']); }); + static::assertSame('their apple', _m($pronouns, ['pronoun' => 'unkown'])); // a bit odd, not sure if we want this $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')); } }