Skip to content

Commit cbbfbfd

Browse files
committed
Add ios_lint_localization_placeholder_changes action
Adds a platform-agnostic `StringPlaceholdersHelper` primitive (`placeholder_signature`, `placeholders_compatible?`, `incompatible_placeholder_changes`) and an iOS action that 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 aborts (by default, via `check_duplicate_keys`) when either input file defines a key more than once, since `plutil` keeps only the last value and a duplicate could otherwise hide a real change; and it reports a clean error when an input file cannot be parsed. As part of the duplicate-key check, `StringsFileValidationHelper.find_duplicated_keys` now also handles unquoted keys (valid `.strings` syntax, common in `InfoPlist.strings`). Part of #736.
1 parent de7ff74 commit cbbfbfd

7 files changed

Lines changed: 721 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`) 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. [#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: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
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+
# Duplicate keys must be caught *before* the comparison: `plutil` silently keeps only the last
13+
# value for a duplicated key, so a duplicate could hide a real placeholder change. They always
14+
# abort — independently of `abort_on_violations`, which only governs *placeholder changes* —
15+
# because the comparison itself is unreliable until each key is defined exactly once.
16+
if params[:check_duplicate_keys]
17+
duplicate_keys = duplicate_keys_by_file(params)
18+
unless duplicate_keys.empty?
19+
report_duplicate_keys(duplicate_keys)
20+
UI.abort_with_message!(duplicate_keys_abort_message(duplicate_keys))
21+
end
22+
end
23+
24+
old_strings = read_strings(params[:old_file])
25+
new_strings = read_strings(params[:new_file])
26+
27+
violations = Fastlane::Helper::StringPlaceholdersHelper.incompatible_placeholder_changes(old_strings, new_strings)
28+
report(violations)
29+
30+
UI.abort_with_message!(abort_message(violations.length)) if !violations.empty? && params[:abort_on_violations]
31+
32+
violations
33+
end
34+
35+
# Reads a `.strings` file into a `{ key => value }` hash. A malformed file is an *input* error
36+
# (like a missing file), so surface a clean, user-facing message naming it rather than letting a
37+
# raw `plutil` stderr dump escape the lane. `read_strings_file_as_hash` shells out to `plutil`,
38+
# which handles text/XML/binary plists, so this only trips on genuinely unparseable input.
39+
def self.read_strings(path)
40+
strings =
41+
begin
42+
Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: path)
43+
rescue StandardError => e
44+
UI.user_error!("Could not parse `#{File.basename(path)}` as a `.strings` file. #{e.message.strip}")
45+
end
46+
return strings if strings.is_a?(Hash)
47+
48+
UI.user_error!("`#{File.basename(path)}` is not a valid `.strings` file (expected a dictionary of key/value pairs).")
49+
end
50+
51+
def self.report(violations)
52+
if violations.empty?
53+
UI.success('No incompatible placeholder changes found between the two versions of the strings file.')
54+
return
55+
end
56+
57+
violations.each do |violation|
58+
UI.error <<~MSG
59+
`#{violation[:key]}` changed its format placeholders incompatibly:
60+
was: #{violation[:old].inspect} [#{describe_signature(violation[:old_signature])}]
61+
now: #{violation[:new].inspect} [#{describe_signature(violation[:new_signature])}]
62+
MSG
63+
end
64+
end
65+
66+
def self.describe_signature(signature)
67+
signature.empty? ? 'no placeholders' : signature
68+
end
69+
70+
def self.abort_message(count)
71+
<<~MSG
72+
#{count} localization key(s) changed their format placeholders incompatibly.
73+
74+
Existing translations are keyed by the string's key, not its English text, so changing
75+
the placeholders of an existing key would silently break every translation for that key.
76+
Give the changed copy a brand new key instead, so the previous translations stay valid
77+
for the previous key and the new copy gets translated from scratch.
78+
MSG
79+
end
80+
81+
# Finds duplicate keys in the old and new files, returned as `{ path => { key => [lines] } }`,
82+
# only including files that actually have duplicates.
83+
#
84+
# Only ASCII-plist (`:text`) files can be inspected: XML/binary plists can't represent a
85+
# duplicate key. A `:text` file the validator can't parse is skipped with a warning rather
86+
# than failing the whole check.
87+
def self.duplicate_keys_by_file(params)
88+
[params[:old_file], params[:new_file]].uniq.each_with_object({}) do |path, result|
89+
duplicates = duplicate_keys_in(path)
90+
result[path] = duplicates unless duplicates.empty?
91+
end
92+
end
93+
94+
def self.duplicate_keys_in(path)
95+
return {} unless Fastlane::Helper::Ios::L10nHelper.strings_file_type(path: path) == :text
96+
97+
Fastlane::Helper::Ios::StringsFileValidationHelper.find_duplicated_keys(file: path)
98+
rescue StandardError => e
99+
UI.important("Could not check `#{File.basename(path)}` for duplicate keys, skipping: #{e.message}")
100+
{}
101+
end
102+
103+
def self.report_duplicate_keys(duplicate_keys)
104+
duplicate_keys.each do |path, duplicates|
105+
UI.error <<~MSG
106+
`#{File.basename(path)}` defines #{duplicates.count} key(s) more than once:
107+
#{duplicates.map { |key, lines| " `#{key}` at lines #{lines.join(', ')}" }.join("\n")}
108+
MSG
109+
end
110+
end
111+
112+
def self.duplicate_keys_abort_message(duplicate_keys)
113+
count = duplicate_keys.values.sum(&:count)
114+
<<~MSG
115+
#{count} localization key(s) are defined more than once in the source strings.
116+
117+
Parsing `.strings` files relies on `plutil`, which silently keeps only the *last* value for
118+
a duplicated key. This check would therefore compare against the wrong value and could miss a
119+
real, translation-breaking placeholder change — the exact failure it exists to catch. Define
120+
each key exactly once and regenerate the file before re-running this check.
121+
MSG
122+
end
123+
124+
#####################################################
125+
# @!group Documentation
126+
#####################################################
127+
128+
def self.description
129+
'Checks that no existing localization key changed its format placeholders between two versions of a source `.strings` file'
130+
end
131+
132+
def self.details
133+
<<~DETAILS
134+
Compares two versions (old vs new) of the same base-language `.strings` file and reports any
135+
key — present in both — whose format placeholders (`%@`, `%1$d`, …) changed in count, position,
136+
or argument type.
137+
138+
This is the temporal counterpart to `ios_lint_localizations` (which compares each translation
139+
against the base language at a single point in time). Translations are filed by key, not by
140+
English text, so changing the placeholders of an existing key silently breaks every translation
141+
of that key. New and removed keys are ignored on purpose: copy that needs different placeholders
142+
is expected to land under a brand new key.
143+
144+
By default it also aborts if either input file defines the same key more than once: `plutil`
145+
silently keeps only the last value for a duplicated key, which would let a duplicate hide a
146+
real placeholder change. Disable this with `check_duplicate_keys: false`.
147+
148+
Note: parsing `.strings` files relies on `plutil`, so this action only runs on macOS.
149+
DETAILS
150+
end
151+
152+
def self.available_options
153+
[
154+
FastlaneCore::ConfigItem.new(
155+
key: :old_file,
156+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_OLD_FILE',
157+
description: 'Path to the previous version of the base-language `.strings` file',
158+
optional: false,
159+
type: String,
160+
verify_block: proc do |value|
161+
UI.user_error!("`old_file` not found at path: #{value}") unless File.exist?(value)
162+
end
163+
),
164+
FastlaneCore::ConfigItem.new(
165+
key: :new_file,
166+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_NEW_FILE',
167+
description: 'Path to the newly-generated version of the base-language `.strings` file',
168+
optional: false,
169+
type: String,
170+
verify_block: proc do |value|
171+
UI.user_error!("`new_file` not found at path: #{value}") unless File.exist?(value)
172+
end
173+
),
174+
FastlaneCore::ConfigItem.new(
175+
key: :abort_on_violations,
176+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_ABORT',
177+
description: 'Should we abort the rest of the lane with a global error if any incompatible changes are found?',
178+
optional: true,
179+
default_value: true,
180+
type: Boolean
181+
),
182+
FastlaneCore::ConfigItem.new(
183+
key: :check_duplicate_keys,
184+
env_name: 'FL_IOS_LINT_L10N_PLACEHOLDER_CHANGES_CHECK_DUPLICATE_KEYS',
185+
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',
186+
optional: true,
187+
default_value: true,
188+
type: Boolean
189+
),
190+
]
191+
end
192+
193+
def self.return_type
194+
:array
195+
end
196+
197+
def self.return_value
198+
'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.'
199+
end
200+
201+
def self.authors
202+
['Automattic']
203+
end
204+
205+
def self.is_supported?(platform)
206+
%i[ios mac].include?(platform)
207+
end
208+
end
209+
end
210+
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
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
# frozen_string_literal: true
2+
3+
module Fastlane
4+
module Helper
5+
# Platform-agnostic primitive for comparing the placeholder "shape" — the
6+
# count, position, and argument type of the printf/`NSString` format
7+
# specifiers — of localized string values.
8+
#
9+
# This is purposely **pure Ruby** (no file I/O, no shelling out) so it runs
10+
# on any platform: the same `%@`/`%1$d`/`%2$s` placeholder syntax is shared by
11+
# iOS and Android. The platform-specific actions parse their respective file
12+
# format into a `{ key => value }` hash and then call into this helper.
13+
#
14+
# The main use case is a temporal guardrail: when a source-language string is
15+
# regenerated, an existing key's value must not change its placeholders
16+
# (e.g. `"%1$@ liked your post"` → `"%1$d people liked your post"`). Existing
17+
# translations stay filed under the same key, so a placeholder-incompatible
18+
# change would silently break every translation of that key. Copy that needs
19+
# different placeholders is expected to land under a brand new key instead.
20+
#
21+
module StringPlaceholdersHelper
22+
# printf / `NSString` conversion characters grouped by the argument type a
23+
# translation must preserve. Width, precision and length modifiers don't
24+
# affect the argument type, so they are intentionally not part of the class.
25+
#
26+
# `%d` ↔ `%i` is compatible (both consume an `int`); `%d` ↔ `%@` is not
27+
# (`int` vs `object`).
28+
CONVERSION_CLASSES = {
29+
'@' => 'object',
30+
'd' => 'int', 'i' => 'int', 'u' => 'int', 'o' => 'int', 'x' => 'int', 'X' => 'int',
31+
'f' => 'float', 'e' => 'float', 'E' => 'float', 'g' => 'float', 'G' => 'float', 'a' => 'float', 'A' => 'float',
32+
'c' => 'char', 'C' => 'char',
33+
's' => 'cstring', 'S' => 'cstring',
34+
'p' => 'pointer'
35+
}.freeze
36+
37+
# A single format specifier: a `%`, an optional positional argument (`1$`),
38+
# flags, width, precision, an optional length modifier, then the conversion
39+
# character. `%%` (a literal percent) is matched too so it can be explicitly
40+
# skipped rather than mistaken for a placeholder.
41+
SPECIFIER = /%(?<position>\d+\$)?[-+ 0#]*(?:\d+|\*)?(?:\.(?:\d+|\*))?(?:hh|h|ll|l|q|L|z|t|j)?(?<conversion>[@diouxXeEfgGaAcCsSpn%])/
42+
43+
# A canonical signature of the placeholders in a string value.
44+
#
45+
# Two values with the same signature are placeholder-compatible. The
46+
# signature is invariant to benign copy edits and to reordering equivalent
47+
# positional arguments, but sensitive to a change in the count, position, or
48+
# argument type of the placeholders.
49+
#
50+
# @param [String] value The string value to compute a signature for.
51+
# @return [String] e.g. `"1:int,2:object"`, or `''` if there are no placeholders.
52+
#
53+
def self.placeholder_signature(value)
54+
# Key each specifier by its position — explicit for `%1$@`, otherwise its
55+
# order of appearance — then sort by it. This way reordering equivalent
56+
# positional args (`%1$@ %2$@` vs `%2$@ %1$@`) yields the same signature,
57+
# while a changed count or argument type does not.
58+
extract_specifiers(value)
59+
.each_with_index
60+
.map { |specifier, index| [specifier[:position] || (index + 1), specifier[:class]] }
61+
.sort_by(&:first)
62+
.map { |position, klass| "#{position}:#{klass}" }
63+
.join(',')
64+
end
65+
66+
# Whether two string values share the same placeholder shape.
67+
#
68+
# @param [String] old_value The first value to compare.
69+
# @param [String] new_value The second value to compare.
70+
# @return [Boolean] `true` if both values have the same placeholder signature.
71+
#
72+
def self.placeholders_compatible?(old_value, new_value)
73+
placeholder_signature(old_value) == placeholder_signature(new_value)
74+
end
75+
76+
# Given two `{ key => value }` hashes, finds the keys present in **both**
77+
# whose placeholder signature changed.
78+
#
79+
# New and removed keys are ignored on purpose: copy that needs a fresh
80+
# translation is expected to land under a new key (which shows up as
81+
# remove-old + add-new, not as a change to an existing key).
82+
#
83+
# @param [Hash<String,String>] old_strings The previous `{ key => value }` strings.
84+
# @param [Hash<String,String>] new_strings The new `{ key => value }` strings.
85+
# @return [Array<Hash>] One entry per incompatible change, sorted by key, each
86+
# `{ key:, old:, new:, old_signature:, new_signature: }`.
87+
#
88+
def self.incompatible_placeholder_changes(old_strings, new_strings)
89+
(old_strings.keys & new_strings.keys).sort.filter_map do |key|
90+
old_signature = placeholder_signature(old_strings[key])
91+
new_signature = placeholder_signature(new_strings[key])
92+
next if old_signature == new_signature
93+
94+
{ key: key, old: old_strings[key], new: new_strings[key], old_signature: old_signature, new_signature: new_signature }
95+
end
96+
end
97+
98+
# The format specifiers found in a value, as an array of
99+
# `{ position:, class: }` hashes, excluding the literal `%%`.
100+
# `position` is `nil` for non-positional specifiers.
101+
#
102+
# @param [String] value The string value to scan.
103+
# @return [Array<Hash>] The specifiers, in order of appearance.
104+
#
105+
def self.extract_specifiers(value)
106+
found = []
107+
value.to_s.scan(SPECIFIER) do
108+
match = Regexp.last_match
109+
conversion = match[:conversion]
110+
next if conversion == '%' # literal percent, not a placeholder
111+
112+
found << { position: match[:position]&.delete('$')&.to_i, class: CONVERSION_CLASSES.fetch(conversion, conversion) }
113+
end
114+
found
115+
end
116+
private_class_method :extract_specifiers
117+
end
118+
end
119+
end

0 commit comments

Comments
 (0)