Skip to content

Commit e155caf

Browse files
committed
Fix merge_strings crash on nested-dictionary .strings value
`merge_strings` bookkeeps keys via `plutil` but rewrites them by re-tokenizing the raw lines through `StringsFileValidationHelper.prefix_keys`, whose flat-`.strings` grammar has no `{`. A valid file like `"k" = { a = b; };` clears the `:text` format gate, reaches `prefix_keys`, and raised `Invalid character` — aborting a merge that previously copied the line through. Fail soft: rescue the tokenizer and copy the file's lines through unprefixed with a `UI.important` warning, mirroring the `scan_for_duplicate_keys` `:unscannable` path. Flip the regression test to assert the graceful degradation (no raise, warning emitted, line survives).
1 parent 2950cea commit e155caf

3 files changed

Lines changed: 26 additions & 6 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ _None_
1818
- `StringsFileValidationHelper.find_duplicated_keys` now also accepts comments placed *between* the tokens of a statement (e.g. `"key" /* note */ = "value";`), which `plutil` allows, instead of raising `Invalid character` on the `/`. [#741]
1919
- `ios_lint_localizations`' `check_duplicate_keys` no longer crashes the lane on a file that parses as a property list but isn't a flat `.strings` (e.g. a nested-dictionary value); it now warns via `UI.important` and skips that file. [#741]
2020
- `L10nHelper.merge_strings` now applies the key prefix via a comment-aware tokenizer, so it correctly prefixes unquoted keys containing `. - $ : /`, lines with an *unquoted* value (e.g. `CFBundleName = WordPress;`), and keys sitting behind an inter-token `.strings` comment (e.g. `CFBundleName /* note */ = WordPress;`) — matching the grammar `plutil` accepts. Previously the line-based matcher left those keys written to the merged file without the prefix while still bookkeeping them *with* it, leaving the output inconsistent with the reported keys (which could resurface the very collisions the prefix avoids and break downstream key extraction). [#741]
21+
- `L10nHelper.merge_strings` no longer crashes on a `:text` `.strings` file whose value is a nested dictionary (`"k" = { … };`) — valid input `plutil` accepts but the flat-`.strings` prefixing tokenizer can't rewrite. It now copies that file's lines through unprefixed with a `UI.important` warning instead of aborting the whole merge, mirroring the fail-soft behavior on the duplicate-key scanner path. [#750]
2122

2223
### Internal Changes
2324

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ def self.read_utf8_lines(file)
7878
# @note The method is able to handle input files which are using different encodings,
7979
# guessing the encoding of each input file using the BOM (and defaulting to UTF8).
8080
# The generated file will always be in utf-8, by convention.
81+
# @note If a file's keys can't be prefixed (e.g. it holds a nested-dictionary value, `"k" = { … };`,
82+
# which `plutil` accepts but the flat-`.strings` tokenizer can't rewrite), its lines are copied
83+
# through unprefixed with a warning rather than aborting the whole merge.
8184
#
8285
# @raise [RuntimeError] If one of the paths provided is not in text format (but XML or binary instead), or if any of the files are missing.
8386
#
@@ -104,7 +107,17 @@ def self.merge_strings(paths:, output_path:)
104107
# sit (e.g. `CFBundleName /* note */ = WordPress;`) and `key = value`-looking text inside a comment is
105108
# left alone — keeping the written keys consistent with the (`plutil`-derived) keys bookkept above.
106109
lines = read_utf8_lines(input_file)
107-
lines = Fastlane::Helper::Ios::StringsFileValidationHelper.prefix_keys(lines: lines, prefix: prefix)
110+
begin
111+
lines = Fastlane::Helper::Ios::StringsFileValidationHelper.prefix_keys(lines: lines, prefix: prefix)
112+
rescue StandardError => e
113+
# `plutil` accepts flat-`.strings` constructs the tokenizer doesn't — most notably a nested-dictionary
114+
# value (`"k" = { … };`), which parses fine (so the file clears the `:text` gate above) yet exposes no
115+
# flat `key = value` for `prefix_keys` to rewrite, so it raises on the `{`. Fail soft: copy this file's
116+
# lines through unprefixed rather than aborting the whole merge — mirroring the scanner path, where
117+
# `scan_for_duplicate_keys` returns `:unscannable` instead of crashing the lane. `lines` is untouched
118+
# by the raise (the assignment above never completes), so it still holds the original file contents.
119+
UI.important("Could not add prefix `#{prefix}` to the keys in `#{input_file}` (#{e.message}); copying its lines through unprefixed.")
120+
end
108121
lines.each { |line| tmp_file.write(line) }
109122
tmp_file.write("\n")
110123
end

spec/ios_l10n_helper_spec.rb

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -160,19 +160,25 @@ def file_encoding(path)
160160
end
161161
end
162162

163-
it 'does not crash merging a text `.strings` file whose value is a nested dictionary' do
164-
# Regression: `merge_strings` bookkeeps keys via `plutil` (which parses a nested-dictionary value
165-
# fine) but re-tokenizes the raw lines through `prefix_keys`, which raises `Invalid character` on
166-
# the `{`. Such a file is still `:text` and passes the format gate, so a public `ios_merge_strings_files`
167-
# run that previously copied the line through now aborts. It should degrade gracefully instead of raising.
163+
it 'copies a nested-dictionary value through unprefixed (with a warning) instead of crashing the merge' do
164+
# Regression: `merge_strings` bookkeeps keys via `plutil` (which parses a nested-dictionary value fine)
165+
# but rewrites keys by re-tokenizing the raw lines through `prefix_keys`, whose flat-`.strings` grammar
166+
# has no `{` — so it raised `Invalid character`. Such a file is still `:text` and clears the format gate,
167+
# so a public `ios_merge_strings_files` run that previously copied the line through would abort. It now
168+
# degrades gracefully — copying the un-prefixable line verbatim and warning — like the scanner path.
168169
content = %("k" = { a = b; };\n)
169170
Dir.mktmpdir('a8c-release-toolkit-l10n-helper-tests-') do |tmp_dir|
170171
input_file = File.join(tmp_dir, 'InfoPlist.strings')
171172
File.write(input_file, content)
172173
output_file = File.join(tmp_dir, 'output.strings')
174+
175+
expect(FastlaneCore::UI).to receive(:important).with(a_string_including('Could not add prefix `pfx.`').and(a_string_including('unprefixed')))
173176
expect do
174177
described_class.merge_strings(paths: { input_file => 'pfx.' }, output_path: output_file)
175178
end.not_to raise_error
179+
180+
# The nested-dict line survives verbatim — unprefixed, since it has no flat `key = value` to rewrite.
181+
expect(File.read(output_file)).to include('"k" = { a = b; };')
176182
end
177183
end
178184

0 commit comments

Comments
 (0)