Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(JSResourceLocator): Consider configured app roots for files #43917

Merged
merged 1 commit into from
Mar 7, 2024
Merged

fix(JSResourceLocator): Consider configured app roots for files #43917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 22 additions & 40 deletions lib/private/Template/JSResourceLocator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,22 @@ public function doFind($script) {
// Extracting the appId and the script file name
$app = substr($script, 0, strpos($script, '/'));
$scriptName = basename($script);
// Get the app root path
$appRoot = $this->serverroot . 'apps/';
$appWebRoot = null;
try {
// We need the dir name as getAppPath appends the appid
$appRoot = dirname($this->appManager->getAppPath($app));
// Only do this if $app_path is set, because an empty argument to realpath gets turned into cwd.
if ($appRoot) {
// Handle symlinks
$appRoot = realpath($appRoot);
}
// Get the app webroot
$appWebRoot = dirname($this->appManager->getAppWebPath($app));
} catch (AppPathNotFoundException $e) {
// ignore
}

if (str_contains($script, '/l10n/')) {
// For language files we try to load them all, so themes can overwrite
Expand All @@ -60,13 +76,8 @@ public function doFind($script) {
$found += $this->appendScriptIfExist($this->serverroot, $theme_dir.'core/'.$script);
$found += $this->appendScriptIfExist($this->serverroot, $script);
$found += $this->appendScriptIfExist($this->serverroot, $theme_dir.$script);

foreach (\OC::$APPSROOTS as $appRoot) {
$dirName = basename($appRoot['path']);
$rootPath = dirname($appRoot['path']);
$found += $this->appendScriptIfExist($rootPath, $dirName.'/'.$script);
$found += $this->appendScriptIfExist($this->serverroot, $theme_dir.$dirName.'/'.$script);
}
$found += $this->appendScriptIfExist($appRoot, $script, $appWebRoot);
$found += $this->appendScriptIfExist($this->serverroot, $theme_dir.'apps/'.$script);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug previously, because for all other scripts we always looked into themes/{themename}/apps/{appname}.
The mentioned PR broke this for translations that were now only searched in `themes/{themename}/{appswebroot}/{appname}.


if ($found) {
return;
Expand All @@ -76,8 +87,9 @@ public function doFind($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->appendScriptIfExist($appRoot, $script, $appWebRoot)
|| $this->cacheAndAppendCombineJsonIfExist($this->serverroot, $script.'.json')
|| $this->cacheAndAppendCombineJsonIfExist($appRoot, $script.'.json', $appWebRoot)
|| $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")
Expand All @@ -87,43 +99,13 @@ public function doFind($script) {
return;
}

$script = substr($script, strpos($script, '/') + 1);
$app_url = null;

try {
$app_url = $this->appManager->getAppWebPath($app);
} catch (AppPathNotFoundException) {
// pass
}

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);

// check combined files
if (!str_starts_with($script, 'l10n/') && $this->cacheAndAppendCombineJsonIfExist($app_path, $script.'.json', $app)) {
return;
}

// fallback to plain file location
if ($this->appendScriptIfExist($app_path, $script, $app_url)) {
return;
}
} catch (AppPathNotFoundException) {
// pass (general error handling happens below)
}

// missing translations files will be ignored
if (str_starts_with($script, 'l10n/')) {
if (str_contains($script, '/l10n/')) {
return;
}

$this->logger->error('Could not find resource {resource} to load', [
'resource' => $app . '/' . $script . '.js',
'resource' => $script . '.js',
'app' => 'jsresourceloader',
]);
}
Expand Down
8 changes: 4 additions & 4 deletions tests/lib/Template/JSResourceLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ public function testFindWithAppPathSymlink() {
$webRoot = $resource[1];
$file = $resource[2];

$expectedRoot = $new_apps_path . '/test-js-app';
$expectedWebRoot = \OC::$WEBROOT . '/js-apps-test/test-js-app';
$expectedFile = 'test-file.js';
$expectedRoot = $new_apps_path;
$expectedWebRoot = \OC::$WEBROOT . '/js-apps-test';
$expectedFile = $appName . '/test-file.js';

$this->assertEquals($expectedRoot, $root,
'Ensure the app path symlink is resolved into the real path');
Expand All @@ -145,7 +145,7 @@ public function testNotExistingTranslationHandledSilent() {
->method('getAppPath')
->with('core')
->willThrowException(new AppPathNotFoundException());
$this->appManager->expects($this->once())
$this->appManager->expects($this->atMost(1))
->method('getAppWebPath')
->with('core')
->willThrowException(new AppPathNotFoundException());
Expand Down
Loading