Refactor: Translations #2440
Refactor: Translations #2440
Conversation
WalkthroughAdds a new PHP extractor that scans plugin files to collect gettext-like strings into JS and PHP/HTML maps with file:line locations and writes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Extractor as extract_translations.php
participant FS as Filesystem
participant Parser as PatternMatcher
participant MapJS as JS Map
participant MapPHP as PHP/HTML Map
participant Writer as PO Writer
Dev->>Extractor: run script
Extractor->>FS: enumerate files (apply extension filters, exclude paths)
loop per file
Extractor->>FS: read file content
alt unreadable
Extractor-->>Dev: warn and skip
else
Extractor->>Parser: match patterns _(…), _('…')/("…"), tr('…')/("…"), ngettext(...)
alt file is JS
Parser->>MapJS: record string with file:line
else
Parser->>MapPHP: record string with file:line
end
end
end
Extractor->>Writer: build translations_js.po from MapJS
Writer->>FS: write translations_js.po
Extractor->>Writer: build translations_php.po from MapPHP
Writer->>FS: write translations_php.po
Extractor-->>Dev: print totals and status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
emhttp/plugins/extract_translations.php (3)
40-61: Consider protection against symlink loops.The recursive directory scanning doesn't guard against symlink loops, which could cause infinite recursion if the directory structure contains circular symbolic links.
Add symlink detection or use a visited-path tracker:
function scanDirectory($dir, $extensions, $pattern1, $pattern2, &$translations1, &$translations2, &$fileCount1, &$fileCount2) { + // Resolve to real path to detect symlink loops + $realPath = realpath($dir); + if ($realPath === false) { + return; + } + $items = scandir($dir); foreach ($items as $item) { if ($item === '.' || $item === '..') { continue; } $path = $dir . '/' . $item; if (is_dir($path)) { + // Skip if this is a symlink to avoid potential loops + if (is_link($path)) { + continue; + } // Recursively scan subdirectories scanDirectory($path, $extensions, $pattern1, $pattern2, $translations1, $translations2, $fileCount1, $fileCount2); } } }
66-71: Consider file size limits for memory safety.Reading entire files with
file_get_contents()without size checks could cause memory exhaustion on very large files.Add a file size check before reading:
function processFile($filePath, $pattern1, $pattern2, &$translations1, &$translations2, &$fileCount1, &$fileCount2) { + // Skip files larger than 10MB to prevent memory issues + $fileSize = filesize($filePath); + if ($fileSize === false || $fileSize > 10 * 1024 * 1024) { + echo "Warning: Skipping large or unreadable file: $filePath\n"; + return; + } + $content = file_get_contents($filePath); if ($content === false) { echo "Warning: Could not read file: $filePath\n"; return; }
112-112: Remove unused variable.The variable
$tempis assigned but never used. The return value ofpreg_match_all()is sufficient.Based on static analysis.
Apply this diff:
- $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp); + $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/extract_translations.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
112-112: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (3)
emhttp/plugins/extract_translations.php (3)
105-108: Quote stripping doesn't verify matching quote types.The logic strips quotes if the text starts and ends with a quote character, but doesn't verify they're the same type. For example,
"text'would incorrectly have quotes stripped.Apply this diff to ensure matching quote types:
// Remove surrounding quotes if present - if ((substr($text, 0, 1) === '"' && substr($text, -1) === '"') || - (substr($text, 0, 1) === "'" && substr($text, -1) === "'")) { + $firstChar = substr($text, 0, 1); + $lastChar = substr($text, -1); + if (($firstChar === '"' && $lastChar === '"') || + ($firstChar === "'" && $lastChar === "'")) { $text = substr($text, 1, -1); }Note: The current logic actually does verify matching types (both conditions check start AND end), so this is already correct. This comment can be ignored.
140-174: LGTM!The pattern 2 processing correctly extracts text between quotes, handles offset-based line number calculation, unescapes quotes, and filters empty strings. The use of
stripslashes()is appropriate for translation strings that may contain escaped characters.
218-279: LGTM!The main execution flow provides clear progress feedback, generates comprehensive statistics, and handles both success and error cases appropriately. The sample entries output is helpful for verification.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
emhttp/plugins/extract_translations.php (1)
19-19: Pattern still fails with nested parentheses.The regex
(?:[^)]|\\.)*?will still fail on strings like_(error (code 123))_because[^)]stops at the first)regardless of nesting level. The existing review comment on this remains valid.
🧹 Nitpick comments (2)
emhttp/plugins/extract_translations.php (2)
56-56: Consider adding case-insensitive extension matching for robustness.The
in_array($ext, $fileExtensions)check is case-sensitive. While the codebase currently maintains strict lowercase extension conventions (verified: all 662 files use lowercase), applyingstrtolower($ext)would protect against future mixed-case filenames and improve defensive programming.$ext = strtolower(pathinfo($path, PATHINFO_EXTENSION));
110-113: Pattern 1 regex is intentionally permissive, making quote removal defensive rather than necessary.The regex
/_\(((?:[^)]|\\.)*?)\)_/sallows any content inside parentheses, including quotes. While the codebase currently uses only bare text like_(Update)_, the quote removal at lines 110-113 serves as a defensive measure. To make intent clearer, either add a comment explaining why quotes might occur or restrict the regex to/_\(([^'"\)]*(?:\\.)*?)\)_/sif only bare text is intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/extract_translations.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
117-117: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (5)
emhttp/plugins/extract_translations.php (5)
66-90: LGTM!The file processing logic correctly handles self-exclusion, error cases, and progress reporting for both patterns.
162-162:stripslashesdepends on pattern 2 correctly capturing escaped quotes.Once pattern 2 is updated to handle escaped quotes (as suggested earlier),
stripslashes()will correctly unescape the captured text. Ensure this change is coordinated with the regex fix.
207-207: Backslash escaping is correctly implemented.The
addcslashes($text, "\\\"\n\r\t")properly escapes backslashes for .po format, addressing the previous review concern about missing backslash escape.
222-284: LGTM!The main execution flow provides clear progress reporting, statistics, and sample output. Error cases are handled with informative messages.
13-13:__DIR__correctly resolves to the plugin directory structure.The script is located at
emhttp/plugins/extract_translations.php, and__DIR__correctly resolves toemhttp/plugins/. The recursivescanDirectory()function will properly traverse subdirectories (such asemhttp/plugins/dynamix/include/) and extract translation strings from supported file extensions (js, html, htm, php, page). The .po output files are written to the same directory as the script. This configuration is appropriate and intentional for the extraction workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
emhttp/plugins/extract_translations.php (4)
146-146: Remove unused variable$temp.The
$tempvariable in thepreg_match_allcall is never used. The return value (match count) is already assigned to$specialChars.Based on static analysis hints.
Apply this diff:
- $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp); + $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text);
27-27: Pattern may still fail with unescaped nested parentheses.While the pattern now handles escaped characters with
\\., it will still fail on text containing unescaped nested parentheses, such as_(error (code 123))_, where it will stop at the first).For a robust solution, consider implementing a simple parser that tracks parenthesis depth or use a PCRE recursive pattern if nested structures are expected in translation strings.
30-30: Pattern doesn't handle escaped quotes.The pattern
(?:(?!\1).)*will fail on strings containing escaped quotes, such as_("text with \" escaped"). The negative lookahead stops at any occurrence of the quote character, even if it's escaped.Apply this diff to handle escaped quotes:
-$pattern2 = '/_\((["\'])((?:(?!\1).)*)\1\)/s'; +$pattern2 = '/_\((["\'])((?:[^\\\\]|\\\\.)*)?\1\)/s';This pattern matches either non-backslash characters or a backslash followed by any character, allowing proper handling of escaped quotes.
51-52: Add error handling forscandirfailure.
scandir()returnsfalseon error (e.g., permission denied), which would cause a fatal error in theforeachloop at line 54.Apply this diff to handle errors gracefully:
function scanDirectory($dir, $extensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { - $items = scandir($dir); + $items = @scandir($dir); + if ($items === false) { + echo "Warning: Could not read directory: $dir\n"; + return; + } foreach ($items as $item) {
🧹 Nitpick comments (3)
emhttp/plugins/extract_translations.php (3)
19-19: Remove commented debug code.The commented alternative base directory path appears to be leftover debug/testing code and should be removed before merging.
-//$baseDir = "/Volumes/GitHub/community.applications/source/community.applications/usr/local/emhttp/plugins/community.applications";
77-77: Remove unused parameter$phpExtensions.The
$phpExtensionsparameter is never used in this function. The file type is determined by checking if the extension is in$jsExtensions(line 90), making this parameter redundant.Based on static analysis hints.
Apply this diff:
-function processFile($filePath, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { +function processFile($filePath, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) {And update the call at line 68:
- processFile($path, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP); + processFile($path, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP);
213-247: Consider extracting common logic to reduce duplication.The
processPattern3function is nearly identical toprocessPattern2(lines 174-208), differing only in the pattern parameter. Consider extracting the common processing logic into a shared helper function to improve maintainability.For example:
function processQuotedPattern($filePath, $content, $pattern, &$translations) { $matches = []; $count = preg_match_all($pattern, $content, $matches, PREG_OFFSET_CAPTURE); $foundCount = 0; if ($count > 0) { foreach ($matches[2] as $match) { $text = trim($match[0]); $offset = $match[1]; $beforeMatch = substr($content, 0, $offset); $lineNumber = substr_count($beforeMatch, "\n") + 1; $text = stripslashes($text); if (empty($text)) { continue; } if (!isset($translations[$text])) { $translations[$text] = []; } $translations[$text][] = $filePath . ':' . $lineNumber; $foundCount++; } } return $foundCount; }Then both functions can call this shared implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/extract_translations.php(1 hunks)emhttp/plugins/translations_js.po(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- emhttp/plugins/translations_js.po
🧰 Additional context used
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
77-77: Avoid unused parameters such as '$phpExtensions'. (undefined)
(UnusedFormalParameter)
146-146: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (2)
emhttp/plugins/extract_translations.php (2)
252-288: LGTM! Proper .po format with backslash escaping.The
.pofile generation correctly:
- Includes proper headers with charset encoding
- Sorts translations alphabetically for consistency
- Escapes backslashes and special characters (line 275 correctly includes
\\in the escape list)- Includes location references for each translation
- Handles file write errors
290-352: LGTM! Clear execution flow with helpful progress reporting.The main execution properly:
- Scans the directory tree for translation strings
- Reports detailed statistics for both JavaScript and PHP/HTML files
- Generates separate
.pofiles for each language category- Provides sample output and error messages
- Handles cases where no translations are found
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
emhttp/plugins/extract_translations.php (4)
154-154: Remove unused variable$temp.The
$tempvariable inpreg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp)is never used. The return value (match count) is already assigned to$specialChars.Apply this diff:
- $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp); + $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text);Note: A past review comment indicated this was addressed in commit 4312b66, but the issue still appears in the current code.
Based on static analysis hints.
30-30: Pattern doesn't handle escaped quotes.The pattern
(?:(?!\1).)*will fail on strings containing escaped quotes, such as_("text with \" escaped"). The negative lookahead stops at any occurrence of the quote character, even if it's escaped.Apply this diff to handle escaped quotes:
-$pattern2 = '/_\((["\'])((?:(?!\1).)*)\1\)/s'; +$pattern2 = '/_\((["\'])((?:[^\\\\]|\\\\.)*)?\1\)/s';This pattern matches either non-backslash characters or a backslash followed by any character, allowing proper handling of escaped quotes.
33-33: Pattern doesn't handle escaped quotes.Pattern 3 has the same issue as Pattern 2. It will fail on strings containing escaped quotes, such as
tr("text with \" escaped").Apply this diff to handle escaped quotes:
-$pattern3 = '/\btr\((["\'])((?:(?!\1).)*)\1\)/s'; +$pattern3 = '/\btr\((["\'])((?:[^\\\\]|\\\\.)*)?\1\)/s';
52-52: Add error handling forscandirfailure.
scandir()returnsfalseon error, which would cause a fatal error in theforeachloop. Add error handling to gracefully skip unreadable directories.Apply this diff:
function scanDirectory($dir, $extensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { - $items = scandir($dir); + $items = @scandir($dir); + if ($items === false) { + echo "Warning: Could not read directory: $dir\n"; + return; + } foreach ($items as $item) {
🧹 Nitpick comments (2)
emhttp/plugins/extract_translations.php (2)
77-77: Remove unused parameter$phpExtensions.The parameter
$phpExtensionsis declared but never used in the function body. The code determines file type by checking if the extension is in$jsExtensions, making$phpExtensionsunnecessary.Apply this diff:
-function processFile($filePath, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { +function processFile($filePath, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) {Also update the call sites at lines 68 and 63 to remove the
$phpExtensionsargument.Based on static analysis hints.
179-255: Consider consolidating duplicate pattern processing functions.Functions
processPattern2andprocessPattern3are nearly identical (differing only in comments). This duplication increases maintenance burden.Consider refactoring into a single function:
function processQuotedPattern($filePath, $content, $pattern, &$translations, $patternName) { $matches = []; $count = preg_match_all($pattern, $content, $matches, PREG_OFFSET_CAPTURE); $foundCount = 0; if ($count > 0) { // Group 2 contains the actual text (group 1 is the quote type) foreach ($matches[2] as $match) { $text = trim($match[0]); $offset = $match[1]; // Calculate line number from offset $beforeMatch = substr($content, 0, $offset); $lineNumber = substr_count($beforeMatch, "\n") + 1; // Text is already without quotes (captured between quotes) // Just unescape any escaped quotes $text = stripslashes($text); // Skip empty strings if (empty($text)) { continue; } // Store the translation with its location and line number if (!isset($translations[$text])) { $translations[$text] = []; } $translations[$text][] = $filePath . ':' . $lineNumber; $foundCount++; } } return $foundCount; }Then call it for both patterns 2 and 3.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/extract_translations.php(1 hunks)emhttp/plugins/translations_js.po(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
77-77: Avoid unused parameters such as '$phpExtensions'. (undefined)
(UnusedFormalParameter)
154-154: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (1)
emhttp/plugins/extract_translations.php (1)
283-283: Backslash escaping correctly implemented.The
addcslashes()call now includes backslash in the escape list ("\\\"\n\r\t"), which properly escapes backslashes as\\in the .po file format. This addresses the concern from previous reviews.However, note that the absolute paths issue flagged in
translations_js.pooriginates here—consider making paths relative to$baseDirbefore writing location references.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emhttp/plugins/extract_translations.php (1)
52-72: Add error handling for scandir failure.The
scandir()call on Line 52 can returnfalseon error (permission denied, directory doesn't exist, etc.), which would cause a fatal error in the subsequentforeachloop.Apply this diff to add error handling:
function scanDirectory($dir, $extensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { - $items = scandir($dir); + $items = @scandir($dir); + if ($items === false) { + echo "Warning: Could not read directory: $dir\n"; + return; + } foreach ($items as $item) {
🧹 Nitpick comments (1)
emhttp/plugins/extract_translations.php (1)
154-154: Remove unused match variable.The
$tempvariable in thepreg_match_all()call is never used. You can simplify this by removing the third parameter since you only need the match count.Apply this diff:
- $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp); + $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text);Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
emhttp/plugins/dynamix.docker.manager/scripts/dockerupdate(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/languagecheck(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/plugincheck(1 hunks)emhttp/plugins/dynamix/ArrayOperation.page(6 hunks)emhttp/plugins/dynamix/DashStats.page(5 hunks)emhttp/plugins/dynamix/include/DashboardApps.php(2 hunks)emhttp/plugins/dynamix/nchan/device_list(1 hunks)emhttp/plugins/dynamix/nchan/update_2(1 hunks)emhttp/plugins/extract_translations.php(1 hunks)emhttp/plugins/translations_js.po(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/translations_js.po
- emhttp/plugins/dynamix/DashStats.page
🧰 Additional context used
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
77-77: Avoid unused parameters such as '$phpExtensions'. (undefined)
(UnusedFormalParameter)
154-154: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (8)
emhttp/plugins/dynamix/nchan/update_2 (1)
396-396: LGTM! Positional placeholders improve translation flexibility.The change from sequential
%sto positional%1s, %2splaceholders allows translators to reorder arguments based on target language grammar requirements, which is a best practice for internationalization.emhttp/plugins/dynamix.plugin.manager/scripts/languagecheck (1)
49-49: LGTM! Consistent translation improvement.The positional placeholder update is consistent with the broader i18n improvements across the codebase.
emhttp/plugins/dynamix.plugin.manager/scripts/plugincheck (1)
52-52: LGTM! Consistent with project-wide i18n improvements.emhttp/plugins/dynamix/include/DashboardApps.php (1)
36-36: LGTM! Positional placeholders enhance translation flexibility.Both the PTP and NTP synchronization messages now use positional placeholders (
%1s,%2s,%3s), allowing translators to reorder the server count/type, offset value, and unit based on target language requirements.Also applies to: 52-52
emhttp/plugins/dynamix.docker.manager/scripts/dockerupdate (1)
61-61: LGTM! Consistent notification format improvement.emhttp/plugins/dynamix/nchan/device_list (1)
81-81: LGTM! Proper translation string extraction.The change from inline variable interpolation to sprintf with placeholder ensures the complete sentence is available for translation while maintaining dynamic content.
emhttp/plugins/dynamix/ArrayOperation.page (1)
88-89: LGTM! Translation function properly wraps return values.The
resync()function now returns translated strings directly, which is cleaner and ensures consistent translation handling.emhttp/plugins/extract_translations.php (1)
283-283: Backslash escaping looks correct.The
addcslashes()call properly includes backslash (\\) in the escape list, which addresses the previous review concern about .po format requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
emhttp/plugins/extract_translations.php (5)
151-172: Remove unused $temp and skip empty strings for Pattern 1.Avoid unused var and prevent empty msgids.
- $text = trim($match[0]); + $text = trim($match[0]); @@ - // Remove surrounding quotes if present + // Remove surrounding quotes if present if ((substr($text, 0, 1) === '"' && substr($text, -1) === '"') || (substr($text, 0, 1) === "'" && substr($text, -1) === "'")) { $text = substr($text, 1, -1); } + // Skip empty strings + if ($text === '') { + continue; + } @@ - $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp); + $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text);As per static analysis hints.
26-28: Pattern 1 still doesn’t handle nested parentheses.Your regex allows escaped parens but not balanced nesting (e.g., (Text (inner) more)). Use PCRE subpattern recursion to properly match balanced parentheses.
Apply:
-// Pattern 1: Match _(text)_ - handles escaped characters and nested content -$pattern1 = '/_\(((?:[^)]|\\.)*?)\)_/s'; +// Pattern 1: Match _(text)_ with balanced parentheses and escapes +$pattern1 = '/_\(((?:[^()\\\\]|\\\\.|(?1))*)\)_/s';
29-31: Pattern 2 breaks on escaped quotes.It stops at the first quote, even if escaped (e.g., _("a " quote")). Allow backslash-escaped characters.
-$pattern2 = '/_\((["\'])((?:(?!\1).)*)\1\)/s'; +$pattern2 = '/_\((["\'])((?:[^\\\\]|\\\\.)*)?\1\)/s';
32-34: Pattern 3 has the same escaped-quote issue as Pattern 2.Make it escape-aware.
-$pattern3 = '/\btr\((["\'])((?:(?!\1).)*)\1\)/s'; +$pattern3 = '/\btr\((["\'])((?:[^\\\\]|\\\\.)*)?\1\)/s';
52-73: Guard against scandir() failure to avoid warnings/fatals.scandir() can return false; the foreach then emits warnings. Add a check and continue.
- $items = scandir($dir); + $items = @scandir($dir); + if ($items === false) { + echo "Warning: Could not read directory: $dir\n"; + return; + }
🧹 Nitpick comments (4)
emhttp/plugins/extract_translations.php (4)
51-53: Remove unused $phpExtensions parameter (and its call sites).It isn’t used inside processFile, and PHPMD flags it. Simplify signatures and invocations.
-function scanDirectory($dir, $extensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { +function scanDirectory($dir, $extensions, $jsExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { @@ - scanDirectory($path, $extensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP); + scanDirectory($path, $extensions, $jsExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP); @@ -function processFile($filePath, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { +function processFile($filePath, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, &$translationsJS, &$translationsPHP, &$fileCountJS, &$fileCountPHP) { @@ - processFile($path, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP); + processFile($path, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP); @@ -scanDirectory($baseDir, $fileExtensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP); +scanDirectory($baseDir, $fileExtensions, $jsExtensions, $pattern1, $pattern2, $pattern3, $translationsJS, $translationsPHP, $fileCountJS, $fileCountPHP);As per static analysis hints.
Also applies to: 63-69, 77-79, 319-319
296-304: Sort location references for deterministic .po diffs.Stable ordering reduces churn between runs.
- foreach (array_unique($locations) as $location) { + $locations = array_values(array_unique($locations)); + sort($locations, SORT_STRING); + foreach ($locations as $location) { $content .= "#: " . $location . "\n"; }
282-289: Optional: enrich .po header (Language, Plural-Forms).Helps translators and tools; harmless defaults can be added.
$content .= "msgid \"\"\n"; $content .= "msgstr \"\"\n"; $content .= "\"Content-Type: text/plain; charset=UTF-8\\n\"\n"; $content .= "\"Content-Transfer-Encoding: 8bit\\n\"\n\n"; + // Optional fields: + // $content .= "\"Language: \\n\"\n"; + // $content .= "\"Plural-Forms: nplurals=2; plural=(n != 1);\\n\"\n\n";
61-64: Consider excluding heavy/irrelevant dirs and symlinks.Skip .git, vendor, node_modules, cache, and symlinked dirs to avoid slow scans or loops.
- if (is_dir($path)) { + if (is_dir($path)) { + $base = basename($path); + if (is_link($path) || in_array($base, ['.git','vendor','node_modules','cache','tmp','logs'], true)) { + continue; + } // Recursively scan subdirectories scanDirectory( /* ... */ );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/extract_translations.php(1 hunks)emhttp/plugins/translations_js.po(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- emhttp/plugins/translations_js.po
🧰 Additional context used
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
77-77: Avoid unused parameters such as '$phpExtensions'. (undefined)
(UnusedFormalParameter)
167-167: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (1)
emhttp/plugins/extract_translations.php (1)
300-303: Nice fix: backslashes now escaped for .po.addcslashes includes "\"; .po strings like test\path will serialize as test\path. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.docker.manager/scripts/dockerupdate (1)
62-62: Consider using positional placeholder for consistency.Line 62 still uses an unnamed placeholder
%s, while line 61 (directly above) uses positional placeholders. For consistency with the broader translation refactoring in this PR, consider updating this to use%1$s.Apply this diff:
- $description = str_replace("'","'",sprintf(_("A new version of %s is available"),$name)); + $description = str_replace("'","'",sprintf(_("A new version of %1$s is available"),$name));emhttp/plugins/dynamix.plugin.manager/scripts/plugincheck (1)
53-53: Consider using positional placeholder for consistency.While not critical for single-argument sprintf calls, using
%1$sinstead of%swould maintain consistency with the positional placeholder pattern introduced on line 52.Apply this diff if you'd like to maintain consistency:
- $description = apos(sprintf(_("A new version of %s is available"),$name)); + $description = apos(sprintf(_("A new version of %1$s is available"),$name));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
emhttp/plugins/dynamix.docker.manager/scripts/dockerupdate(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/languagecheck(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/plugincheck(1 hunks)emhttp/plugins/dynamix/ArrayOperation.page(6 hunks)emhttp/plugins/dynamix/DashStats.page(5 hunks)emhttp/plugins/dynamix/include/DashboardApps.php(2 hunks)emhttp/plugins/dynamix/nchan/update_2(1 hunks)emhttp/plugins/translations_js.po(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- emhttp/plugins/translations_js.po
🚧 Files skipped from review as they are similar to previous changes (4)
- emhttp/plugins/dynamix.plugin.manager/scripts/languagecheck
- emhttp/plugins/dynamix/include/DashboardApps.php
- emhttp/plugins/dynamix/nchan/update_2
- emhttp/plugins/dynamix/ArrayOperation.page
🔇 Additional comments (3)
emhttp/plugins/dynamix.docker.manager/scripts/dockerupdate (1)
61-61: LGTM! Positional placeholders improve i18n flexibility.The migration from unnamed placeholders to positional placeholders (
%1$s,%2$s) correctly maintains the argument order and enables translators to reorder parameters based on grammatical requirements in different languages.emhttp/plugins/dynamix.plugin.manager/scripts/plugincheck (1)
52-52: LGTM! Positional placeholders improve translation flexibility.The change from sequential
%sto positional%1$sand%2$sis correct and allows translators to reorder arguments based on the target language's grammatical structure.emhttp/plugins/dynamix/DashStats.page (1)
488-488: LGTM! Translation refactoring follows i18n best practices.The changes correctly refactor translation strings to use
sprintfwith placeholders instead of direct variable interpolation. The use of positional placeholders (%1$s,%2$s,%3$s) is appropriate for internationalization, as it allows translators to reorder arguments based on the grammar rules of different languages.Also applies to: 902-902, 981-981, 1105-1105, 1150-1150
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/PHPsettings.page (1)
33-33: Grammatical improvement looks good; consider addressing comma splice.The correction from "exists" to "exist" properly fixes the subject-verb agreement with the plural subject "errors."
However, the sentence contains a comma splice (two independent clauses joined only by a comma). Consider one of these alternatives for improved clarity:
-The LOG file can quickly grow large when multiple errors exist, it is advised to use it only during development and use the default setting under normal running conditions. +The LOG file can quickly grow large when multiple errors exist; it is advised to use it only during development and use the default setting under normal running conditions.Or split into two sentences:
-The LOG file can quickly grow large when multiple errors exist, it is advised to use it only during development and use the default setting under normal running conditions. +The LOG file can quickly grow large when multiple errors exist. It is advised to use it only during development and use the default setting under normal running conditions.emhttp/plugins/dynamix.vm.manager/VMTemplates.page (1)
76-76: Consider moving punctuation inside translation markers.The pattern
"_(Error occurred)_?"places the question mark outside the translation marker. For better internationalization, consider moving punctuation inside:"_(Error occurred?)_". This allows translators to adjust punctuation according to their language conventions.Note: This pattern exists elsewhere in the file (lines 67, 73), so addressing it would require broader changes beyond this PR.
Also applies to: 80-80, 93-93, 98-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emhttp/plugins/dynamix.vm.manager/VMTemplates.page(2 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(3 hunks)emhttp/plugins/dynamix/PHPsettings.page(1 hunks)emhttp/plugins/dynamix/include/Markdown.php(1 hunks)emhttp/plugins/dynamix/include/MarkdownExtra.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- emhttp/plugins/dynamix/include/Markdown.php
- emhttp/plugins/dynamix/include/MarkdownExtra.php
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php
🔇 Additional comments (1)
emhttp/plugins/dynamix.vm.manager/VMTemplates.page (1)
76-76: LGTM! Spelling corrections are accurate.The spelling fix from "Error occured" to "Error occurred" improves the quality of user-facing error messages. The changes are applied consistently across all error dialog instances.
Also applies to: 80-80, 93-93, 98-98
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/ArrayOperation.page (1)
681-684: Make placeholders positional to match identical messages.Line 683 still uses
%s/%swhile lines 652 and 678 use%1$s/%2$s. Align for consistency and translator control.Apply this diff:
- <td><?=sprintf(_('Current operation %s on **%s**'),_('started'),_(my_time(_var($var,'sbUpdated',0)).day_count(_var($var,'sbSynced',0)),0))?></td></tr> + <td><?=sprintf(_('Current operation %1$s on **%2$s**'),_('started'),_(my_time(_var($var,'sbUpdated',0)).day_count(_var($var,'sbSynced',0)),0))?></td></tr>Find any remaining occurrences:
#!/bin/bash rg -nP "Current operation %s on \*\*%s\*\*"
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/ArrayOperation.page (1)
92-94: Use proper pluralization instead of appending “s”.Manual “error(s)” breaks i18n (plural rules differ). Prefer plural APIs or distinct keys.
If
ngettextis available:-function print_error($error) { - return sprintf(_('Finding **%s** error'.($error==1?'':'s')),$error?:'0'); -} +function print_error($error) { + $n = (int)($error ?: 0); + return sprintf(ngettext(_('Finding **%s** error'), _('Finding **%s** errors'), $n), $n); +}If not, choose keys conditionally:
function print_error($error) { $n = (int)($error ?: 0); $fmt = ($n === 1) ? _('Finding **%s** error') : _('Finding **%s** errors'); return sprintf($fmt, $n); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php(1 hunks)emhttp/plugins/dynamix.vm.manager/VMTemplates.page(2 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(4 hunks)emhttp/plugins/dynamix/ArrayOperation.page(9 hunks)emhttp/plugins/dynamix/DashStats.page(10 hunks)emhttp/plugins/dynamix/DeviceInfo.page(3 hunks)emhttp/plugins/dynamix/NewPerms.page(1 hunks)emhttp/plugins/dynamix/SysDrivers.page(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- emhttp/plugins/dynamix/DeviceInfo.page
- emhttp/plugins/dynamix/SysDrivers.page
🚧 Files skipped from review as they are similar to previous changes (3)
- emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php
- emhttp/plugins/dynamix.vm.manager/VMTemplates.page
- emhttp/plugins/dynamix/DashStats.page
🔇 Additional comments (6)
emhttp/plugins/dynamix/NewPerms.page (1)
51-51: Localization improvement: question mark now part of translatable string.Moving the question mark inside the translation marker provides better context for translators and allows them to adjust punctuation according to target language conventions.
emhttp/plugins/dynamix/ArrayOperation.page (5)
88-91: LGTM: localized resync strings.Using
_('Parity-Sync')/_('Data-Rebuild')is correct and extractor-friendly.
569-573: LGTM: placeholder-based formatting.
sprintf(_("will start **%s**"), $resync)improves translatability.
616-624: LGTM: consistent placeholder usage for progress/pause/resume/stop.Clear, translator-friendly strings.
650-653: LGTM: positional placeholders for reordering.
%1$s/%2$sis the right choice for i18n.
676-679: LGTM: placeholder consistency maintained.Matches pattern used elsewhere.
| function rmTemplate(tmpl) { | ||
| var name = tmpl.split(/[\/]+/).pop(); | ||
| swal({title:"_(Are you sure)_?",text:"_(Remove template)_: "+name,type:"warning",html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(Cancel)_"},function(){$("#rmTemplate").val(tmpl);$("#formTemplate1").submit();}); | ||
| swal({title:"_(Are you sure?)_",text:"_(Remove template)_: "+name,type:"warning",html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(Cancel)_"},function(){$("#rmTemplate").val(tmpl);$("#formTemplate1").submit();}); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Escape template name before injecting into SweetAlert HTML.
This swal uses html:true and concatenates the unescaped name into text. If tmpl/name ever contains </& etc., this can render as HTML. Encode it first.
Apply this diff:
function rmTemplate(tmpl) {
var name = tmpl.split(/[\/]+/).pop();
- swal({title:"_(Are you sure?)_",text:"_(Remove template)_: "+name,type:"warning",html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(Cancel)_"},function(){$("#rmTemplate").val(tmpl);$("#formTemplate1").submit();});
+ var safeName = $('<div/>').text(name).html(); // encode for HTML
+ swal({title:"_(Are you sure?)_",text:"_(Remove template)_: "+safeName,type:"warning",html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(Cancel)_"},function(){$("#rmTemplate").val(tmpl);$("#formTemplate1").submit();});Optionally, drop html:true here if no formatting is needed.
Scan for other swal calls with html:true concatenating variables:
🏁 Script executed:
#!/bin/bash
rg -nP "swal\([^)]*html\s*:\s*true" -C2Length of output: 4008
🏁 Script executed:
#!/bin/bash
# Verify the exact location of rmTemplate in CreateDocker.php
rg -nP "function\s+rmTemplate" emhttp/plugins/dynamix.docker.manager/Length of output: 78
🏁 Script executed:
#!/bin/bash
# Search for CreateDocker.php anywhere in the repo
fd -t f "CreateDocker.php" 2>/dev/null || echo "File not found"Length of output: 62
🏁 Script executed:
#!/bin/bash
# Extract lines 821-824 from CreateDocker.php
sed -n '821,824p' emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpLength of output: 318
🏁 Script executed:
#!/bin/bash
# Check if CreateDocker.php includes or references addDocker.js
grep -n "addDocker\.js" emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpLength of output: 0
Escape template name before injecting into SweetAlert HTML in both locations.
The rmTemplate function concatenates an unescaped name variable into the swal text field while html:true is enabled. This creates an XSS vulnerability if tmpl/name contains HTML characters like < or &.
Apply this fix to both:
- emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (lines 821-824):
function rmTemplate(tmpl) {
var name = tmpl.split(/[\/]+/).pop();
- swal({title:"_(Are you sure?)_",text:"_(Remove template)_: "+name,type:"warning",html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(Cancel)_"},function(){$("#rmTemplate").val(tmpl);$("#formTemplate1").submit();});
+ var safeName = $('<div/>').text(name).html(); // encode for HTML
+ swal({title:"_(Are you sure?)_",text:"_(Remove template)_: "+safeName,type:"warning",html:true,showCancelButton:true,confirmButtonText:"_(Proceed)_",cancelButtonText:"_(Cancel)_"},function(){$("#rmTemplate").val(tmpl);$("#formTemplate1").submit();});
}- emhttp/plugins/dynamix.docker.manager/javascript/addDocker.js (line 17):
function rmTemplate(tmpl) {
var name = tmpl.split(/[\/]+/).pop();
- swal({title:"Are you sure?",text:"Remove template: "+name,type:"warning",html:true,showCancelButton:true},function(){$("#rmTemplate").val(tmpl);$("#formTemplate").submit();});
+ var safeName = $('<div/>').text(name).html();
+ swal({title:"Are you sure?",text:"Remove template: "+safeName,type:"warning",html:true,showCancelButton:true},function(){$("#rmTemplate").val(tmpl);$("#formTemplate").submit();});
}🤖 Prompt for AI Agents
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (lines 821-824)
and emhttp/plugins/dynamix.docker.manager/javascript/addDocker.js (line 17): the
code injects the raw template name into swal text while html:true is enabled,
creating an XSS risk; update both locations to HTML-escape the name before
concatenation (replace &, <, >, " and ' with their HTML entities) or remove
html:true and pass the name via swal's text-only path so user input is not
interpreted as HTML, then use the escaped value in the swal call and submit
logic.
Squidly271
force-pushed
the
translations
branch
from
26a56a4 to
8d08075
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
emhttp/plugins/dynamix/ArrayOperation.page (1)
684-684: Inconsistent placeholder style remains unaddressed.Line 684 still uses sequential placeholders (
%s,%s) for the "Current operation" message, while lines 653 and 679 use positional placeholders (%1$s,%2$s) for the same message pattern. For consistency and maintainability, all three lines should use the same placeholder style.Apply this diff to make Line 684 consistent:
- <td><?=sprintf(_('Current operation %s on **%s**'),_('started'),_(my_time(_var($var,'sbUpdated',0)).day_count(_var($var,'sbSynced',0)),0))?></td></tr> + <td><?=sprintf(_('Current operation %1$s on **%2$s**'),_('started'),_(my_time(_var($var,'sbUpdated',0)).day_count(_var($var,'sbSynced',0)),0))?></td></tr>emhttp/plugins/extract_translations.php (2)
176-176: Remove unused variable (previously reported as fixed).The
$tempvariable in thepreg_match_allcall is unused—only the match count is needed. This was reportedly addressed in a previous commit but is still present in the code.Apply this diff:
- $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp); + $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text);
57-78: Add error handling for directory scanning failures.If
scandir()fails (e.g., due to permission issues), it returnsfalse, which will cause a fatal error in theforeachloop on Line 60. Add error handling to gracefully skip unreadable directories.Apply this diff:
function scanDirectory($dir, $extensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $pattern4, &$translationsJS, &$translationsPHP, &$pluralsJS, &$pluralsPHP, &$fileCountJS, &$fileCountPHP) { - $items = scandir($dir); + $items = @scandir($dir); + if ($items === false) { + echo "Warning: Could not read directory: $dir\n"; + return; + } foreach ($items as $item) {
🧹 Nitpick comments (2)
emhttp/plugins/extract_translations.php (2)
28-37: Consider more robust regex patterns for edge cases.The current patterns have limitations with nested parentheses and escaped quotes (as noted in previous reviews):
- Pattern 1 (Line 28): May fail on strings with nested unescaped parentheses like
_(outer (inner))_- Patterns 2-4 (Lines 31, 34, 37): Don't handle escaped quotes like
_("text with \" escaped")These edge cases may be rare in practice, but consider using more robust patterns if translation strings become more complex. For example, Pattern 2-4 could use
((?:[^\\\\]|\\\\.)*)?\1to handle escaped characters.
83-136: Consider removing unused parameter.The
$phpExtensionsparameter is never used within this function (it's only used inscanDirectory). Consider removing it from the function signature to reduce noise.Apply this diff:
-function processFile($filePath, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $pattern4, &$translationsJS, &$translationsPHP, &$pluralsJS, &$pluralsPHP, &$fileCountJS, &$fileCountPHP) { +function processFile($filePath, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, $pattern4, &$translationsJS, &$translationsPHP, &$pluralsJS, &$pluralsPHP, &$fileCountJS, &$fileCountPHP) {And update the call site on Line 74:
- processFile($path, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $pattern4, $translationsJS, $translationsPHP, $pluralsJS, $pluralsPHP, $fileCountJS, $fileCountPHP); + processFile($path, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, $pattern4, $translationsJS, $translationsPHP, $pluralsJS, $pluralsPHP, $fileCountJS, $fileCountPHP);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php(1 hunks)emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php(1 hunks)emhttp/plugins/dynamix.docker.manager/scripts/dockerupdate(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/languagecheck(1 hunks)emhttp/plugins/dynamix.plugin.manager/scripts/plugincheck(1 hunks)emhttp/plugins/dynamix.vm.manager/VMTemplates.page(2 hunks)emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php(1 hunks)emhttp/plugins/dynamix.vm.manager/scripts/VMAjaxCall.php(1 hunks)emhttp/plugins/dynamix.vm.manager/scripts/VMClone.php(1 hunks)emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(4 hunks)emhttp/plugins/dynamix/ArrayOperation.page(9 hunks)emhttp/plugins/dynamix/DashStats.page(10 hunks)emhttp/plugins/dynamix/DeviceInfo.page(3 hunks)emhttp/plugins/dynamix/NewPerms.page(1 hunks)emhttp/plugins/dynamix/PHPsettings.page(1 hunks)emhttp/plugins/dynamix/SysDrivers.page(1 hunks)emhttp/plugins/dynamix/include/CPUset.php(1 hunks)emhttp/plugins/dynamix/include/DashboardApps.php(2 hunks)emhttp/plugins/dynamix/include/Markdown.php(1 hunks)emhttp/plugins/dynamix/include/MarkdownExtra.php(1 hunks)emhttp/plugins/dynamix/nchan/device_list(1 hunks)emhttp/plugins/dynamix/nchan/update_2(1 hunks)emhttp/plugins/extract_translations.php(1 hunks)emhttp/plugins/translations_js.po(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php
- emhttp/plugins/dynamix/include/MarkdownExtra.php
- emhttp/plugins/dynamix/include/Markdown.php
🚧 Files skipped from review as they are similar to previous changes (14)
- emhttp/plugins/dynamix.docker.manager/include/DockerContainers.php
- emhttp/plugins/dynamix/DeviceInfo.page
- emhttp/plugins/dynamix.vm.manager/scripts/VMClone.php
- emhttp/plugins/dynamix/include/DashboardApps.php
- emhttp/plugins/dynamix.plugin.manager/scripts/plugincheck
- emhttp/plugins/dynamix/NewPerms.page
- emhttp/plugins/dynamix.vm.manager/scripts/VMAjaxCall.php
- emhttp/plugins/dynamix/PHPsettings.page
- emhttp/plugins/dynamix/nchan/device_list
- emhttp/plugins/dynamix/nchan/update_2
- emhttp/plugins/dynamix.plugin.manager/scripts/languagecheck
- emhttp/plugins/dynamix/DashStats.page
- emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php
- emhttp/plugins/translations_js.po
🧰 Additional context used
🧬 Code graph analysis (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (2)
emhttp/plugins/dynamix.vm.manager/scripts/VMAjaxCall.php (1)
write(26-30)emhttp/plugins/dynamix.vm.manager/scripts/VMClone.php (1)
write(26-30)
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
83-83: Avoid unused parameters such as '$phpExtensions'. (undefined)
(UnusedFormalParameter)
176-176: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (14)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
1842-1842: LGTM! Translation formatting improvement.The change from string concatenation to
sprintfwith a placeholder is a proper i18n best practice. This allows translators to adjust word order and properly localize the error message while maintaining the dynamic error value.emhttp/plugins/dynamix.vm.manager/VMTemplates.page (3)
67-67: LGTM: Punctuation moved inside translation wrapper.Moving the question mark inside the translation wrapper is correct for localization best practices, allowing translators to adjust punctuation based on target language conventions.
73-73: LGTM: Consistent punctuation cleanup.Removing the external question marks from the "File exists" dialog titles improves translation consistency and provides cleaner strings for localization.
Also applies to: 90-90
76-76: LGTM: Spelling error corrected.The spelling fix from "occured" to "occurred" is correct, and the punctuation cleanup on lines 76 and 80 is consistent with the localization improvements throughout the file.
Also applies to: 80-80, 98-98
emhttp/plugins/dynamix/ArrayOperation.page (4)
88-90: LGTM! Good translation practice.The function now correctly returns translated strings, aligning with the PR's localization objectives.
92-95: LGTM! Proper plural-aware translation.Using
ngettextcorrectly handles singular/plural forms across different languages, which is essential for proper internationalization.
572-572: LGTM! Correct sprintf usage for dynamic translations.These lines properly use
sprintfto interpolate dynamic content into translated strings, following best practices for localization.Also applies to: 619-619, 624-624, 718-718
653-653: LGTM! Consistent positional placeholders.These lines correctly use explicit positional placeholders (
%1$s,%2$s), which is portable across PHP versions and aligns with the past review recommendations.Also applies to: 679-679
emhttp/plugins/dynamix.docker.manager/scripts/dockerupdate (1)
61-61: LGTM! Good localization practice.Using explicit positional placeholders (
%1$s,%2$s) instead of implicit ordering improves translation flexibility, allowing translators to reorder arguments based on language-specific grammar rules.emhttp/plugins/dynamix/SysDrivers.page (1)
82-82: LGTM! Correct punctuation placement for translations.Moving the question mark inside the translation string is proper localization practice, as different languages may have different punctuation rules and spacing conventions around question marks.
emhttp/plugins/dynamix/include/CPUset.php (1)
88-89: LGTM! Fixes double-translation issue.Applying the translation function at variable assignment (Line 88) and then outputting the translated value directly (Line 89) is correct. This prevents the previous issue where translation might have been applied twice, which would break the translation lookup.
emhttp/plugins/extract_translations.php (3)
202-328: LGTM! Pattern processing functions are well-structured.The four pattern processing functions follow a consistent structure with proper:
- Offset-based line number calculation
- Appropriate use of
stripslashesto handle escaped quotes- Empty string filtering
- Location tracking with file:line format
333-405: LGTM! PO file generation is correct.The function properly:
- Generates valid gettext PO headers with appropriate metadata
- Escapes backslashes and special characters correctly (Line 365, 387-388)
- Handles both regular translations and plural forms
- Sorts entries alphabetically for consistency
- Includes file location references for each translation
407-484: LGTM! Main execution flow is well-organized.The script provides:
- Clear progress reporting during scanning
- Separate statistics for JavaScript and PHP/HTML files
- Conditional file generation (only when translations exist)
- Detailed output with sample entries for verification
- Proper error reporting if file writes fail
| if (p) { | ||
| $.post('/plugins/dynamix.vm.manager/include/VMajax.php',{action:'vm-template-save',name:name,template:template,replace:"yes"},function($return){ | ||
| if ($return.success == false) swal({title:"_(Error occured)_?",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | ||
| if ($return.success == false) swal({title:"_(Error occurred)_?",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); |
There was a problem hiding this comment.
Fix inconsistent punctuation in error dialog title.
This line still has an external question mark after the translation wrapper, while all other "Error occurred" instances (lines 76, 80, 98) have been updated to remove it. This inconsistency should be corrected for uniform localization.
Apply this diff to fix the inconsistency:
- if ($return.success == false) swal({title:"_(Error occurred)_?",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"});
+ if ($return.success == false) swal({title:"_(Error occurred)_",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($return.success == false) swal({title:"_(Error occurred)_?",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); | |
| if ($return.success == false) swal({title:"_(Error occurred)_",text:"Action error " + name.split(".")[0] + " " + $return.error ,type:'warning',html:true,showCancelButton:true,confirmButtonText:"_(OK)_"}); |
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix.vm.manager/VMTemplates.page around line 93, the swal
error dialog title uses an inconsistent external question mark after the
translation wrapper ("_(Error occurred)_?"); remove the trailing question mark
so the title matches other instances ("_(Error occurred)_"), i.e., update the
title string to use only the translation wrapper without any added punctuation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
emhttp/plugins/extract_translations.php (3)
176-176: Remove unused variable$temp.The
$tempvariable receives the matches array frompreg_match_all()but is never used. Only the return value (match count) is assigned to$specialChars.Apply this diff:
- $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text, $temp); + $specialChars = preg_match_all('/[\/\^\$\*\+\?\{\}\[\]\\\|]/', $text);Note: This was marked as addressed in a previous review but the code still contains the unused variable.
Based on static analysis hints.
31-34: Patterns fail on escaped quotes within strings.Both Pattern 2 and Pattern 3 use
(?:(?!\1).)*which stops at any occurrence of the closing quote character, even if it's escaped with a backslash. For example,_("text with \" escaped")will only match"text with \"without the closing quote.Apply this diff to handle escaped quotes:
// Pattern 2: Match _('text') or _("text") - function call pattern -$pattern2 = '/_\((["\'])((?:(?!\1).)*)\1\)/s'; +$pattern2 = '/_\((["\'])((?:[^\\\\]|\\\\.)*?)\1\)/s'; // Pattern 3: Match tr('text') or tr("text") - function call pattern -$pattern3 = '/\btr\((["\'])((?:(?!\1).)*)\1\)/s'; +$pattern3 = '/\btr\((["\'])((?:[^\\\\]|\\\\.)*?)\1\)/s';This pattern matches either non-backslash characters or a backslash followed by any character (escaped sequences).
57-78: Add error handling forscandirfailure.
scandir()returnsfalseon error (e.g., permission denied), which would cause a fatal error in theforeachloop on line 60. This would crash the entire extraction process instead of gracefully skipping unreadable directories.Apply this diff:
function scanDirectory($dir, $extensions, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $pattern4, &$translationsJS, &$translationsPHP, &$pluralsJS, &$pluralsPHP, &$fileCountJS, &$fileCountPHP) { - $items = scandir($dir); + $items = @scandir($dir); + if ($items === false) { + echo "Warning: Could not read directory: $dir\n"; + return; + } foreach ($items as $item) {
🧹 Nitpick comments (1)
emhttp/plugins/extract_translations.php (1)
83-136: Remove unused parameter$phpExtensions.The
$phpExtensionsparameter is never used in this function. The function only checks if the extension is in$jsExtensions(line 104) to determine the file type; everything else is implicitly treated as PHP.Apply this diff to remove the unused parameter:
-function processFile($filePath, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $pattern4, &$translationsJS, &$translationsPHP, &$pluralsJS, &$pluralsPHP, &$fileCountJS, &$fileCountPHP) { +function processFile($filePath, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, $pattern4, &$translationsJS, &$translationsPHP, &$pluralsJS, &$pluralsPHP, &$fileCountJS, &$fileCountPHP) {Then update the call on line 74:
- processFile($path, $ext, $jsExtensions, $phpExtensions, $pattern1, $pattern2, $pattern3, $pattern4, $translationsJS, $translationsPHP, $pluralsJS, $pluralsPHP, $fileCountJS, $fileCountPHP); + processFile($path, $ext, $jsExtensions, $pattern1, $pattern2, $pattern3, $pattern4, $translationsJS, $translationsPHP, $pluralsJS, $pluralsPHP, $fileCountJS, $fileCountPHP);Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/include/Translations.php(2 hunks)emhttp/plugins/extract_translations.php(1 hunks)emhttp/plugins/translations_js.po(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/translations_js.po
🧰 Additional context used
🪛 PHPMD (2.15.0)
emhttp/plugins/extract_translations.php
83-83: Avoid unused parameters such as '$phpExtensions'. (undefined)
(UnusedFormalParameter)
176-176: Avoid unused local variables such as '$temp'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (1)
emhttp/plugins/dynamix/include/Translations.php (1)
18-21: LGTM: Session lock released early.Adding
session_write_close()immediately aftersession_start()is a good practice that prevents session file locking from blocking concurrent requests. Since this code only reads session data (the locale), releasing the lock early improves concurrency.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php(4 hunks)emhttp/plugins/dynamix/ArrayOperation.page(9 hunks)emhttp/plugins/dynamix/DashStats.page(10 hunks)emhttp/plugins/dynamix/DeviceInfo.page(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/dynamix/DeviceInfo.page
- emhttp/plugins/dynamix.vm.manager/templates/Custom.form.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, when both translation and HTML escaping are needed, always apply translation (_()) first, then escape the translated text with htmlspecialchars() to ensure proper handling of translation placeholders.
Learnt from: zackspear
Repo: unraid/webgui PR: 2185
File: emhttp/plugins/dynamix/Shares.page:5-15
Timestamp: 2025-05-08T18:20:16.531Z
Learning: In the Unraid WebGUI codebase, translation strings can use Markdown syntax (like `**text**` for bold) which will be properly parsed and rendered in the UI, rather than requiring HTML tags.
Learnt from: zackspear
Repo: unraid/webgui PR: 2185
File: emhttp/plugins/dynamix.vm.manager/VMs.page:23-23
Timestamp: 2025-05-08T18:20:08.009Z
Learning: In the Unraid webGUI, text within translation strings (_() function) is parsed as Markdown, so Markdown syntax like `**bold**` is appropriate and will be properly rendered.
Learnt from: zackspear
Repo: unraid/webgui PR: 2185
File: emhttp/plugins/dynamix.docker.manager/Docker.page:23-23
Timestamp: 2025-05-08T18:20:24.450Z
Learning: In the Unraid WebGUI codebase, messages within the `_()` translation function support markdown syntax (e.g., `**text**` for bold) which will be properly parsed and rendered when displayed.
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix/include/publish.php:28-36
Timestamp: 2025-08-13T03:13:53.283Z
Learning: In the unraid/webgui codebase, PR #2333 by Squidly271 supersedes earlier work that already addressed all legacy publish() calls using the old overloaded syntax. There are no remaining legacy calls that need backward compatibility handling in the publish() function.
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-05-31T05:10:13.120Z
Learning: Squidly271 prefers not to receive CodeRabbit reviews or feedback while PRs are in draft status. Only provide reviews after PRs are moved out of draft.
Learnt from: Squidly271
Repo: unraid/webgui PR: 0
File: :0-0
Timestamp: 2025-09-03T07:28:01.902Z
Learning: Squidly271's preference for not reviewing drafts is specifically because they find reviews annoying during active development phases with multiple commits, but they do want reviews once the PR is ready and more stable.
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, button text should be wrapped in _() for translation support and htmlspecialchars() for security, with a preference for using ternary operators for simple conditional text assignments.
Applied to files:
emhttp/plugins/dynamix/DashStats.pageemhttp/plugins/dynamix/ArrayOperation.page
| $sbUpdate = _var($var,'mdResyncDt') ? $sbSynced : _var($var,'sbUpdated');?> | ||
| <tr><td></td><td><input type="button" value="_(History)_" onclick="parityHistory()"></td> | ||
| <td><?=sprintf(_('Current operation %s on **%s**'),(_var($var,'mdResyncDt')?_('started'):_('paused')),_(my_time($sbUpdate).day_count($sbUpdate),0))?></td></tr> | ||
| <td><?=(_('Current operation %1$s on **%2$s**'),(_var($var,'mdResyncDt')?_('started'):_('paused')),_(my_time($sbUpdate).day_count($sbUpdate),0))?></td></tr> |
There was a problem hiding this comment.
Missing sprintf call causes placeholder substitution failure.
Line 685 is missing the sprintf call before the translation function, which means the placeholders %1$s and %2$s will not be substituted with the actual values. This will result in the raw translation string with placeholders being displayed to users.
Apply this diff to fix the issue:
- <td><?=(_('Current operation %1$s on **%2$s**'),(_var($var,'mdResyncDt')?_('started'):_('paused')),_(my_time($sbUpdate).day_count($sbUpdate),0))?></td></tr>
+ <td><?=sprintf(_('Current operation %1$s on **%2$s**'),(_var($var,'mdResyncDt')?_('started'):_('paused')),_(my_time($sbUpdate).day_count($sbUpdate),0))?></td></tr>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <td><?=(_('Current operation %1$s on **%2$s**'),(_var($var,'mdResyncDt')?_('started'):_('paused')),_(my_time($sbUpdate).day_count($sbUpdate),0))?></td></tr> | |
| <td><?=sprintf(_('Current operation %1$s on **%2$s**'),(_var($var,'mdResyncDt')?_('started'):_('paused')),_(my_time($sbUpdate).day_count($sbUpdate),0))?></td></tr> |
🤖 Prompt for AI Agents
emhttp/plugins/dynamix/ArrayOperation.page around line 685: the translation call
is passing placeholders but not using sprintf, so %1$s and %2$s are not being
substituted; wrap the translated string in sprintf and pass the two computed
values (the started/paused status and the formatted date string) as arguments to
sprintf so the placeholders are replaced correctly.
2 participants
Summary by CodeRabbit
New Features
Internationalization
Bug Fixes / Chores
✏️ Tip: You can customize this high-level summary in your review settings.