Skip to content

Commit cacc5ac

Browse files
committed
Add ios_lint_localization_placeholder_changes action
A temporal localization guardrail: compares two versions of a base-language `.strings` file and fails when an existing key's value changes its format placeholders (count, position, or argument type), which silently breaks every translation filed under that key. - `StringPlaceholdersHelper` — a pure-Ruby, platform-agnostic primitive that reduces a value's printf/`NSString` specifiers to a canonical signature. - `ios_lint_localization_placeholder_changes` — the iOS action wrapping it, with fail-closed guards for duplicate keys and mixed positional/non-positional specifiers. Builds on the `.strings` parsing and `scan_for_duplicate_keys` helper from #741.
1 parent 535c167 commit cacc5ac

5 files changed

Lines changed: 1085 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ _None_
1111
### New Features
1212

1313
- Added `find_or_create_pull_request` action and `GithubHelper#find_pull_request`: returns the URL of the open Pull Request for a head branch, creating one only if none exists yet. Useful for "rolling" automations (e.g. a daily translations or dependency-update job) that force-push the same head branch on every run. [#733]
14+
- Added the platform-agnostic `StringPlaceholdersHelper` primitive (`placeholder_signature`, `placeholders_compatible?`, `incompatible_placeholder_changes`, `mixed_operators?`) and the `ios_lint_localization_placeholder_changes` action: fails when an existing localization key's source-language value changes its format placeholders — count, position, or argument type — between two versions of the base `.strings` file. Such a change silently breaks every existing translation filed under that key. Complements `ios_lint_localizations` on the temporal (base↔base across versions) axis. The action also aborts (by default, via `check_duplicate_keys`) when either input file defines a key more than once, since `plutil` silently keeps only the last value and a duplicate could otherwise hide a real placeholder change, and always aborts when a source value mixes positional (`%1$@`) and non-positional (`%@`) placeholders, which is invalid and makes the placeholder shape impossible to compare reliably. [#738]
1415

1516
### Bug Fixes
1617

Lines changed: 275 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,275 @@
1+
# frozen_string_literal: true
2+
3+
require 'fastlane/action'
4+
require_relative '../../helper/string_placeholders_helper'
5+
require_relative '../../helper/ios/ios_l10n_helper'
6+
require_relative '../../helper/ios/ios_strings_file_validation_helper'
7+
8+
module Fastlane
9+
module Actions
10+
class IosLintLocalizationPlaceholderChangesAction < Action
11+
def self.run(params)
12+
# Parse and validate both inputs first, so a genuinely invalid file (a non-dictionary plist, an
13+
# unparseable file) surfaces an accurate input error rather than tripping the duplicate-key
14+
# scanner below and reporting it as a duplicate-check failure.
15+
old_strings = read_strings(params[:old_file])
16+
new_strings = read_strings(params[:new_file])
17+
18+
# Duplicate keys must be caught *before the comparison*: `plutil` silently keeps only the last
19+
# value for a duplicated key, so a duplicate could hide a real placeholder change. They always
20+
# abort — independently of `abort_on_violations`, which only governs *placeholder changes* —
21+
# because the comparison itself is unreliable until each key is defined exactly once.
22+
if params[:check_duplicate_keys]
23+
duplicate_keys = duplicate_keys_by_file(params)
24+
unless duplicate_keys.empty?
25+
report_duplicate_keys(duplicate_keys)
26+
UI.abort_with_message!(duplicate_keys_abort_message(duplicate_keys))
27+
end
28+
end
29+
30+
# A value mixing positional (`%1$@`) and non-positional (`%@`) placeholders is invalid —
31+
# printf/`NSString` require all-or-nothing positional — so its placeholder signature has no
32+
# well-defined argument order and can't be compared. Always abort (like duplicate keys, and
33+
# independently of `abort_on_violations`): the comparison itself is unreliable for such a value.
34+
mixed_keys = mixed_operator_keys_by_file(params[:old_file] => old_strings, params[:new_file] => new_strings)
35+
unless mixed_keys.empty?
36+
report_mixed_operators(mixed_keys)
37+
UI.abort_with_message!(mixed_operators_abort_message(mixed_keys))
38+
end
39+
40+
violations = Fastlane::Helper::StringPlaceholdersHelper.incompatible_placeholder_changes(old_strings, new_strings)
41+
report(violations)
42+
43+
UI.abort_with_message!(abort_message(violations.length)) if !violations.empty? && params[:abort_on_violations]
44+
45+
violations
46+
end
47+
48+
# Reads a `.strings` file into a `{ key => value }` hash. A malformed file is an *input* error
49+
# (like a missing file), so surface a clean, user-facing message naming it rather than letting a
50+
# raw `plutil` stderr dump escape the lane. `read_strings_file_as_hash` shells out to `plutil`,
51+
# which handles text/XML/binary plists, so this only trips on genuinely unparseable input.
52+
def self.read_strings(path)
53+
strings =
54+
begin
55+
Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: path)
56+
rescue StandardError => e
57+
UI.user_error!("Could not parse `#{File.basename(path)}` as a `.strings` file. #{e.message.strip}")
58+
end
59+
return strings if strings.is_a?(Hash)
60+
61+
UI.user_error!("`#{File.basename(path)}` is not a valid `.strings` file (expected a dictionary of key/value pairs).")
62+
end
63+
64+
def self.report(violations)
65+
if violations.empty?
66+
UI.success('No incompatible placeholder changes found between the two versions of the strings file.')
67+
return
68+
end
69+
70+
violations.each do |violation|
71+
UI.error <<~MSG
72+
`#{violation[:key]}` changed its format placeholders incompatibly:
73+
was: #{violation[:old].inspect} [#{describe_signature(violation[:old_signature])}]
74+
now: #{violation[:new].inspect} [#{describe_signature(violation[:new_signature])}]
75+
MSG
76+
end
77+
end
78+
79+
def self.describe_signature(signature)
80+
signature.empty? ? 'no placeholders' : signature
81+
end
82+
83+
def self.abort_message(count)
84+
<<~MSG
85+
#{count} localization key(s) changed their format placeholders incompatibly.
86+
87+
Existing translations are keyed by the string's key, not its English text, so changing
88+
the placeholders of an existing key would silently break every translation for that key.
89+
Give the changed copy a brand new key instead, so the previous translations stay valid
90+
for the previous key and the new copy gets translated from scratch.
91+
MSG
92+
end
93+
94+
# Finds duplicate keys in the old and new files, returned as `{ path => { key => [lines] } }`,
95+
# only including files that actually have duplicates. Format detection and scanning are delegated
96+
# to `StringsFileValidationHelper.scan_for_duplicate_keys`; `duplicate_keys_in` maps each outcome.
97+
def self.duplicate_keys_by_file(params)
98+
[params[:old_file], params[:new_file]].uniq.each_with_object({}) do |path, result|
99+
duplicates = duplicate_keys_in(path)
100+
result[path] = duplicates unless duplicates.empty?
101+
end
102+
end
103+
104+
def self.duplicate_keys_in(path)
105+
# `run` has already parsed both files via `read_strings`, so the plist is known-valid here —
106+
# `assume_valid: true` skips the redundant `plutil -lint` inside the format check.
107+
status, payload = Fastlane::Helper::Ios::StringsFileValidationHelper.scan_for_duplicate_keys(file: path, assume_valid: true)
108+
case status
109+
when :scanned then payload
110+
# A non-`:text` plist (xml/binary) has no tokenizer here, but `plutil` collapses any duplicate to
111+
# its last value when parsing it, and toolkit-generated XML is built from a Hash and is dup-free
112+
# anyway — so there's nothing to catch.
113+
when :unsupported_format then {}
114+
# A `:text` file `plutil` parses but our scanner can't tokenize (e.g. a nested dictionary/array
115+
# value — valid old-style plist, but not a flat `.strings`). We can't guarantee each key is defined
116+
# exactly once, and `plutil` keeps only the *last* value for a duplicate, so proceeding could
117+
# compare the wrong value and miss a real placeholder change. Fail closed rather than compare blindly.
118+
when :unscannable then UI.abort_with_message!(unverifiable_duplicates_abort_message(path, payload))
119+
end
120+
end
121+
122+
# Emits one `UI.error` per file in a `{ path => { key => detail } }` hash: a headline naming the
123+
# file and the offending-key count, then one indented line per key formatted by the given block.
124+
def self.report_per_file(by_path, headline:, &entry)
125+
by_path.each do |path, entries|
126+
UI.error <<~MSG
127+
`#{File.basename(path)}` #{headline.call(entries.count)}:
128+
#{entries.map(&entry).join("\n")}
129+
MSG
130+
end
131+
end
132+
133+
def self.report_duplicate_keys(duplicate_keys)
134+
report_per_file(duplicate_keys, headline: ->(count) { "defines #{count} key(s) more than once" }) do |key, lines|
135+
" `#{key}` at lines #{lines.join(', ')}"
136+
end
137+
end
138+
139+
def self.duplicate_keys_abort_message(duplicate_keys)
140+
count = duplicate_keys.values.sum(&:count)
141+
<<~MSG
142+
#{count} localization key(s) are defined more than once in the source strings.
143+
144+
Parsing `.strings` files relies on `plutil`, which silently keeps only the *last* value for
145+
a duplicated key. This check would therefore compare against the wrong value and could miss a
146+
real, translation-breaking placeholder change — the exact failure it exists to catch. Define
147+
each key exactly once and regenerate the file before re-running this check.
148+
MSG
149+
end
150+
151+
def self.unverifiable_duplicates_abort_message(path, error_message)
152+
<<~MSG
153+
Could not verify `#{File.basename(path)}` for duplicate keys: #{error_message.strip}
154+
155+
Parsing `.strings` files relies on `plutil`, which silently keeps only the *last* value for a
156+
duplicated key, so this action guards against a duplicate hiding a real placeholder change. The
157+
file parses as text but our duplicate-key scanner couldn't tokenize it, so that guarantee can't
158+
be established and the comparison would be unreliable. Make sure the file is a flat `.strings`
159+
(each line a `key = value;` pair), or pass `check_duplicate_keys: false` to skip this check.
160+
MSG
161+
end
162+
163+
# Finds keys whose value mixes positional and non-positional placeholders, per file, returned as
164+
# `{ path => { key => value } }`, only including files that have any.
165+
def self.mixed_operator_keys_by_file(strings_by_path)
166+
strings_by_path.each_with_object({}) do |(path, strings), result|
167+
mixed = strings.select { |_key, value| Fastlane::Helper::StringPlaceholdersHelper.mixed_operators?(value) }
168+
result[path] = mixed unless mixed.empty?
169+
end
170+
end
171+
172+
def self.report_mixed_operators(mixed_keys)
173+
report_per_file(mixed_keys, headline: ->(count) { "has #{count} key(s) that mix positional and non-positional placeholders" }) do |key, value|
174+
" `#{key}`: #{value.inspect}"
175+
end
176+
end
177+
178+
def self.mixed_operators_abort_message(mixed_keys)
179+
count = mixed_keys.values.sum(&:count)
180+
<<~MSG
181+
#{count} localization key(s) mix positional (`%1$@`) and non-positional (`%@`) placeholders in a single value.
182+
183+
Mixing the two in one format string is invalid — `printf`/`NSString` require either all-positional
184+
or all-non-positional — so the placeholder shape can't be compared reliably. Give every placeholder
185+
an explicit `%N$` position, or none, and regenerate the file before re-running this check.
186+
MSG
187+
end
188+
189+
#####################################################
190+
# @!group Documentation
191+
#####################################################
192+
193+
def self.description
194+
'Checks that no existing localization key changed its format placeholders between two versions of a source `.strings` file'
195+
end
196+
197+
def self.details
198+
<<~DETAILS
199+
Compares two versions (old vs new) of the same base-language `.strings` file and reports any
200+
key — present in both — whose format placeholders (`%@`, `%1$d`, …) changed in count, position,
201+
or argument type.
202+
203+
This is the temporal counterpart to `ios_lint_localizations` (which compares each translation
204+
against the base language at a single point in time). Translations are filed by key, not by
205+
English text, so changing the placeholders of an existing key silently breaks every translation
206+
of that key. New and removed keys are ignored on purpose: copy that needs different placeholders
207+
is expected to land under a brand new key.
208+
209+
By default it also aborts if either input file defines the same key more than once: `plutil`
210+
silently keeps only the last value for a duplicated key, which would let a duplicate hide a
211+
real placeholder change. Disable this with `check_duplicate_keys: false`.
212+
213+
Note: parsing `.strings` files relies on `plutil`, so this action only runs on macOS.
214+
DETAILS
215+
end
216+
217+
def self.available_options
218+
[
219+
FastlaneCore::ConfigItem.new(
220+
key: :old_file,
221+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_OLD_FILE',
222+
description: 'Path to the previous version of the base-language `.strings` file',
223+
optional: false,
224+
type: String,
225+
verify_block: proc do |value|
226+
UI.user_error!("`old_file` not found at path: #{value}") unless File.exist?(value)
227+
end
228+
),
229+
FastlaneCore::ConfigItem.new(
230+
key: :new_file,
231+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_NEW_FILE',
232+
description: 'Path to the newly-generated version of the base-language `.strings` file',
233+
optional: false,
234+
type: String,
235+
verify_block: proc do |value|
236+
UI.user_error!("`new_file` not found at path: #{value}") unless File.exist?(value)
237+
end
238+
),
239+
FastlaneCore::ConfigItem.new(
240+
key: :abort_on_violations,
241+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_ABORT',
242+
description: 'Should we abort the rest of the lane with a global error if any incompatible changes are found?',
243+
optional: true,
244+
default_value: true,
245+
type: Boolean
246+
),
247+
FastlaneCore::ConfigItem.new(
248+
key: :check_duplicate_keys,
249+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_CHECK_DUPLICATE_KEYS',
250+
description: 'Abort if either input file defines the same key more than once. `plutil` keeps only the last value for a duplicated key, which would otherwise let a duplicate silently hide a real placeholder change',
251+
optional: true,
252+
default_value: true,
253+
type: Boolean
254+
),
255+
]
256+
end
257+
258+
def self.return_type
259+
:array
260+
end
261+
262+
def self.return_value
263+
'An array of incompatible changes, each a hash with the keys `:key`, `:old`, `:new`, `:old_signature` and `:new_signature`. Empty if there are no incompatible changes.'
264+
end
265+
266+
def self.authors
267+
['Automattic']
268+
end
269+
270+
def self.is_supported?(platform)
271+
%i[ios mac].include?(platform)
272+
end
273+
end
274+
end
275+
end

0 commit comments

Comments
 (0)