From dbb1fa12738e4d30ed22deffa0b2dd921f96af04 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 10 Jan 2023 01:06:26 +0100 Subject: [PATCH 1/5] fix(OC/Template): Remove usage of deprecated functions in `JSResourceLocator` Move from `\OC_App::getAppPath` to `IAppManager::getAppPath`. Signed-off-by: Ferdinand Thiessen --- lib/private/Template/JSResourceLocator.php | 30 ++++++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/lib/private/Template/JSResourceLocator.php b/lib/private/Template/JSResourceLocator.php index 7648c7953f3..0745451bbd5 100644 --- a/lib/private/Template/JSResourceLocator.php +++ b/lib/private/Template/JSResourceLocator.php @@ -27,16 +27,22 @@ */ namespace OC\Template; +use OCP\App\AppPathNotFoundException; use Psr\Log\LoggerInterface; +use \OCP\App\IAppManager; class JSResourceLocator extends ResourceLocator { /** @var JSCombiner */ protected $jsCombiner; - public function __construct(LoggerInterface $logger, JSCombiner $JSCombiner) { + /** @var IAppManager */ + protected $appManager; + + public function __construct(LoggerInterface $logger, JSCombiner $JSCombiner, IAppManager $appManager) { parent::__construct($logger); $this->jsCombiner = $JSCombiner; + $this->appManager = $appManager; } /** @@ -80,14 +86,23 @@ class JSResourceLocator extends ResourceLocator { } $script = substr($script, strpos($script, '/') + 1); - $app_path = \OC_App::getAppPath($app); - $app_url = \OC_App::getAppWebPath($app); + $app_path = false; + $app_url = false; - if ($app_path !== false) { + try { + $app_path = $this->appManager->getAppPath($app); // Account for the possibility of having symlinks in app path. Only // do this if $app_path is set, because an empty argument to realpath // gets turned into cwd. $app_path = realpath($app_path); + } catch (AppPathNotFoundException) { + // pass + } + + try { + $app_url = $this->appManager->getAppWebPath($app); + } catch (AppPathNotFoundException) { + // pass } // missing translations files fill be ignored @@ -122,7 +137,12 @@ class JSResourceLocator extends ResourceLocator { } else { // Add all the files from the json $files = $this->jsCombiner->getContent($root, $file); - $app_url = \OC_App::getAppWebPath($app); + $app_url = null; + try { + $app_url = $this->appManager->getAppWebPath($app); + } catch (AppPathNotFoundException) { + // pass + } foreach ($files as $jsFile) { $this->append($root, $jsFile, $app_url); From 00e041b90704b20a37e2e13b8842d46d616d685d Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 10 Jan 2023 01:11:34 +0100 Subject: [PATCH 2/5] feat(template): Allow `JSResourceLocator` to find ES6 scripts Enable module js (ES6) support on the `JSResourceLocator`. This changes `JSResourceLocator` to look for `.mjs` files first to allow applications to provide a fallback `.js` for older Nextcloud versions. Signed-off-by: Ferdinand Thiessen --- lib/private/Template/JSResourceLocator.php | 45 +++++++----- tests/lib/Template/JSResourceLocatorTest.php | 76 ++++++++++++++++---- 2 files changed, 92 insertions(+), 29 deletions(-) diff --git a/lib/private/Template/JSResourceLocator.php b/lib/private/Template/JSResourceLocator.php index 0745451bbd5..4b41e51ae14 100644 --- a/lib/private/Template/JSResourceLocator.php +++ b/lib/private/Template/JSResourceLocator.php @@ -59,27 +59,27 @@ class JSResourceLocator extends ResourceLocator { // For language files we try to load them all, so themes can overwrite // single l10n strings without having to translate all of them. $found = 0; - $found += $this->appendIfExist($this->serverroot, 'core/'.$script.'.js'); - $found += $this->appendIfExist($this->serverroot, $theme_dir.'core/'.$script.'.js'); - $found += $this->appendIfExist($this->serverroot, $script.'.js'); - $found += $this->appendIfExist($this->serverroot, $theme_dir.$script.'.js'); - $found += $this->appendIfExist($this->serverroot, 'apps/'.$script.'.js'); - $found += $this->appendIfExist($this->serverroot, $theme_dir.'apps/'.$script.'.js'); + $found += $this->appendScriptIfExist($this->serverroot, 'core/'.$script); + $found += $this->appendScriptIfExist($this->serverroot, $theme_dir.'core/'.$script); + $found += $this->appendScriptIfExist($this->serverroot, $script); + $found += $this->appendScriptIfExist($this->serverroot, $theme_dir.$script); + $found += $this->appendScriptIfExist($this->serverroot, 'apps/'.$script); + $found += $this->appendScriptIfExist($this->serverroot, $theme_dir.'apps/'.$script); if ($found) { return; } - } elseif ($this->appendIfExist($this->serverroot, $theme_dir.'apps/'.$script.'.js') - || $this->appendIfExist($this->serverroot, $theme_dir.$script.'.js') - || $this->appendIfExist($this->serverroot, $script.'.js') - || $this->appendIfExist($this->serverroot, $theme_dir . "dist/$app-$scriptName.js") - || $this->appendIfExist($this->serverroot, "dist/$app-$scriptName.js") - || $this->appendIfExist($this->serverroot, 'apps/'.$script.'.js') + } elseif ($this->appendScriptIfExist($this->serverroot, $theme_dir.'apps/'.$script) + || $this->appendScriptIfExist($this->serverroot, $theme_dir.$script) + || $this->appendScriptIfExist($this->serverroot, $script) + || $this->appendScriptIfExist($this->serverroot, $theme_dir."dist/$app-$scriptName") + || $this->appendScriptIfExist($this->serverroot, "dist/$app-$scriptName") + || $this->appendScriptIfExist($this->serverroot, 'apps/'.$script) || $this->cacheAndAppendCombineJsonIfExist($this->serverroot, $script.'.json') - || $this->appendIfExist($this->serverroot, $theme_dir.'core/'.$script.'.js') - || $this->appendIfExist($this->serverroot, 'core/'.$script.'.js') - || (strpos($scriptName, '/') === -1 && ($this->appendIfExist($this->serverroot, $theme_dir . "dist/core-$scriptName.js") - || $this->appendIfExist($this->serverroot, "dist/core-$scriptName.js"))) + || $this->appendScriptIfExist($this->serverroot, $theme_dir.'core/'.$script) + || $this->appendScriptIfExist($this->serverroot, 'core/'.$script) + || (strpos($scriptName, '/') === -1 && ($this->appendScriptIfExist($this->serverroot, $theme_dir."dist/core-$scriptName") + || $this->appendScriptIfExist($this->serverroot, "dist/core-$scriptName"))) || $this->cacheAndAppendCombineJsonIfExist($this->serverroot, 'core/'.$script.'.json') ) { return; @@ -107,7 +107,7 @@ class JSResourceLocator extends ResourceLocator { // missing translations files fill be ignored if (strpos($script, 'l10n/') === 0) { - $this->appendIfExist($app_path, $script . '.js', $app_url); + $this->appendScriptIfExist($app_path, $script, $app_url); return; } @@ -130,6 +130,17 @@ class JSResourceLocator extends ResourceLocator { public function doFindTheme($script) { } + /** + * Try to find ES6 script file (`.mjs`) with fallback to plain javascript (`.js`) + * @see appendIfExist() + */ + protected function appendScriptIfExist($root, $file, $webRoot = null) { + if (!$this->appendIfExist($root, $file . '.mjs', $webRoot)) { + return $this->appendIfExist($root, $file . '.js', $webRoot); + } + return true; + } + protected function cacheAndAppendCombineJsonIfExist($root, $file, $app = 'core') { if (is_file($root.'/'.$file)) { if ($this->jsCombiner->process($root, $file, $app)) { diff --git a/tests/lib/Template/JSResourceLocatorTest.php b/tests/lib/Template/JSResourceLocatorTest.php index 20fd79a91b5..627fe676680 100644 --- a/tests/lib/Template/JSResourceLocatorTest.php +++ b/tests/lib/Template/JSResourceLocatorTest.php @@ -26,6 +26,7 @@ namespace Test\Template; use OC\SystemConfig; use OC\Template\JSCombiner; use OC\Template\JSResourceLocator; +use OCP\App\IAppManager; use OCP\Files\IAppData; use OCP\ICacheFactory; use OCP\IURLGenerator; @@ -42,6 +43,8 @@ class JSResourceLocatorTest extends \Test\TestCase { protected $cacheFactory; /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ protected $logger; + /** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */ + protected $appManager; protected function setUp(): void { parent::setUp(); @@ -51,6 +54,7 @@ class JSResourceLocatorTest extends \Test\TestCase { $this->config = $this->createMock(SystemConfig::class); $this->cacheFactory = $this->createMock(ICacheFactory::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->appManager = $this->createMock(IAppManager::class); } private function jsResourceLocator() { @@ -63,7 +67,8 @@ class JSResourceLocatorTest extends \Test\TestCase { ); return new JSResourceLocator( $this->logger, - $jsCombiner + $jsCombiner, + $this->appManager, ); } @@ -84,25 +89,34 @@ class JSResourceLocatorTest extends \Test\TestCase { } public function testFindWithAppPathSymlink() { + $appName = 'test-js-app'; + // First create new apps path, and a symlink to it $apps_dirname = $this->randomString(); $new_apps_path = sys_get_temp_dir() . '/' . $apps_dirname; $new_apps_path_symlink = $new_apps_path . '_link'; - mkdir($new_apps_path); - symlink($apps_dirname, $new_apps_path_symlink); + $this->assertTrue(( + mkdir($new_apps_path) && symlink($apps_dirname, $new_apps_path_symlink) + ), 'Setup of apps path failed'); // Create an app within that path - mkdir($new_apps_path . '/' . 'test-js-app'); + $this->assertTrue(( + mkdir($new_apps_path . '/' . $appName) && touch($new_apps_path . '/' . $appName . '/' . 'test-file.js') + ), 'Setup of app within the new apps path failed'); // Use the symlink as the app path - \OC::$APPSROOTS[] = [ - 'path' => $new_apps_path_symlink, - 'url' => '/js-apps-test', - 'writable' => false, - ]; - + $this->appManager->expects($this->once()) + ->method('getAppPath') + ->with($appName) + ->willReturn("$new_apps_path_symlink/$appName"); + $this->appManager->expects($this->once()) + ->method('getAppWebPath') + ->with($appName) + ->willReturn("/js-apps-test/$appName"); + + // Run the tests $locator = $this->jsResourceLocator(); - $locator->find(['test-js-app/test-file']); + $locator->find(["$appName/test-file"]); $resources = $locator->getResources(); $this->assertCount(1, $resources); @@ -122,7 +136,45 @@ class JSResourceLocatorTest extends \Test\TestCase { $this->assertEquals($expectedFile, $file); array_pop(\OC::$APPSROOTS); - unlink($new_apps_path_symlink); + //unlink($new_apps_path_symlink); + //$this->rrmdir($new_apps_path); + } + + public function testFindModuleJSWithFallback() { + // First create new apps path, and a symlink to it + $apps_dirname = $this->randomString(); + $new_apps_path = sys_get_temp_dir() . '/' . $apps_dirname; + mkdir($new_apps_path); + + // Create an app within that path + mkdir("$new_apps_path/test-js-app"); + touch("$new_apps_path/test-js-app/module.mjs"); + touch("$new_apps_path/test-js-app/both.mjs"); + touch("$new_apps_path/test-js-app/both.js"); + touch("$new_apps_path/test-js-app/plain.js"); + + // Use the app path + $this->appManager->expects($this->any()) + ->method('getAppPath') + ->with('test-js-app') + ->willReturn("$new_apps_path/test-js-app"); + + $locator = $this->jsResourceLocator(); + $locator->find(['test-js-app/module', 'test-js-app/both', 'test-js-app/plain']); + + $resources = $locator->getResources(); + $this->assertCount(3, $resources); + + $expectedRoot = $new_apps_path . '/test-js-app'; + $expectedWebRoot = \OC::$WEBROOT . '/js-apps-test/test-js-app'; + $expectedFiles = ['module.mjs', 'both.mjs', 'plain.js']; + + for ($idx = 0; $idx++; $idx < 3) { + $this->assertEquals($expectedWebRoot, $resources[$idx][1]); + $this->assertEquals($expectedFiles[$idx], $resources[$idx][2]); + } + + array_pop(\OC::$APPSROOTS); $this->rrmdir($new_apps_path); } } From b642137c652e5262c88dd4ddc055df684c8da8a2 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 10 Jan 2023 01:40:42 +0100 Subject: [PATCH 3/5] feat(OC/Template): Add `type="module"` for ES6 scripts Signed-off-by: Ferdinand Thiessen --- lib/private/legacy/template/functions.php | 16 +++++++++---- tests/lib/TemplateFunctionsTest.php | 29 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/lib/private/legacy/template/functions.php b/lib/private/legacy/template/functions.php index 56c488d5abe..7081bd4f743 100644 --- a/lib/private/legacy/template/functions.php +++ b/lib/private/legacy/template/functions.php @@ -72,14 +72,19 @@ function emit_css_loading_tags($obj) { * Prints a