Skip to content

Commit bb81e99

Browse files
committed
Add ios_lint_localization_placeholder_changes action
Adds a temporal placeholder-compatibility guardrail for localization source strings: it fails when an existing key's base-language value changes its format placeholders — count, position, or argument type — between two versions of the base `.strings` file. Translations are filed by key, so such a change silently breaks every existing translation of that key. This is the temporal counterpart to `ios_lint_localizations` (base ↔ base across versions, rather than translation ↔ base at one point in time). `StringPlaceholdersHelper` — a pure, platform-agnostic primitive (`placeholder_signature`, `placeholders_compatible?`, `incompatible_placeholder_changes`, `mixed_operators?`). The signature is invariant to benign copy edits and to reordering equivalent positional arguments, but sensitive to a change in placeholder count, position, or argument type. It recognizes positional specifiers (`%1$d`), length modifiers, and the `,` / `'` thousands-grouping flags; `%d`↔`%i` are compatible; `%%` is skipped; `*` dynamic width/precision is deliberately left unrecognized so a change to/from it surfaces rather than masquerading. `ios_lint_localization_placeholder_changes` — the iOS action wrapping it. Reuses `L10nHelper.read_strings_file_as_hash` (plutil) rather than adding a second `.strings` parser, so it is macOS-only by design, like `ios_lint_localizations`. Mirrors its `abort_on_violations` (default true). The action fails closed on any input where the comparison would be unreliable: - Aborts (via `check_duplicate_keys`, default true) when either file defines a key more than once — plutil keeps only the last value, which could hide a real placeholder change — and likewise aborts when a `:text` file parses for plutil but not for the duplicate-key scanner, rather than silently skipping the check. - Always aborts when a value mixes positional (`%1$@`) and non-positional (`%@`) placeholders, which is invalid (printf/`NSString` require all-or-nothing positional) and leaves the signature undefined. Also teaches `StringsFileValidationHelper.find_duplicated_keys` to parse unquoted keys (valid `.strings` syntax, common in `InfoPlist.strings`) instead of raising, so `ios_lint_localizations`' `check_duplicate_keys` handles them too.
1 parent de7ff74 commit bb81e99

8 files changed

Lines changed: 1035 additions & 10 deletions

CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,11 @@ _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

17-
_None_
18+
- `StringsFileValidationHelper.find_duplicated_keys` now parses *unquoted* keys (valid `.strings` syntax, common in `InfoPlist.strings`) instead of raising `Invalid character`. This lets duplicate-key detection — `ios_lint_localizations`' `check_duplicate_keys` and the new `ios_lint_localization_placeholder_changes` check — work on files with unquoted keys. [#738]
1819

1920
### Internal Changes
2021

Lines changed: 273 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,273 @@
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.
96+
#
97+
# Only ASCII-plist (`:text`) files are scanned for duplicates here: `find_duplicated_keys`
98+
# tokenizes the ASCII syntax. An XML or binary plist file *can* hold a duplicate key, but parsing
99+
# it always collapses to a single key (`plutil` keeps the last value) and we have no tokenizer for
100+
# those formats, so duplicates in non-`:text` inputs aren't caught — toolkit-generated XML is built
101+
# from a Hash and is dup-free in any case. A `:text` file the validator can't parse aborts the
102+
# action — the duplicate-key guarantee is a precondition for a reliable comparison, so we fail
103+
# closed rather than skip it.
104+
def self.duplicate_keys_by_file(params)
105+
[params[:old_file], params[:new_file]].uniq.each_with_object({}) do |path, result|
106+
duplicates = duplicate_keys_in(path)
107+
result[path] = duplicates unless duplicates.empty?
108+
end
109+
end
110+
111+
def self.duplicate_keys_in(path)
112+
return {} unless Fastlane::Helper::Ios::L10nHelper.strings_file_type(path: path) == :text
113+
114+
Fastlane::Helper::Ios::StringsFileValidationHelper.find_duplicated_keys(file: path)
115+
rescue StandardError => e
116+
# The file is a text plist that `plutil` happily parses into the `{ key => value }` hash we
117+
# compare, but our duplicate-key scanner couldn't read it (e.g. an unquoted value, or an
118+
# unquoted key with a character it doesn't accept). We therefore can't guarantee each key is
119+
# defined exactly once, and `plutil` silently keeps only the *last* value for a duplicate — so
120+
# proceeding could compare the wrong value and miss a real placeholder change. Fail closed (the
121+
# same reason an actual duplicate aborts) rather than skip the check and compare blindly.
122+
UI.abort_with_message!(unverifiable_duplicates_abort_message(path, e.message))
123+
end
124+
125+
def self.report_duplicate_keys(duplicate_keys)
126+
duplicate_keys.each do |path, duplicates|
127+
UI.error <<~MSG
128+
`#{File.basename(path)}` defines #{duplicates.count} key(s) more than once:
129+
#{duplicates.map { |key, lines| " `#{key}` at lines #{lines.join(', ')}" }.join("\n")}
130+
MSG
131+
end
132+
end
133+
134+
def self.duplicate_keys_abort_message(duplicate_keys)
135+
count = duplicate_keys.values.sum(&:count)
136+
<<~MSG
137+
#{count} localization key(s) are defined more than once in the source strings.
138+
139+
Parsing `.strings` files relies on `plutil`, which silently keeps only the *last* value for
140+
a duplicated key. This check would therefore compare against the wrong value and could miss a
141+
real, translation-breaking placeholder change — the exact failure it exists to catch. Define
142+
each key exactly once and regenerate the file before re-running this check.
143+
MSG
144+
end
145+
146+
def self.unverifiable_duplicates_abort_message(path, error_message)
147+
<<~MSG
148+
Could not verify `#{File.basename(path)}` for duplicate keys: #{error_message.strip}
149+
150+
Parsing `.strings` files relies on `plutil`, which silently keeps only the *last* value for a
151+
duplicated key, so this action guards against a duplicate hiding a real placeholder change. The
152+
file parses as text but our duplicate-key scanner couldn't read it, so that guarantee can't be
153+
established and the comparison would be unreliable. Make sure the file parses (e.g. quote any
154+
unquoted keys/values), or pass `check_duplicate_keys: false` to skip this check.
155+
MSG
156+
end
157+
158+
# Finds keys whose value mixes positional and non-positional placeholders, per file, returned as
159+
# `{ path => { key => value } }`, only including files that have any.
160+
def self.mixed_operator_keys_by_file(strings_by_path)
161+
strings_by_path.each_with_object({}) do |(path, strings), result|
162+
mixed = strings.select { |_key, value| Fastlane::Helper::StringPlaceholdersHelper.mixed_operators?(value) }
163+
result[path] = mixed unless mixed.empty?
164+
end
165+
end
166+
167+
def self.report_mixed_operators(mixed_keys)
168+
mixed_keys.each do |path, keys|
169+
UI.error <<~MSG
170+
`#{File.basename(path)}` has #{keys.count} key(s) that mix positional and non-positional placeholders:
171+
#{keys.map { |key, value| " `#{key}`: #{value.inspect}" }.join("\n")}
172+
MSG
173+
end
174+
end
175+
176+
def self.mixed_operators_abort_message(mixed_keys)
177+
count = mixed_keys.values.sum(&:count)
178+
<<~MSG
179+
#{count} localization key(s) mix positional (`%1$@`) and non-positional (`%@`) placeholders in a single value.
180+
181+
Mixing the two in one format string is invalid — `printf`/`NSString` require either all-positional
182+
or all-non-positional — so the placeholder shape can't be compared reliably. Give every placeholder
183+
an explicit `%N$` position, or none, and regenerate the file before re-running this check.
184+
MSG
185+
end
186+
187+
#####################################################
188+
# @!group Documentation
189+
#####################################################
190+
191+
def self.description
192+
'Checks that no existing localization key changed its format placeholders between two versions of a source `.strings` file'
193+
end
194+
195+
def self.details
196+
<<~DETAILS
197+
Compares two versions (old vs new) of the same base-language `.strings` file and reports any
198+
key — present in both — whose format placeholders (`%@`, `%1$d`, …) changed in count, position,
199+
or argument type.
200+
201+
This is the temporal counterpart to `ios_lint_localizations` (which compares each translation
202+
against the base language at a single point in time). Translations are filed by key, not by
203+
English text, so changing the placeholders of an existing key silently breaks every translation
204+
of that key. New and removed keys are ignored on purpose: copy that needs different placeholders
205+
is expected to land under a brand new key.
206+
207+
By default it also aborts if either input file defines the same key more than once: `plutil`
208+
silently keeps only the last value for a duplicated key, which would let a duplicate hide a
209+
real placeholder change. Disable this with `check_duplicate_keys: false`.
210+
211+
Note: parsing `.strings` files relies on `plutil`, so this action only runs on macOS.
212+
DETAILS
213+
end
214+
215+
def self.available_options
216+
[
217+
FastlaneCore::ConfigItem.new(
218+
key: :old_file,
219+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_OLD_FILE',
220+
description: 'Path to the previous version of the base-language `.strings` file',
221+
optional: false,
222+
type: String,
223+
verify_block: proc do |value|
224+
UI.user_error!("`old_file` not found at path: #{value}") unless File.exist?(value)
225+
end
226+
),
227+
FastlaneCore::ConfigItem.new(
228+
key: :new_file,
229+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_NEW_FILE',
230+
description: 'Path to the newly-generated version of the base-language `.strings` file',
231+
optional: false,
232+
type: String,
233+
verify_block: proc do |value|
234+
UI.user_error!("`new_file` not found at path: #{value}") unless File.exist?(value)
235+
end
236+
),
237+
FastlaneCore::ConfigItem.new(
238+
key: :abort_on_violations,
239+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_ABORT',
240+
description: 'Should we abort the rest of the lane with a global error if any incompatible changes are found?',
241+
optional: true,
242+
default_value: true,
243+
type: Boolean
244+
),
245+
FastlaneCore::ConfigItem.new(
246+
key: :check_duplicate_keys,
247+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_CHECK_DUPLICATE_KEYS',
248+
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',
249+
optional: true,
250+
default_value: true,
251+
type: Boolean
252+
),
253+
]
254+
end
255+
256+
def self.return_type
257+
:array
258+
end
259+
260+
def self.return_value
261+
'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.'
262+
end
263+
264+
def self.authors
265+
['Automattic']
266+
end
267+
268+
def self.is_supported?(platform)
269+
%i[ios mac].include?(platform)
270+
end
271+
end
272+
end
273+
end

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

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,26 @@ module Ios
66
class StringsFileValidationHelper
77
# context can be one of:
88
# :root, :maybe_comment_start, :in_line_comment, :in_block_comment,
9-
# :maybe_block_comment_end, :in_quoted_key,
9+
# :maybe_block_comment_end, :in_quoted_key, :in_unquoted_key,
1010
# :after_quoted_key_before_eq, :after_quoted_key_and_equal,
1111
# :in_quoted_value, :after_quoted_value
1212
State = Struct.new(:context, :buffer, :in_escaped_ctx, :found_key)
1313

