From 120ef51cbf76afaf1195f87f74c1b04f8dcc4749 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 18 Nov 2021 17:09:48 -0800 Subject: [PATCH] SettingsBuilder: Add YAML file format. If php-yaml extension in installed, use that. Otherwise we fallback to symfony Yaml parser. php-yaml is about 20 times faster then symfony, for default-settings.yaml it will take PHP-yaml 6ms to load it vs 100ms for symfony. But given that the result will be cached, it's better not to bring in a required native dependency. Bug: T294751 Change-Id: I3ffde926c3f264cacf39810ff7bd338c9f78823d --- composer.json | 3 +- includes/Settings/Source/FileSource.php | 71 +++++------ .../Settings/Source/Format/JsonFormat.php | 2 +- .../Settings/Source/Format/SettingsFormat.php | 2 +- .../Settings/Source/Format/YamlFormat.php | 119 ++++++++++++++++++ .../Settings/Source/Format/JsonFormatTest.php | 18 +-- .../Settings/Source/Format/YamlFormatTest.php | 95 ++++++++++++++ 7 files changed, 259 insertions(+), 51 deletions(-) create mode 100644 includes/Settings/Source/Format/YamlFormat.php create mode 100644 tests/phpunit/unit/includes/Settings/Source/Format/YamlFormatTest.php diff --git a/composer.json b/composer.json index 84e7cf48d1c..07dcb10591c 100644 --- a/composer.json +++ b/composer.json @@ -45,6 +45,7 @@ "psr/log": "1.1.4", "ralouphie/getallheaders": "3.0.3", "symfony/polyfill-php80": "1.23.1", + "symfony/yaml": "5.3.6", "wikimedia/assert": "0.5.0", "wikimedia/at-ease": "2.1.0", "wikimedia/base-convert": "2.0.1", @@ -93,7 +94,6 @@ "phpunit/phpunit": "^8.5", "psy/psysh": "^0.10.5", "seld/jsonlint": "1.8.3", - "symfony/yaml": "~3.4|~5.1", "wikimedia/testing-access-wrapper": "~2.0", "wmde/hamcrest-html-matchers": "^1.0.0" }, @@ -104,6 +104,7 @@ "suggest": { "ext-apcu": "Local data cache for greatly improved performance", "ext-curl": "Improved http communication abilities", + "ext-yaml": "Improved YAML parsing performance", "ext-openssl": "Cryptographical functions", "ext-wikidiff2": "Diff accelerator", "monolog/monolog": "Flexible debug logging system", diff --git a/includes/Settings/Source/FileSource.php b/includes/Settings/Source/FileSource.php index 2320f377049..16f806dd677 100644 --- a/includes/Settings/Source/FileSource.php +++ b/includes/Settings/Source/FileSource.php @@ -5,6 +5,7 @@ namespace MediaWiki\Settings\Source; use MediaWiki\Settings\SettingsBuilderException; use MediaWiki\Settings\Source\Format\JsonFormat; use MediaWiki\Settings\Source\Format\SettingsFormat; +use MediaWiki\Settings\Source\Format\YamlFormat; use UnexpectedValueException; use Wikimedia\AtEase\AtEase; @@ -14,17 +15,18 @@ use Wikimedia\AtEase\AtEase; * @since 1.38 */ class FileSource implements SettingsSource { - /** - * Default format with which to attempt decoding if none are given to the - * constructor. - */ - private const DEFAULT_FORMAT = JsonFormat::class; + + private const BUILT_IN_FORMATS = [ + JsonFormat::class, + YamlFormat::class, + ]; /** - * Possible formats. - * @var array + * Format to use for reading the file, if given. + * + * @var ?SettingsFormat */ - private $formats; + private $format; /** * Path to local file. @@ -33,45 +35,37 @@ class FileSource implements SettingsSource { private $path; /** - * Constructs a new FileSource for the given path and possible matching - * formats. The first format to match the path's file extension will be - * used to decode the content. + * Constructs a new FileSource for the given path and possibly a custom format + * to decode the contents. If no format is given, the built-in formats will be + * tried and the first one that supports the file extension will be used. * - * An end-user caller may be explicit about the given path's format by - * providing only one format. + * Built-in formats: + * - JsonFormat + * - YamlFormat * * * load(); * * - * While a generalized caller may want to pass a number of supported - * formats. + * While a specialized caller may want to pass a specialized format * * * load(); * * * @param string $path - * @param SettingsFormat ...$formats + * @param SettingsFormat|null $format */ - public function __construct( string $path, SettingsFormat ...$formats ) { + public function __construct( string $path, SettingsFormat $format = null ) { $this->path = $path; - $this->formats = $formats; - - if ( empty( $this->formats ) ) { - $class = self::DEFAULT_FORMAT; - $this->formats = [ new $class() ]; - } + $this->format = $format; } /** @@ -86,20 +80,19 @@ class FileSource implements SettingsSource { // If there's only one format, don't bother to match the file // extension. - if ( count( $this->formats ) == 1 ) { - return $this->readAndDecode( $this->formats[0] ); + if ( $this->format ) { + return $this->readAndDecode( $this->format ); } - foreach ( $this->formats as $format ) { - if ( $format->supportsFileExtension( $ext ) ) { - return $this->readAndDecode( $format ); + foreach ( self::BUILT_IN_FORMATS as $format ) { + if ( call_user_func( [ $format, 'supportsFileExtension' ], $ext ) ) { + return $this->readAndDecode( new $format() ); } } throw new SettingsBuilderException( - "None of the given formats ({formats}) are suitable for '{path}'", + "None of the built-in formats are suitable for '{path}'", [ - 'formats' => implode( ', ', $this->formats ), 'path' => $this->path, ] ); diff --git a/includes/Settings/Source/Format/JsonFormat.php b/includes/Settings/Source/Format/JsonFormat.php index 1151ab93a2c..ec9fd15d0b7 100644 --- a/includes/Settings/Source/Format/JsonFormat.php +++ b/includes/Settings/Source/Format/JsonFormat.php @@ -42,7 +42,7 @@ class JsonFormat implements SettingsFormat { * * @return bool */ - public function supportsFileExtension( string $ext ): bool { + public static function supportsFileExtension( string $ext ): bool { return strtolower( $ext ) == 'json'; } diff --git a/includes/Settings/Source/Format/SettingsFormat.php b/includes/Settings/Source/Format/SettingsFormat.php index 943faa88d54..8b7f298f0a6 100644 --- a/includes/Settings/Source/Format/SettingsFormat.php +++ b/includes/Settings/Source/Format/SettingsFormat.php @@ -31,5 +31,5 @@ interface SettingsFormat extends Stringable { * * @return bool */ - public function supportsFileExtension( string $ext ): bool; + public static function supportsFileExtension( string $ext ): bool; } diff --git a/includes/Settings/Source/Format/YamlFormat.php b/includes/Settings/Source/Format/YamlFormat.php new file mode 100644 index 00000000000..47254475d50 --- /dev/null +++ b/includes/Settings/Source/Format/YamlFormat.php @@ -0,0 +1,119 @@ +useParsers = $useParsers; + } + + public function decode( string $data ): array { + foreach ( $this->useParsers as $parser ) { + if ( self::isParserAvailable( $parser ) ) { + return $this->parseWith( $parser, $data ); + } + } + throw new LogicException( 'No parser available' ); + } + + /** + * Check whether a specific YAML parser is available. + * + * @param string $parser one of the PARSER_* constants. + * @return bool + */ + public static function isParserAvailable( string $parser ): bool { + switch ( $parser ) { + case self::PARSER_PHP_YAML: + return function_exists( 'yaml_parse' ); + case self::PARSER_SYMFONY: + return true; + default: + throw new LogicException( 'Unknown parser: ' . $parser ); + } + } + + /** + * @param string $parser + * @param string $data + * @return array + */ + private function parseWith( string $parser, string $data ): array { + switch ( $parser ) { + case self::PARSER_PHP_YAML: + return $this->parseWithPhp( $data ); + case self::PARSER_SYMFONY: + return $this->parseWithSymfony( $data ); + default: + throw new LogicException( 'Unknown parser: ' . $parser ); + } + } + + private function parseWithPhp( string $data ): array { + $previousValue = ini_set( 'yaml.decode_php', false ); + try { + $ndocs = 0; + $result = AtEase::quietCall( + 'yaml_parse', + $data, + 0, + $ndocs, + [ + /** + * Crash if provided YAML has PHP constants in it. + * We do not want to support that. + * + * @return never + */ + '!php/const' => static function () { + throw new UnexpectedValueException( + 'PHP constants are not supported' + ); + }, + ] + ); + if ( $result === false ) { + throw new UnexpectedValueException( 'Failed to parse YAML' ); + } + return $result; + } finally { + ini_set( 'yaml.decode_php', $previousValue ); + } + } + + private function parseWithSymfony( string $data ): array { + try { + return Yaml::parse( $data, Yaml::PARSE_EXCEPTION_ON_INVALID_TYPE ); + } catch ( ParseException $e ) { + throw new UnexpectedValueException( + 'Failed to parse YAML ' . $e->getMessage() + ); + } + } + + public static function supportsFileExtension( string $ext ): bool { + $ext = strtolower( $ext ); + return $ext === 'yml' || $ext === 'yaml'; + } + + public function __toString() { + return 'YAML'; + } +} diff --git a/tests/phpunit/unit/includes/Settings/Source/Format/JsonFormatTest.php b/tests/phpunit/unit/includes/Settings/Source/Format/JsonFormatTest.php index 15f20942945..1a44bfd81dd 100644 --- a/tests/phpunit/unit/includes/Settings/Source/Format/JsonFormatTest.php +++ b/tests/phpunit/unit/includes/Settings/Source/Format/JsonFormatTest.php @@ -26,16 +26,16 @@ class JsonFormatTest extends TestCase { $format->decode( '{ bad }' ); } - public function testSupportsFileExtension() { - $format = new JsonFormat(); - - $this->assertTrue( $format->supportsFileExtension( 'json' ) ); - $this->assertTrue( $format->supportsFileExtension( 'JSON' ) ); + public function provideSupportsFileExtension() { + yield 'Supported' => [ 'json', true ]; + yield 'Supported, uppercase' => [ 'JSON', true ]; + yield 'Unsupported' => [ 'txt', false ]; } - public function testSupportsFileExtensionUnsupported() { - $format = new JsonFormat(); - - $this->assertFalse( $format->supportsFileExtension( 'yaml' ) ); + /** + * @dataProvider provideSupportsFileExtension + */ + public function testSupportsFileExtension( $extension, $expected ) { + $this->assertSame( $expected, JsonFormat::supportsFileExtension( $extension ) ); } } diff --git a/tests/phpunit/unit/includes/Settings/Source/Format/YamlFormatTest.php b/tests/phpunit/unit/includes/Settings/Source/Format/YamlFormatTest.php new file mode 100644 index 00000000000..953f3800663 --- /dev/null +++ b/tests/phpunit/unit/includes/Settings/Source/Format/YamlFormatTest.php @@ -0,0 +1,95 @@ + [ 'parser' => YamlFormat::PARSER_PHP_YAML ]; + yield 'symfony' => [ 'parser' => YamlFormat::PARSER_SYMFONY ]; + } + + /** + * @dataProvider provideParser + */ + public function testDecode( string $parser ) { + if ( !YamlFormat::isParserAvailable( $parser ) ) { + $this->markTestSkipped( "Parser '$parser' is not available" ); + } + + $format = new YamlFormat( [ $parser ] ); + $this->assertEquals( + [ 'config-schema' => [ 'MySetting' => [ 'type' => 'boolean' ] ] ], + $format->decode( self::VALID_YAML ) + ); + } + + /** + * @dataProvider provideParser + */ + public function testDecodeInvalid( string $parser ) { + if ( !YamlFormat::isParserAvailable( $parser ) ) { + $this->markTestSkipped( "Parser '$parser' is not available" ); + } + + $format = new YamlFormat( [ $parser ] ); + $this->expectException( UnexpectedValueException::class ); + $format->decode( self::INVALID_YAML ); + } + + /** + * @dataProvider provideParser + */ + public function testDecodeDoesNotUnserializeObjects( string $parser ) { + if ( !YamlFormat::isParserAvailable( $parser ) ) { + $this->markTestSkipped( "Parser '$parser' is not available" ); + } + + $format = new YamlFormat( [ $parser ] ); + $this->expectException( UnexpectedValueException::class ); + $format->decode( 'object: !php/object "' . serialize( $format ) . '"' ); + } + + /** + * @dataProvider provideParser + */ + public function testDecodeDoesNotRespectPHPConst( string $parser ) { + if ( !YamlFormat::isParserAvailable( $parser ) ) { + $this->markTestSkipped( "Parser '$parser' is not available" ); + } + + $format = new YamlFormat( [ $parser ] ); + $this->expectException( UnexpectedValueException::class ); + $format->decode( '{ bar: !php/const PHP_INT_SIZE }' ); + } + + public function provideSupportsFileExtension() { + yield 'Supported' => [ 'yaml', true ]; + yield 'Supported, uppercase' => [ 'YAML', true ]; + yield 'Supported, short' => [ 'yml', true ]; + yield 'Unsupported' => [ 'txt', false ]; + } + + /** + * @dataProvider provideSupportsFileExtension + */ + public function testSupportsFileExtension( $extension, $expected ) { + $this->assertSame( $expected, YamlFormat::supportsFileExtension( $extension ) ); + } +}