Harden language catalog repair and syntax validation - #1114
Conversation
|
I think we'd be better off doing a one off fix on those files with invalid syntax, and just aborting/breaking within update-languages.sh if there is invalid syntax in the future (as in, stop trying to be clever fixing up invalid syntax). |
|
Thanks, agreed. I simplified languages/language-update.sh so it only validates catalog PHP syntax and aborts the scan on the first invalid file; the 24 affected catalogs remain a one-off correction. The optional repair logic is now isolated in ADDITIONS/fix-language-syntax.php and is never called by the main script or CI. The updated GitHub CI run passed lint, PHP 8.2 through 8.5, MySQL, and PostgreSQL. |
| function next_backup_filename(string $file): string | ||
| { | ||
| $backup = "$file.bak"; | ||
| for ($suffix = 2; file_exists($backup); $suffix++) { |
There was a problem hiding this comment.
file_exists comes with some funny details:
- it returns false on dead symlinks (so you can have symlinks attack even without any race involved)
- it caches the result
This is probably not very problematic for this script, but maybe you should at least add a comment (in the file header) like "don't run this script with files in directories that are writeable by others (like /tmp/)"
There was a problem hiding this comment.
the cache only lasts as long as the php process, so probably not a problem here.
I just don't see any need to have the 'fix-language-syntax.php' script in git, that's all.
What we do probably need is to just run 'php -l' on each language file (or include languages/ in what psalm or parallel-lint checks).
| } | ||
|
|
||
| $backup = next_backup_filename($file); | ||
| if (!copy($file, $backup)) { |
There was a problem hiding this comment.
This could be affected by a race condition (or even without a race condition on dead symlinks).
Again, <copy&paste>this is probably not very problematic for this script, but maybe you should at least add a comment like "don't run this script with files in directories that are writeable by others (like /tmp/)"</copy&paste>
| $written = file_put_contents($file, $fixedSource); | ||
| if ($written !== strlen($fixedSource)) { | ||
| if (!copy($backup, $file)) { | ||
| fwrite(STDERR, "*** $file: write and backup restoration failed ***\n"); |
There was a problem hiding this comment.
Mentioning $backup as part of this error message would be helpful - assuming $backup contains the previous state, users can manually restore it.
I introduced these syntax errors when I updated the *.lang files two days ago. The most obvious (and most boring) issue was that some strings were missing in most language files. There were also some texts that were changed, and not yet translated in most languages. Instead of just adding a comment, I replaced the not-yet-translated texts with the new text using some script magic, and accidentally killed the semicolon. Sorry for that! (I'm not sure if my translation updates + fixes from this PR are also needed in the 4.0 branch - feel free to review and backport them.) |
3 participants
Summary
ADDITIONS/fix-language-syntax.phpmaintenance helperRoot cause
The tokenizer-based parser accepted token streams from syntactically invalid catalogs. Missing semicolons caused adjacent
$PALANGassignments to be treated as one statement, so--patchcould insert duplicate keys.Behavior
languages/language-update.shdoes not attempt automatic repair. It validates the complete PHP source before comparing translation keys and aborts the scan on the first syntax error.The standalone
ADDITIONS/fix-language-syntax.phphelper is only run explicitly by an administrator. It handles a limited set of unambiguous missing-semicolon errors, validates the complete repaired source before writing, and creates.bak,.bak2, and later numbered backups without overwriting them. It is not called bylanguage-update.shor CI.Read-only catalog checks return status 1 when any catalog has missing or obsolete keys.
--patchcan return 0 after adding every missing key, but still returns 1 when obsolete keys remain. The GitHub Actions lint job runs the complete catalog check.Validation
git diff --checkpassedThe full PHPUnit suite was not run locally because dependencies are unavailable in this checkout. Regression tests are included for CI.
Fixes #1112.
Fixes #1113.