From a80d071a2ccf1e96ea0392e20cd048a82c27a0dd Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 12 Jun 2019 01:57:17 +0100 Subject: [PATCH] resourceloader: Support 'versionCallback' for computed package files The use cases we've seen for using computed (or virtual) package files, involve expensive computations to expand and transform data for the client that we don't want to evaluate in full just to compute the module's version hash (e.g. in the StartupModule, where we need to do this for 1000s of modules). For such cases, the module can specify a 'versionCallback' of which the return value will be used to seed the module's version hash. The default remains the same as before, which is to use the full content to seed the version hash (via getDefinitionSummary). Bug: T223260 Change-Id: I76f573239e6bd429287e7adb33a92ffd5e260c20 --- .../ResourceLoaderFileModule.php | 62 ++++++++++-- .../ResourceLoaderFileModuleTest.php | 94 ++++++++++++++++++- 2 files changed, 144 insertions(+), 12 deletions(-) diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index 015c8285f1..87a45155b1 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/includes/resourceloader/ResourceLoaderFileModule.php @@ -619,9 +619,24 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { $options[$member] = $this->{$member}; } + $packageFiles = $this->expandPackageFiles( $context ); + if ( $packageFiles ) { + // Extract the minimum needed: + // - The 'main' pointer (included as-is). + // - The 'files' array, simplied to only which files exist (the keys of + // this array), and something that represents their non-file content. + // For packaged files that reflect files directly from disk, the + // 'getFileHashes' method tracks this already. + // It is important that the keys of the 'files' array are preserved, + // as they affect the module output. + $packageFiles['files'] = array_map( function ( $fileInfo ) { + return $fileInfo['definitionSummary'] ?? ( $fileInfo['content'] ?? null ); + }, $packageFiles['files'] ); + } + $summary[] = [ 'options' => $options, - 'packageFiles' => $this->expandPackageFiles( $context ), + 'packageFiles' => $packageFiles, 'fileHashes' => $this->getFileHashes( $context ), 'messageBlob' => $this->getMessageBlob( $context ), ]; @@ -1068,16 +1083,22 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { } /** - * Expand the packageFiles definition into something that's (almost) the right format for - * getPackageFiles() to return. This expands shorthands, resolves config vars and callbacks, - * but does not expand file paths or read the actual contents of files. Those things are done - * by getPackageFiles(). + * Internal helper for use by getPackageFiles(), getFileHashes() and getDefinitionSummary(). + * + * This expands the 'packageFiles' definition into something that's (almost) the right format + * for getPackageFiles() to return. It expands shorthands, resolves config vars, and handles + * summarising any non-file data for getVersionHash(). For file-based data, getFileHashes() + * handles it instead, which also ends up in getDefinitionSummary(). * - * This is split up in this way so that getFileHashes() can get a list of file names, and - * getDefinitionSummary() can get config vars and callback results in their expanded form. + * What it does not do is reading the actual contents of any specified files, nor invoking + * the computation callbacks. Those things are done by getPackageFiles() instead to improve + * backend performance by only doing this work when the module response is needed, and not + * when merely computing the version hash for StartupModule, or when checking + * If-None-Match headers for a HTTP 304 response. * * @param ResourceLoaderContext $context * @return array|null + * @throws MWException If the 'packageFiles' definition is invalid. */ private function expandPackageFiles( ResourceLoaderContext $context ) { $hash = $context->getHash(); @@ -1113,19 +1134,32 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { } } + // Perform expansions (except 'file' and 'callback'), creating one of these keys: + // - 'content': literal value. + // - 'filePath': content to be read from a file. + // - 'callback': content computed by a callable. if ( isset( $fileInfo['content'] ) ) { $expanded['content'] = $fileInfo['content']; } elseif ( isset( $fileInfo['file'] ) ) { $expanded['filePath'] = $fileInfo['file']; } elseif ( isset( $fileInfo['callback'] ) ) { - if ( is_callable( $fileInfo['callback'] ) ) { - $expanded['content'] = $fileInfo['callback']( $context ); - } else { + if ( !is_callable( $fileInfo['callback'] ) ) { $msg = __METHOD__ . ": invalid callback for package file \"{$fileInfo['name']}\"" . " in module \"{$this->getName()}\""; wfDebugLog( 'resourceloader', $msg ); throw new MWException( $msg ); } + if ( isset( $fileInfo['versionCallback'] ) ) { + if ( !is_callable( $fileInfo['versionCallback'] ) ) { + throw new MWException( __METHOD__ . ": invalid versionCallback for file" . + " \"{$fileInfo['name']}\" in module \"{$this->getName()}\"" ); + } + $expanded['definitionSummary'] = ( $fileInfo['versionCallback'] )( $context ); + // Don't invoke 'callback' here as it may be expensive (T223260). + $expanded['callback'] = $fileInfo['callback']; + } else { + $expanded['content'] = ( $fileInfo['callback'] )( $context ); + } } elseif ( isset( $fileInfo['config'] ) ) { if ( $type !== 'data' ) { $msg = __METHOD__ . ": invalid use of \"config\" for package file \"{$fileInfo['name']}\" " . @@ -1184,6 +1218,8 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { // Expand file contents foreach ( $expandedPackageFiles['files'] as &$fileInfo ) { + // Turn any 'filePath' or 'callback' key into actual 'content', + // and remove the key after that. if ( isset( $fileInfo['filePath'] ) ) { $localPath = $this->getLocalPath( $fileInfo['filePath'] ); if ( !file_exists( $localPath ) ) { @@ -1198,7 +1234,13 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { } $fileInfo['content'] = $content; unset( $fileInfo['filePath'] ); + } elseif ( isset( $fileInfo['callback'] ) ) { + $fileInfo['content'] = ( $fileInfo['callback'] )( $context ); + unset( $fileInfo['callback'] ); } + + // Not needed for client response, exists for getDefinitionSummary(). + unset( $fileInfo['definitionSummary'] ); } return $expandedPackageFiles; diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php index 2aa0d27f99..1585cbcef0 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php @@ -373,6 +373,68 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase { 'lessVars' => [ 'key' => 'value' ], ]; yield 'identical Less variables' => [ $x, $x, true ]; + + $a = [ + 'packageFiles' => [ [ 'name' => 'data.json', 'callback' => function () { + return [ 'aaa' ]; + } ] ] + ]; + $b = [ + 'packageFiles' => [ [ 'name' => 'data.json', 'callback' => function () { + return [ 'bbb' ]; + } ] ] + ]; + yield 'packageFiles with different callback' => [ $a, $b, false ]; + + $a = [ + 'packageFiles' => [ [ 'name' => 'aaa.json', 'callback' => function () { + return [ 'x' ]; + } ] ] + ]; + $b = [ + 'packageFiles' => [ [ 'name' => 'bbb.json', 'callback' => function () { + return [ 'x' ]; + } ] ] + ]; + yield 'packageFiles with different file name and a callback' => [ $a, $b, false ]; + + $a = [ + 'packageFiles' => [ [ 'name' => 'data.json', 'versionCallback' => function () { + return [ 'A-version' ]; + }, 'callback' => function () { + throw new Exception( 'Unexpected computation' ); + } ] ] + ]; + $b = [ + 'packageFiles' => [ [ 'name' => 'data.json', 'versionCallback' => function () { + return [ 'B-version' ]; + }, 'callback' => function () { + throw new Exception( 'Unexpected computation' ); + } ] ] + ]; + yield 'packageFiles with different versionCallback' => [ $a, $b, false ]; + + $a = [ + 'packageFiles' => [ [ 'name' => 'aaa.json', + 'versionCallback' => function () { + return [ 'X-version' ]; + }, + 'callback' => function () { + throw new Exception( 'Unexpected computation' ); + } + ] ] + ]; + $b = [ + 'packageFiles' => [ [ 'name' => 'bbb.json', + 'versionCallback' => function () { + return [ 'X-version' ]; + }, + 'callback' => function () { + throw new Exception( 'Unexpected computation' ); + } + ] ] + ]; + yield 'packageFiles with different file name and a versionCallback' => [ $a, $b, false ]; } /** @@ -471,7 +533,7 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase { 'main' => 'init.js' ] ], - [ + 'package file with callback' => [ $base + [ 'packageFiles' => [ [ 'name' => 'foo.json', 'content' => [ 'Hello' => 'world' ] ], @@ -518,6 +580,34 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase { 'lang' => 'fy' ] ], + 'package file with callback and versionCallback' => [ + $base + [ + 'packageFiles' => [ + [ 'name' => 'bar.js', 'content' => "console.log('Hello');" ], + [ 'name' => 'data.json', 'versionCallback' => function ( $context ) { + return $context->getLanguage(); + }, 'callback' => function ( $context ) { + return [ 'langCode' => $context->getLanguage() ]; + } ], + ] + ], + [ + 'files' => [ + 'bar.js' => [ + 'type' => 'script', + 'content' => "console.log('Hello');", + ], + 'data.json' => [ + 'type' => 'data', + 'content' => [ 'langCode' => 'fy' ] + ], + ], + 'main' => 'bar.js' + ], + [ + 'lang' => 'fy' + ] + ], [ $base + [ 'packageFiles' => [ @@ -526,7 +616,7 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase { ], false ], - [ + 'package file with invalid callback' => [ $base + [ 'packageFiles' => [ [ 'name' => 'foo.json', 'callback' => 'functionThatDoesNotExist142857' ] -- 2.20.1