From 6923fcb664e129247bd07e3ad3c1f51a32ff9ae2 Mon Sep 17 00:00:00 2001 From: Francesco Lodolo Date: Tue, 15 Sep 2026 20:01:51 +0200 Subject: [PATCH 1/3] Fix warnings on HTML elements in placeholders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the last commit in the #4525, I oversimplified the code on a wrong assumption > ▎ "The parser can't produce — policy is parsed as {#a href=|%1$s|}policy{/a}" That's only true for raw, unescaped XML elements, not for escaped HTML (`<a href="%1$s">…</a>`), which is the case for markup in Android strings.xml. Updating tests and adding in the docstring what's the textual representation of the string, since it only shows up as MF2 in the code. Fixes #4524 --- pontoon/checks/libraries/custom.py | 20 +++- pontoon/checks/tests/test_custom.py | 163 +++++++++++++++++++++++++--- 2 files changed, 167 insertions(+), 16 deletions(-) diff --git a/pontoon/checks/libraries/custom.py b/pontoon/checks/libraries/custom.py index 2f96510e3c..7e3275ba14 100644 --- a/pontoon/checks/libraries/custom.py +++ b/pontoon/checks/libraries/custom.py @@ -258,6 +258,7 @@ def require_placeholders_match( if src: for pattern in get_patterns(src): preview = "" + ph_spans: list[tuple[int, int, str]] = [] for el in pattern: if isinstance(el, str): if "%" in el: @@ -272,10 +273,12 @@ def require_placeholders_match( and el.function in (None, "html") ): src_ph_strings.add(ps) - required_ph.add(ps) + ph_spans.append((len(preview), len(preview) + len(ps), ps)) preview += ps + enclosed_spans: set[tuple[int, int, str]] = set() elements: Counter[str] = Counter() src_mismatched = mismatched_tags(preview) + # Put tags back together when placeholders split them into parts. for pm in ph_re.finditer(preview): if pm[0].startswith("<") and ( is_element(pm[0], preview) or pm.span() in src_mismatched @@ -283,6 +286,18 @@ def require_placeholders_match( src_ph_strings.add(pm[0]) required_ph.add(pm[0]) elements[pm[0]] += 1 + enclosed_spans.update( + (start, end, ps) + for start, end, ps in ph_spans + if pm.start() <= start + and end <= pm.end() + and pm.span() != (start, end) + ) + required_ph.update( + ps + for start, end, ps in ph_spans + if (start, end, ps) not in enclosed_spans + ) variant_counts = count_unnumbered_placeholders(preview) src_max_counts |= variant_counts src_min_counts = ( @@ -302,8 +317,9 @@ def require_placeholders_match( tgt_mismatched = mismatched_tags(pat_src) for pm in ph_re.finditer(pat_src): + rest = pat_src[pm.start() :] for ph in src_ph_strings: - if pat_src.startswith(ph, pm.start()): + if rest.startswith(ph): found_ph.add(ph) break else: diff --git a/pontoon/checks/tests/test_custom.py b/pontoon/checks/tests/test_custom.py index ff8a859679..b40d654d17 100644 --- a/pontoon/checks/tests/test_custom.py +++ b/pontoon/checks/tests/test_custom.py @@ -283,7 +283,32 @@ def test_android_changed_placeholder(): } +def test_android_placeholder_in_element(): + """Source XML: Read the <a href="%1$s">policy</a>""" + original = 'Read the policy{|| :html}' + entity = mock_entity("android", string=original) + assert run_custom_checks(entity, original) == {} + + def test_android_changed_placeholder_in_element(): + """Source XML: Read the <a href="%1$s">policy</a> + + Translation XML: Leggi la <a href="https://example.com">policy</a> + """ + original = 'Read the policy{|| :html}' + translation = 'Leggi la policy{|| :html}' + entity = mock_entity("android", string=original) + assert run_custom_checks(entity, translation) == { + "pErrors": ['Element not found in reference'], + "pWarnings": ['Element not found in translation'], + } + + +def test_android_changed_placeholder_in_markup(): + """Source XML: Read the policy + + Translation XML: Leggi la policy + """ original = "Read the {#a href=|%1$s|}policy{/a}" translation = "Leggi la {#a href=|https://example.com|}policy{/a}" entity = mock_entity("android", string=original) @@ -302,11 +327,19 @@ def test_android_changed_placeholder_in_element(): ], ) def test_android_unnumbered_placeholder_in_markup_attribute(original): + """Source XML: Hi %s link + + Source XML: %s + """ entity = mock_entity("android", string=original) assert run_custom_checks(entity, original) == {} def test_android_extra_unnumbered_placeholder_with_markup_attribute(): + """Source XML: Hi %s link + + Translation XML: Hi %s %s link + """ original = "Hi {$arg :string @source=|%s|} {#a href=|%s|}link{/a}" translation = ( "Hi {$arg :string @source=|%s|} {$arg2 :string @source=|%s|} " @@ -321,6 +354,7 @@ def test_android_extra_unnumbered_placeholder_with_markup_attribute(): def test_android_paired_element_in_text(): + """Source XML: Read the policy""" original = "Read the {#b}policy{/b}" entity = mock_entity("android", string=original) assert run_custom_checks(entity, "Leggi la {#b}policy{/b}") == {} @@ -382,7 +416,10 @@ def test_android_escaped_element_with_attributes(): def test_android_mismatched_elements_in_text(): - """Tags that don't pair up are markup, whatever they're named.""" + """Tags that don't pair up are markup, whatever they're named. + + Translation XML: <foo>testo</bar> + """ entity = mock_entity("android", string="text") checks = run_custom_checks(entity, "{|| :html}testo{|| :html}") assert sorted(checks["pErrors"]) == [ @@ -407,6 +444,9 @@ def test_xcode_literal_angle_brackets(): def test_android_protections_with_shared_substring(): + """Source XML: + Hi Name and FullName + """ original = ( "Hi {$a :xliff:g id=a @translate=no @source=Name} " "and {$b :xliff:g id=b @translate=no @source=FullName}" @@ -421,12 +461,17 @@ def test_android_protections_with_shared_substring(): def test_android_multi_digit_placeholder(): + """Source XML: Hi %10$s and %2$s""" original = "Hi {$a :string @source=|%10$s|} and {$b :string @source=|%2$s|}" entity = mock_entity("android", string=original) assert run_custom_checks(entity, original) == {} def test_android_changed_multi_digit_placeholder(): + """Source XML: Hi %10$s and %2$s + + Translation XML: Ciao %11$s e %2$s + """ original = "Hi {$a :string @source=|%10$s|} and {$b :string @source=|%2$s|}" translation = "Ciao {$a :string @source=|%11$s|} e {$b :string @source=|%2$s|}" entity = mock_entity("android", string=original) @@ -437,6 +482,9 @@ def test_android_changed_multi_digit_placeholder(): def test_android_protection_matching_element_text(): + """Source XML: + Hi Name, see <a title="Name">link</a> + """ original = ( "Hi {$a :xliff:g id=a @translate=no @source=Name}, " 'see {|| :html}link{|| :html}' @@ -593,13 +641,40 @@ def test_xcode_extra_placeholder(): def test_xcode_html(): + """Source XLIFF: Read the <b>policy</b>""" original = "Read the policy" translation = "Leggi la policy" entity = mock_entity("xcode", string=original) assert run_custom_checks(entity, translation) == {} +def test_xcode_placeholder_in_element(): + """Source XLIFF: Read the <a href="%1$@">policy</a>""" + original = 'Read the policy' + translation = 'Leggi la policy' + entity = mock_entity("xcode", string=original) + assert run_custom_checks(entity, translation) == {} + + def test_xcode_changed_placeholder_in_element(): + """Source XLIFF: Read the <a href="%1$@">policy</a> + + Translation XLIFF: Leggi la <a href="https://example.com">policy</a> + """ + original = 'Read the policy' + translation = 'Leggi la policy' + entity = mock_entity("xcode", string=original) + assert run_custom_checks(entity, translation) == { + "pErrors": ['Element not found in reference'], + "pWarnings": ['Element not found in translation'], + } + + +def test_xcode_changed_placeholder_in_markup(): + """Source XLIFF: Read the policy + + Translation XLIFF: Leggi la policy + """ original = "Read the {#a href=|%1$@|}policy{/a}" translation = "Leggi la {#a href=|https://example.com|}policy{/a}" entity = mock_entity("xcode", string=original) @@ -610,8 +685,15 @@ def test_xcode_changed_placeholder_in_element(): def test_xcode_placeholder_in_and_outside_element(): - original = "Visit {#a href=|%@|}{$arg2 :string @source=|%@|}{/a} for details" - translation = "Click {#a href=|%@|}here{/a} for details" + """Source XLIFF: Visit <a href="%@">%@</a> for details + + Translation XLIFF: Click <a href="%@">here</a> for details + """ + original = ( + 'Visit ' + "{$arg2 :string @source=|%@|} for details" + ) + translation = 'Click here for details' entity = mock_entity("xcode", string=original) assert run_custom_checks(entity, translation) == { "pWarnings": ["Placeholder %@ not found in translation"] @@ -624,6 +706,10 @@ def test_xcode_placeholder_in_and_outside_element(): ) @pytest.mark.parametrize("count", [0, 1, 2]) def test_repeated_unnumbered_placeholder(format, placeholder, count): + """Source XLIFF: Hello %@ and %@ (or %#x); Source XML: Hello %s and %s (or %02d) + + Translation: Hello / Hello %@ / Hello %@ and %@ + """ first = "{$arg1 :string @source=|" + placeholder + "|}" second = "{$arg2 :string @source=|" + placeholder + "|}" entity = mock_entity(format, string=f"Hello {first} and {second}") @@ -652,17 +738,25 @@ def test_repeated_unnumbered_placeholder(format, placeholder, count): ], ) def test_repeated_reusable_placeholder(format, placeholder): + """Source XLIFF: Hello %1$@ and %1$@ (or %#@items@); Source XML: Hello %1$s and %1$s + + Translation: Hello %1$@ + """ argument = "{$arg1 :string @source=|" + placeholder + "|}" entity = mock_entity(format, string=f"Hello {argument} and {argument}") assert run_custom_checks(entity, f"Hello {argument}") == {} def test_repeated_unnumbered_placeholder_outside_element(): - element = "{#a href=|%@|}" + """Source XLIFF: <a href="%@">%@ and %@</a> + + Translation XLIFF: <a href="%@">%@</a> + """ + element = '' first = "{$arg2 :string @source=|%@|}" second = "{$arg3 :string @source=|%@|}" - entity = mock_entity("xcode", string=f"{element}{first} and {second}{{/a}}") - assert run_custom_checks(entity, f"{element}{first}{{/a}}") == { + entity = mock_entity("xcode", string=f"{element}{first} and {second}") + assert run_custom_checks(entity, f"{element}{first}") == { "pWarnings": [ "Placeholder %@ has fewer occurrences in translation (expected 3, found 2)" ] @@ -671,6 +765,11 @@ def test_repeated_unnumbered_placeholder_outside_element(): @pytest.mark.parametrize("missing", [False, True]) def test_repeated_unnumbered_placeholder_plural_variants(missing): + """Source XML: with "For %s and %s" for quantity one and other + + Translation XML: with "For %s" (missing) or "For %s and %s" for + quantity one, few and other + """ first = "{$arg1 :string @source=|%s|}" second = "{$arg2 :string @source=|%s|}" original = ( @@ -700,6 +799,10 @@ def test_repeated_unnumbered_placeholder_plural_variants(missing): def test_repeated_unnumbered_placeholder_extra_occurrence(): + """Source XLIFF: Hello %@ and %@ + + Translation XLIFF: Hello %@, %@ and %@ + """ first = "{$arg1 :string @source=|%@|}" second = "{$arg2 :string @source=|%@|}" third = "{$arg3 :string @source=|%@|}" @@ -712,7 +815,12 @@ def test_repeated_unnumbered_placeholder_extra_occurrence(): def test_repeated_unnumbered_placeholder_fewer_source_variants(): - """Translations can have fewer plural forms than the source.""" + """Translations can have fewer plural forms than the source. + + Source XML: with "%s of %s" for quantity one and "%s items" for other + + Translation XML: with "%s items" for quantity other + """ first = "{$arg1 :string @source=|%s|}" second = "{$arg2 :string @source=|%s|}" original = ( @@ -726,9 +834,13 @@ def test_repeated_unnumbered_placeholder_fewer_source_variants(): def test_unnumbered_placeholder_moved_outside_element(): + """Source XLIFF: <a href="%@">%@</a> + + Translation XLIFF: %@ %@ + """ first = "{$arg1 :string @source=|%@|}" second = "{$arg2 :string @source=|%@|}" - entity = mock_entity("xcode", string=f"{{#a href=|%@|}}{second}{{/a}}") + entity = mock_entity("xcode", string=f'{second}') checks = run_custom_checks(entity, f"{first} {second}") assert list(checks) == ["pWarnings"] assert sorted(checks["pWarnings"]) == [ @@ -739,7 +851,11 @@ def test_unnumbered_placeholder_moved_outside_element(): @pytest.mark.parametrize("source_count, target_count", [(2, 1), (1, 2), (2, 2)]) def test_repeated_element_with_unnumbered_placeholder(source_count, target_count): - element = "{#a href=|%@|}link{/a}" + """Source XLIFF: <a href="%@">link</a>, repeated source_count times + + Translation XLIFF: the same element, repeated target_count times + """ + element = 'link' entity = mock_entity("xcode", string=" ".join([element] * source_count)) expected = {} if source_count != target_count: @@ -755,14 +871,24 @@ def test_repeated_element_with_unnumbered_placeholder(source_count, target_count def test_missing_element_suppresses_unnumbered_placeholder_warning(): - entity = mock_entity("xcode", string="Read {#a href=|%@|}link{/a}") - assert run_custom_checks(entity, "Read link{/a}") == { + """Source XLIFF: Read <a href="%@">link</a> + + Translation XLIFF: Read link</a> + """ + entity = mock_entity( + "xcode", string='Read link' + ) + assert run_custom_checks(entity, "Read link") == { "pWarnings": ['Element not found in translation'] } @pytest.mark.parametrize("count", [1, 3]) def test_unnumbered_placeholder_mismatch_in_one_plural_variant(count): + """Source stringsdict: "%@ %@" for one and other + + Translation stringsdict: "%@" or "%@ %@ %@" for one, "%@ %@" for other + """ argument = "{$arg1 :string @source=|%@|}" pair = argument + " " + argument original = ".input {$n :number} .match $n one {{" + pair + "}} * {{" + pair + "}}" @@ -780,11 +906,20 @@ def test_unnumbered_placeholder_mismatch_in_one_plural_variant(count): def test_missing_elements_in_different_source_variants(): + """Source stringsdict: <a href="%@">link</a> for one, + <a title="%@">link</a> for other + + Translation stringsdict: link</a> for other + """ + argument = "{$arg1 :string @source=|%@|}" original = ( - ".input {$n :number} .match $n " - "one {{{#a href=|%@|}link{/a}}} * {{{#a title=|%@|}link{/a}}}" + '.input {$n :number} .match $n one {{link}} * {{link}}' ) - translation = ".input {$n :number} .match $n * {{link{/a}}}" + translation = ".input {$n :number} .match $n * {{link}}" checks = run_custom_checks(mock_entity("xcode", string=original), translation) assert list(checks) == ["pWarnings"] assert sorted(checks["pWarnings"]) == [ From 42555760baef21b2249876a5242ddc8c3d27e5a6 Mon Sep 17 00:00:00 2001 From: Francesco Lodolo Date: Tue, 15 Sep 2026 21:20:33 +0200 Subject: [PATCH 2/3] Restore optimization (address comment) --- pontoon/checks/libraries/custom.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pontoon/checks/libraries/custom.py b/pontoon/checks/libraries/custom.py index 7e3275ba14..011baaa8b2 100644 --- a/pontoon/checks/libraries/custom.py +++ b/pontoon/checks/libraries/custom.py @@ -319,7 +319,7 @@ def require_placeholders_match( for pm in ph_re.finditer(pat_src): rest = pat_src[pm.start() :] for ph in src_ph_strings: - if rest.startswith(ph): + if pat_src.startswith(ph, pm.start()): found_ph.add(ph) break else: From a513c00c490c5d48f133f8fa50d8d701fef08560 Mon Sep 17 00:00:00 2001 From: Francesco Lodolo Date: Tue, 15 Sep 2026 21:24:05 +0200 Subject: [PATCH 3/3] Fix linter --- pontoon/checks/libraries/custom.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pontoon/checks/libraries/custom.py b/pontoon/checks/libraries/custom.py index 011baaa8b2..d3c160d280 100644 --- a/pontoon/checks/libraries/custom.py +++ b/pontoon/checks/libraries/custom.py @@ -317,7 +317,6 @@ def require_placeholders_match( tgt_mismatched = mismatched_tags(pat_src) for pm in ph_re.finditer(pat_src): - rest = pat_src[pm.start() :] for ph in src_ph_strings: if pat_src.startswith(ph, pm.start()): found_ph.add(ph)