14+
# Characters allowed in an *unquoted* key. Unquoted strings are valid `.strings` syntax (the
15+
# old-style ASCII property-list format) and are common for `InfoPlist.strings` keys such as
16+
# `CFBundleDisplayName`. An unquoted key runs until the first whitespace or `=`.
17+
UNQUOTED_KEY_CHARACTER = /[a-zA-Z0-9_.-]/u
18+
1419
TRANSITIONS = {
1520
root: {
1621
/\s/u => :root,
1722
'/' => :maybe_comment_start,
18-
'"' => :in_quoted_key
23+
'"' => :in_quoted_key,
24+
# An unquoted key, e.g. `CFBundleName = "…";` as used by `InfoPlist.strings`.
25+
UNQUOTED_KEY_CHARACTER => lambda do |state, c|
26+
state.buffer.write(c)
27+
:in_unquoted_key
28+
end
1929
},
2030
maybe_comment_start: {
2131
'/' => :in_line_comment,
@@ -44,6 +54,19 @@ class StringsFileValidationHelper
4454
:in_quoted_key
4555
end
4656
},
57+
in_unquoted_key: {
58+
# The key ends at the first whitespace or `=`. Whitespace still expects an `=` next;
59+
# an `=` moves straight on to the value.
60+
/[\s=]/u => lambda do |state, c|
61+
state.found_key = state.buffer.string.dup
62+
state.buffer = StringIO.new
63+
c == '=' ? :after_quoted_key_and_eq : :after_quoted_key_before_eq
64+
end,
65+
UNQUOTED_KEY_CHARACTER => lambda do |state, c|
66+
state.buffer.write(c)
67+
:in_unquoted_key
68+
end
69+
},
4770
after_quoted_key_before_eq: {
4871
/\s/u => :after_quoted_key_before_eq,
4972
'=' => :after_quoted_key_and_eq

0 commit comments

Comments
 (0)