From f405032dd0f4d9f0d6e5cdc3559434e6d94c1ade Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Mon, 24 Aug 2026 19:46:15 -0700 Subject: [PATCH 1/4] =?UTF-8?q?fix:=20=F0=9F=90=9B=20Resolve=20translation?= =?UTF-8?q?s=20against=20every=20locale,=20not=20a=20hardcoded=20"en".?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #287 made the hover show every locale that defines a key, but go-to-definition and diagnostics kept resolving against a hardcoded "en" across all three key shapes. A key defined only in `de` therefore rendered in the hover, refused to navigate, and was still reported missing — and the comment claiming "navigation, diagnostics, and hover all agree on where a key lives" was simply false. All three now resolve against translation_lookup::available_locales, a single shared helper that returns every locale which could define the key, ordered with the project's APP_LOCALE first and the rest alphabetically. Sharing the ordering matters as much as sharing the set: when several locales define a key, goto lands on the same one the hover leads with. Resolution is now by reading the key rather than probing for the file. The dotted and text branches previously accepted a lang file's existence as proof the key existed, so a file that had been created but never populated made diagnostics report a missing key as present and sent goto to a file that never contained it. resources/lang is wired into the resolver itself (resolve_dotted, resolve_namespaced, resolve_text_key) rather than only into discovery. Diagnostics had always checked both lang roots while the resolver checked only lang/, so on a Laravel-8-style project hover could find nothing while diagnostics resolved happily — the same divergence in the opposite direction. APP_LOCALE is read through config::read_env_value, extracted from DatabaseConfigResolver::resolve_env. That reader is deliberately horizontal to stop one variable's value swallowing the next line, a past credential-leak bug; a second hand-rolled regex would risk reintroducing it, so there is now one reader and database.rs delegates to it. The multi-locale card is rebuilt on the HoverContent template with a structured per-locale field instead of a hand-built markdown string. Source links render inline on each locale's line rather than as separate paragraphs, and when exactly one locale *resolves* the key the card collapses to the dense single-block form — most projects ship one locale and a lone value under a heading costs a line to say nothing. A distinct trailer states the key was found in no locale at all, leaving the single-locale card's "default locale" wording intact where it is still accurate. get_all_translation_keys deliberately keeps its first-wins directory selection: completion asks "what keys exist at all", which any single locale answers, and unioning every catalogue per request would read the whole project to produce the same list. Per-hover I/O is still outside Salsa and is now multiplied by locale count; that trade is tracked in #293. Fixes: #288 --- laravel-lsp/src/config.rs | 26 ++ laravel-lsp/src/database.rs | 33 +- laravel-lsp/src/hover.rs | 90 +++-- laravel-lsp/src/hover/tests.rs | 85 ++++- laravel-lsp/src/main.rs | 319 ++++++++---------- laravel-lsp/src/tests/mod.rs | 1 + .../tests/translation_locale_consistency.rs | 163 +++++++++ laravel-lsp/src/translation_lookup.rs | 136 +++++++- laravel-lsp/src/translation_lookup/tests.rs | 190 +++++++++++ 9 files changed, 796 insertions(+), 247 deletions(-) create mode 100644 laravel-lsp/src/tests/translation_locale_consistency.rs diff --git a/laravel-lsp/src/config.rs b/laravel-lsp/src/config.rs index 8ac7f037..459dd1fe 100644 --- a/laravel-lsp/src/config.rs +++ b/laravel-lsp/src/config.rs @@ -178,6 +178,32 @@ fn find_project_root_upward(start: &Path) -> Option { tentative } +/// Read a single value out of a project's `.env`. +/// +/// The one hardened `.env` reader in this codebase. The pattern is +/// deliberately horizontal (`[ \t]*`, `[^'"\n]*`) so a blank value +/// (`KEY=\n`) captures the empty string rather than swallowing the next +/// line — a naive multi-line-tolerant regex here previously leaked one +/// variable's value into another's, and any second copy of this logic risks +/// reintroducing that. Callers that need an env value go through this. +/// +/// Returns `None` when the file is unreadable, the key is absent, or the value +/// is empty. +pub fn read_env_value(project_root: &Path, key: &str) -> Option { + let env_path = resolve_worktree_fallback(project_root, ".env"); + let content = std::fs::read_to_string(&env_path).ok()?; + let pattern = format!( + r#"(?m)^{}[ \t]*=[ \t]*['"]?([^'"\n]*)['"]?"#, + regex::escape(key) + ); + regex::Regex::new(&pattern) + .ok()? + .captures(&content) + .and_then(|caps| caps.get(1)) + .map(|m| m.as_str().trim().to_string()) + .filter(|s| !s.is_empty()) +} + /// Resolve a directory's real git "common dir" — the directory holding the /// repository's shared object database and refs. /// diff --git a/laravel-lsp/src/database.rs b/laravel-lsp/src/database.rs index ba2bf161..a4090ed6 100644 --- a/laravel-lsp/src/database.rs +++ b/laravel-lsp/src/database.rs @@ -1417,36 +1417,9 @@ impl DatabaseSchemaProvider { /// whatever line followed), which was sent as the literal password /// to MySQL and rejected as bad credentials. fn resolve_env(&self, key: &str) -> Option { - let env_path = crate::config::resolve_worktree_fallback(&self.project_root, ".env"); - let content = match std::fs::read_to_string(&env_path) { - Ok(c) => c, - Err(e) => { - debug!("🗄️ resolve_env({}): Failed to read .env: {}", key, e); - return None; - } - }; - - // Pattern: KEY=value or KEY="value" or KEY='value', all on one - // line. `[ \t]*` stays horizontal so a blank value (`KEY=\n`) - // captures the empty string, not the next line. - let pattern = format!( - r#"(?m)^{}[ \t]*=[ \t]*['"]?([^'"\n]*)['"]?"#, - regex::escape(key) - ); - let regex = match Regex::new(&pattern) { - Ok(r) => r, - Err(e) => { - debug!("🗄️ resolve_env({}): Invalid regex: {}", key, e); - return None; - } - }; - - let result = regex - .captures(&content) - .and_then(|caps| caps.get(1)) - .map(|m| m.as_str().trim().to_string()) - .filter(|s| !s.is_empty()); - + // Delegates to the single hardened reader in `config` — see its doc + // comment for why this logic must not be duplicated. + let result = crate::config::read_env_value(&self.project_root, key); debug!("🗄️ resolve_env({}): {:?}", key, result); result } diff --git a/laravel-lsp/src/hover.rs b/laravel-lsp/src/hover.rs index aaaa978d..383751e6 100644 --- a/laravel-lsp/src/hover.rs +++ b/laravel-lsp/src/hover.rs @@ -49,6 +49,13 @@ pub const FILE_NOT_FOUND_TRAILER: &str = "*(file not found)*"; /// the production string. pub const TRANSLATION_NOT_FOUND_TRAILER: &str = "*(translation not found for default locale)*"; +/// Trailer for the multi-locale card: the key resolved in none of the locales +/// the project actually defines. Distinct from +/// [`TRANSLATION_NOT_FOUND_TRAILER`], which still says "default locale" because +/// the single-locale [`translation_card`] path really did only consult one. +pub const TRANSLATION_NOT_FOUND_ANY_LOCALE_TRAILER: &str = + "*(translation not found in any locale)*"; + /// Anything the cursor might be hovering. Pattern variants come straight from /// the Salsa position index; the Blade-variable variant is extracted by line /// scanning, and only matters in `.blade.php` files. @@ -101,6 +108,11 @@ pub struct HoverContent<'a> { pub code: Option>, /// Italic tag lines (`@param`, `@return`, `@throws`). pub tags: &'a [String], + /// One line per item in a repeated section — the multi-locale translation + /// card's `**de** — “…” · [link]` rows. Rendered as a single block with + /// markdown hard breaks between rows, so N entries render as N adjacent + /// lines rather than N paragraphs. + pub lines: &'a [String], /// Pre-built markdown link string for the source location (e.g. /// `[app/Models/User.php:42](file:///abs/path)`). Rendered verbatim /// — no `at` prefix, no surrounding backticks. @@ -150,6 +162,11 @@ pub fn render(content: &HoverContent<'_>) -> String { }; sections.push(block); } + if !content.lines.is_empty() { + // Two trailing spaces is markdown's hard line break — a bare "\n" + // would let adjacent rows run together into one paragraph. + sections.push(content.lines.join(" \n")); + } if !content.tags.is_empty() { let tag_lines = content .tags @@ -205,32 +222,65 @@ pub fn translation_card( }) } -/// Multi-locale variant of [`translation_card`]: one value line per locale -/// that defines the key, each with its own source link, so a `de` + `en` -/// catalogue shows both translations at once. Locales that don't define the -/// key are skipped; when none does, the not-found trailer renders instead. +/// Multi-locale variant of [`translation_card`]: one line per locale that +/// defines the key, each carrying its own source link inline, so a `de` + `en` +/// catalogue shows both translations at once. +/// +/// Three shapes, by how many locales actually *resolve* the key — which is not +/// the same as how many locale directories exist, since a project can define a +/// dozen locales and still have only one of them carry this key: +/// +/// - **None** — the not-found trailer, naming that no locale had it. +/// - **Exactly one** — collapses to [`translation_card`]'s dense single-block +/// form. Most projects ship one locale, and stacking a lone value under a +/// locale heading would cost a line to say nothing. +/// - **More than one** — a line per locale: +/// +/// ```text +/// `failed_title` +/// +/// **de** — “Analyse fehlgeschlagen” · [lang/de/contract.php](file://…) +/// **en** — “Analysis failed” · [lang/en/contract.php](file://…) +/// ``` pub fn translation_card_locales( key: &str, entries: &[(String, Option, Option)], ) -> String { - let mut body = format!("`{}`", leaf_segment(key)); - let mut any = false; - for (locale, value, source_link) in entries { - let Some(value) = value else { - continue; - }; - any = true; - body.push_str(&format!("\n\n**{locale}** — “{value}”")); - if let Some(link) = source_link { - body.push_str("\n\n"); - body.push_str(link); + let detail = format!("`{}`", leaf_segment(key)); + let resolved: Vec<&(String, Option, Option)> = entries + .iter() + .filter(|(_, value, _)| value.is_some()) + .collect(); + + match resolved.as_slice() { + [] => render(&HoverContent { + detail: Some(&detail), + trailer: Some(TRANSLATION_NOT_FOUND_ANY_LOCALE_TRAILER), + ..Default::default() + }), + [(locale, value, source_link)] => { + translation_card(key, locale, value.as_deref(), source_link.as_deref()) + } + _ => { + let lines: Vec = resolved + .iter() + .map(|(locale, value, source_link)| { + // Curly quotes delimit the value so it can't be mistaken + // for the key or a path — same rule as `translation_card`. + let quoted = value.as_deref().unwrap_or_default(); + match source_link { + Some(link) => format!("**{locale}** — “{quoted}” · {link}"), + None => format!("**{locale}** — “{quoted}”"), + } + }) + .collect(); + render(&HoverContent { + detail: Some(&detail), + lines: &lines, + ..Default::default() + }) } } - if !any { - body.push_str("\n\n"); - body.push_str(TRANSLATION_NOT_FOUND_TRAILER); - } - body } /// The leaf of a translation key: the last `.`-segment, after dropping any diff --git a/laravel-lsp/src/hover/tests.rs b/laravel-lsp/src/hover/tests.rs index 861f2559..3bd9af0b 100644 --- a/laravel-lsp/src/hover/tests.rs +++ b/laravel-lsp/src/hover/tests.rs @@ -62,6 +62,7 @@ fn render_full_section_set_in_order() { "@param mixed $x".to_string(), "@return Response".to_string(), ]; + let lines = vec!["**de** — “eins”".to_string(), "**en** — “one”".to_string()]; let out = render(&HoverContent { header: Some("App\\Foo::bar"), detail: Some("Some detail line"), @@ -71,6 +72,7 @@ fn render_full_section_set_in_order() { content: "public function bar()", }), tags: &tags, + lines: &lines, source_link: Some("[app/Foo.php:10](file:///abs/Foo.php#L10)"), trailer: None, }); @@ -85,6 +87,9 @@ fn render_full_section_set_in_order() { public function bar()\n\ ```\n\ \n\ + **de** — “eins” \n\ + **en** — “one”\n\ + \n\ *@param mixed $x*\n\ \n\ *@return Response*\n\ @@ -728,7 +733,7 @@ fn translation_card_without_value_shows_not_found_trailer() { } #[test] -fn translation_card_locales_renders_every_defined_locale() { +fn translation_card_locales_renders_one_line_per_locale_with_inline_links() { let card = translation_card_locales( "legal::contract.prefill.failed_title", &[ @@ -742,16 +747,62 @@ fn translation_card_locales_renders_every_defined_locale() { Some("Analysis failed".to_string()), Some("[lang/en/contract.php](file:///y)".to_string()), ), + // A locale that resolved but whose source file couldn't be linked — + // it must still occupy exactly one line, not collapse or double. + ("fr".to_string(), Some("Analyse échouée".to_string()), None), ], ); + assert!(card.starts_with("`failed_title`")); - assert!(card.contains("**de** — “Analyse fehlgeschlagen”")); - assert!(card.contains("**en** — “Analysis failed”")); - assert!(card.contains("[lang/de/contract.php](file:///x)")); + assert!(card.contains("**de** — “Analyse fehlgeschlagen” · [lang/de/contract.php](file:///x)")); + assert!(card.contains("**en** — “Analysis failed” · [lang/en/contract.php](file:///y)")); + assert!(card.contains("**fr** — “Analyse échouée”")); + + // Three locales → three adjacent lines in one block, links inline. A + // paragraph-delimited render would put a blank line between every row and + // between each row and its link. + let block = card.split("\n\n").nth(1).expect("a locale block"); + assert_eq!( + block.lines().count(), + 3, + "expected 3 locale lines, got:\n{block}" + ); +} + +#[test] +fn translation_card_locales_collapses_when_only_one_locale_resolves() { + // Two locales discovered, one defines the key: the card must be the dense + // single-block form, not a one-row list. + let card = translation_card_locales( + "messages.welcome", + &[ + ("de".to_string(), None, None), + ( + "en".to_string(), + Some("Welcome".to_string()), + Some("[lang/en/messages.php](file:///y)".to_string()), + ), + ], + ); + + assert_eq!( + card, + translation_card( + "messages.welcome", + "en", + Some("Welcome"), + Some("[lang/en/messages.php](file:///y)") + ), + "a single resolving locale must render identically to translation_card" + ); + assert!( + !card.contains("**en**"), + "no locale heading in the dense form" + ); } #[test] -fn translation_card_locales_skips_missing_and_falls_back_to_trailer() { +fn translation_card_locales_uses_the_any_locale_trailer_when_none_resolve() { let card = translation_card_locales( "messages.welcome", &[ @@ -759,6 +810,28 @@ fn translation_card_locales_skips_missing_and_falls_back_to_trailer() { ("en".to_string(), None, None), ], ); - assert!(card.contains(TRANSLATION_NOT_FOUND_TRAILER)); + assert_eq!( + card, + format!("`welcome`\n\n{TRANSLATION_NOT_FOUND_ANY_LOCALE_TRAILER}") + ); assert!(!card.contains("**de**")); } + +/// The two trailers say different things and must keep saying them: the +/// single-locale card really did consult only one locale, the multi-locale +/// card consulted every locale the project defines. +#[test] +fn the_two_not_found_trailers_have_distinct_wording() { + assert_eq!( + TRANSLATION_NOT_FOUND_TRAILER, + "*(translation not found for default locale)*" + ); + assert_eq!( + TRANSLATION_NOT_FOUND_ANY_LOCALE_TRAILER, + "*(translation not found in any locale)*" + ); + assert_ne!( + TRANSLATION_NOT_FOUND_TRAILER, + TRANSLATION_NOT_FOUND_ANY_LOCALE_TRAILER + ); +} diff --git a/laravel-lsp/src/main.rs b/laravel-lsp/src/main.rs index 79af7dcb..d47b8300 100644 --- a/laravel-lsp/src/main.rs +++ b/laravel-lsp/src/main.rs @@ -14389,6 +14389,18 @@ impl LaravelLanguageServer { }; // Laravel 9+ uses lang/, older versions use resources/lang/ + // + // Deliberately NOT routed through + // `translation_lookup::available_locales`, which the hover / + // go-to-definition / diagnostics trio share (issue #288). That helper + // answers "which locales could define *this key*" and unions every + // candidate directory; completion asks a different question — "what + // keys exist at all" — and any single locale answers it, because a key + // present in one locale is offered regardless of which locale defines + // it. Enumerating the union here would read every catalogue in the + // project on each completion request to produce the same list, and + // would surface a key from a partially-translated locale as though it + // were project-wide. First-wins is the right shape for this caller. let lang_dirs = [root.join("lang"), root.join("resources").join("lang")]; let lang_dir = lang_dirs.iter().find(|d| d.exists()); @@ -15074,43 +15086,77 @@ return [ // Translation validation helpers // ======================================================================== - /// Check if a translation file exists for the given key + /// Does the project define this translation key anywhere? + /// + /// The key is resolved through [`laravel_lsp::translation_lookup`] against + /// **every locale** the project defines, in the order + /// [`laravel_lsp::translation_lookup::available_locales`] returns — the + /// same set and order hover renders and go-to-definition navigates, so all + /// three agree about a key present only in, say, `de`. + /// + /// All three key shapes go through that one resolver: dotted + /// (`validation.required` → `{lang_root}/{locale}/validation.php`), text + /// (`Welcome to our app` → `{lang_root}/{locale}.json`), and namespaced + /// (`filament-tables::table.label` → published `lang/vendor//` first, + /// then the package's own lang dir via `vendor_map`, see + /// [`laravel_lsp::vendor_translations`]). `{lang_root}` is `lang/` or + /// `resources/lang/`. /// - /// Dotted keys like "validation.required" look in lang/en/validation.php. - /// Text keys like "Welcome to our app" look in lang/en.json. - /// Namespaced keys like "filament-tables::table.label" resolve through - /// [`laravel_lsp::translation_lookup`] — published `lang/vendor//` - /// first, then the package's own lang dir via `vendor_map` (see - /// [`laravel_lsp::vendor_translations`]) — the same machinery hover uses, - /// so the diagnostic and hover can't disagree. + /// Resolution is by *reading the key*, never by probing for the file: a + /// lang file that exists but does not define this key is not evidence the + /// key exists, and reporting it as such is how diagnostics and hover came + /// to disagree in the first place (issue #288). + /// + /// `expected_path` — where a missing key should be created — points at the + /// leading locale, which is the project's `APP_LOCALE` when it defines any + /// translations at all. fn check_translation_file( root: &Path, translation_key: &str, vendor_map: Option<&HashMap>, ) -> TranslationCheck { + use laravel_lsp::translation_lookup::{ + available_locales, project_lang_roots, resolve_translation_detailed, + }; + + // The same locale set, in the same order, that hover renders and + // go-to-definition navigates against — so a key defined only in `de` + // is not flagged missing while the hover happily displays it + // (issue #288). The key is *resolved* in each locale rather than the + // lang file merely probed for existence: a file that exists but does + // not define this key is not evidence the key exists. + let locales = available_locales(root, translation_key, vendor_map); + let resolution = locales.iter().find_map(|locale| { + resolve_translation_detailed(root, translation_key, locale, vendor_map) + .map(|r| (locale.clone(), r)) + }); + // Where a missing key should be created: the leading locale, which is + // the project's APP_LOCALE when it defines any translations at all. + let lead_locale = locales + .first() + .cloned() + .expect("available_locales never yields an empty set"); + if let Some((namespace, rest)) = translation_key.split_once("::") { let file_segment = rest.split('.').next().unwrap_or(rest); let nested_key = rest.split_once('.').map(|(_, k)| k.to_string()); - let exists = laravel_lsp::translation_lookup::resolve_translation_detailed( - root, - translation_key, - "en", - vendor_map, - ) - .is_some(); - // Expected location for the diagnostic message: the package's real // lang dir when the vendor scan knows it, else the published path. let lang_dir = vendor_map .and_then(|m| m.get(namespace).cloned()) .unwrap_or_else(|| root.join("lang/vendor").join(namespace)); - let expected = lang_dir.join("en").join(format!("{file_segment}.php")); + let expected = lang_dir + .join(&lead_locale) + .join(format!("{file_segment}.php")); return TranslationCheck { - exists, + exists: resolution.is_some(), is_dotted_key: true, - file_exists: expected.exists(), + file_exists: resolution + .as_ref() + .map(|(_, r)| r.source_file.exists()) + .unwrap_or_else(|| expected.exists()), expected_path: Some(expected), nested_key, }; @@ -15119,71 +15165,43 @@ return [ let is_dotted_key = translation_key.contains('.') && !translation_key.contains(' '); let is_multi_word = translation_key.contains(' '); - let mut exists = false; - let mut expected_path: Option = None; - let mut file_exists = false; - let mut nested_key: Option = None; - - if is_multi_word || (!is_dotted_key && !translation_key.contains('.')) { - // Text key: check JSON files for the KEY, not just file existence - let json_paths = [ - root.join("lang/en.json"), - root.join("resources/lang/en.json"), - ]; - - // Set the expected path to the first option (preferred location) - expected_path = Some(json_paths[0].clone()); - nested_key = Some(translation_key.to_string()); - - for json_path in &json_paths { - if json_path.exists() { - file_exists = true; - expected_path = Some(json_path.clone()); - // Parse JSON and check if key exists - if let Ok(content) = std::fs::read_to_string(json_path) { - if let Ok(json) = serde_json::from_str::(&content) { - if json.get(translation_key).is_some() { - exists = true; - break; - } - } - } - break; // Use the first existing file - } - } - } else if is_dotted_key { - // Dotted key: check PHP file based on first segment - let parts: Vec<&str> = translation_key.split('.').collect(); - if !parts.is_empty() { - let file_name = parts[0]; - // The nested key is everything after the first dot - nested_key = Some(parts[1..].join(".")); - - let php_paths = [ - root.join("lang/en").join(format!("{}.php", file_name)), - root.join("resources/lang/en") - .join(format!("{}.php", file_name)), - ]; - - // Set the expected path to the first option (preferred location) - expected_path = Some(php_paths[0].clone()); - - for php_path in &php_paths { - if php_path.exists() { - file_exists = true; - exists = true; // For PHP, we only check file existence currently - expected_path = Some(php_path.clone()); - break; - } - } - } - } + // A text key lives in `{lang_root}/{locale}.json`; a dotted key in + // `{lang_root}/{locale}/{file}.php`. Both lang roots are candidates, + // matching what the resolver itself searches. + let (expected_path, nested_key) = if is_multi_word || !is_dotted_key { + let candidates: Vec = project_lang_roots(root) + .iter() + .map(|lang| lang.join(format!("{lead_locale}.json"))) + .collect(); + let expected = candidates + .iter() + .find(|p| p.exists()) + .cloned() + .unwrap_or_else(|| candidates[0].clone()); + (Some(expected), Some(translation_key.to_string())) + } else { + let file_name = translation_key.split('.').next().unwrap_or(translation_key); + let nested = translation_key.split_once('.').map(|(_, k)| k.to_string()); + let candidates: Vec = project_lang_roots(root) + .iter() + .map(|lang| lang.join(&lead_locale).join(format!("{file_name}.php"))) + .collect(); + let expected = candidates + .iter() + .find(|p| p.exists()) + .cloned() + .unwrap_or_else(|| candidates[0].clone()); + (Some(expected), nested) + }; TranslationCheck { - exists, + exists: resolution.is_some(), is_dotted_key, + file_exists: resolution + .as_ref() + .map(|(_, r)| r.source_file.exists()) + .unwrap_or_else(|| expected_path.as_ref().is_some_and(|p| p.exists())), expected_path, - file_exists, nested_key, } } @@ -16094,47 +16112,41 @@ return [ ) -> Option { let root_guard = self.root_path.read().await; let root = root_guard.as_ref()?; + let vendor_map = self.vendor_translation_namespaces_for(root).await; + let map_ref = vendor_map.as_ref().map(|m| m.as_ref()); - // Resolve the target lang file. Three shapes, mirroring - // `check_translation_file` / `hover_for_translation` so navigation, - // diagnostics, and hover all agree on where a key lives: - // - namespaced (`app::file.key`) → /en/.php, where - // comes from the merged vendor/app-provider map (falling back to - // the published `lang/vendor/` path) - // - dotted (`validation.required`) → lang/en/.php - // - text (`Welcome to our app`) → lang/en.json - // For PHP files, `php_key_path` is the nested array path to the key - // *within* the file (the segments after the file name), used to jump to - // the key's exact line. `None` marks a JSON text key. - let (translation_path, php_key_path) = - if let Some((namespace, rest)) = trans.key.split_once("::") { - let mut segments = rest.split('.'); - let file_segment = segments.next().unwrap_or(rest); - let key_path: Vec<&str> = segments.collect(); - let vendor_map = self.vendor_translation_namespaces_for(root).await; - let lang_dir = vendor_map - .as_ref() - .and_then(|m| m.get(namespace).cloned()) - .unwrap_or_else(|| root.join("lang/vendor").join(namespace)); - ( - lang_dir.join("en").join(format!("{file_segment}.php")), - Some(key_path), - ) - } else if trans.key.contains('.') && !trans.key.contains(' ') { - // Dotted key: "validation.required" -> lang/en/validation.php - let mut segments = trans.key.split('.'); - let file = segments.next().unwrap_or(&trans.key); - let key_path: Vec<&str> = segments.collect(); - ( - root.join("lang").join("en").join(format!("{file}.php")), - Some(key_path), + // Resolve the key for real, across every locale the project defines, + // in the same APP_LOCALE-led order hover renders and diagnostics + // validate against. Navigation, diagnostics and hover therefore agree + // on where a key lives *and* on which locale wins when several define + // it — a key present only in `de` navigates to the `de` file instead + // of silently failing against a hardcoded `en` path (issue #288). + // + // Resolving rather than probing for the file also means a locale whose + // lang file exists but does not define this key is correctly skipped, + // instead of navigating to a file that never had the key in it. + let locales = laravel_lsp::translation_lookup::available_locales(root, &trans.key, map_ref); + let translation_path = locales + .iter() + .find_map(|locale| { + laravel_lsp::translation_lookup::resolve_translation_detailed( + root, &trans.key, locale, map_ref, ) - } else { - // Text key: "Welcome to our app" -> lang/en.json - (root.join("lang").join("en.json"), None) - }; + })? + .source_file; + + // The nested array path to the key *within* the file (the segments + // after the file name), used to jump to the key's exact line. `None` + // marks a JSON text key, whose key is the string itself. + let php_key_path: Option> = if let Some((_, rest)) = trans.key.split_once("::") { + Some(rest.split('.').skip(1).collect()) + } else if trans.key.contains('.') && !trans.key.contains(' ') { + Some(trans.key.split('.').skip(1).collect()) + } else { + None + }; - if self.file_exists_cached(&translation_path).await { + { if let Ok(target_uri) = Url::from_file_path(&translation_path) { let origin_selection_range = Range { start: Position { @@ -20071,75 +20083,34 @@ return [ let vendor_map = self.vendor_translation_namespaces_for(r).await; let map_ref = vendor_map.as_ref().map(|m| m.as_ref()); - // Every locale that could define the key: the locale directories - // (and `{locale}.json` catalogues) of the key's lang dir(s) — the - // published vendor override plus the registered namespace dir for - // namespaced keys, the project lang dirs otherwise. - let mut lang_dirs: Vec = Vec::new(); - if let Some((namespace, _)) = key.split_once("::") { - lang_dirs.push(r.join("lang/vendor").join(namespace)); - if let Some(dir) = map_ref.and_then(|m| m.get(namespace)) { - lang_dirs.push(dir.clone()); - } - } else { - lang_dirs.push(r.join("lang")); - lang_dirs.push(r.join("resources/lang")); - } - let mut locales: Vec = Vec::new(); - for dir in &lang_dirs { - let Ok(dir_entries) = std::fs::read_dir(dir) else { - continue; - }; - for entry in dir_entries.flatten() { - let path = entry.path(); - let locale = if path.is_dir() { - path.file_name() - .and_then(|n| n.to_str()) - .map(str::to_string) - } else if path.extension().is_some_and(|e| e == "json") { - path.file_stem() - .and_then(|n| n.to_str()) - .map(str::to_string) - } else { - None - }; - if let Some(locale) = locale { - if locale != "vendor" && !locales.contains(&locale) { - locales.push(locale); - } - } - } - } - locales.sort(); - if locales.is_empty() { - locales.push("en".to_string()); - } - let mut entries: Vec<(String, Option, Option)> = Vec::new(); - for locale in &locales { + for locale in laravel_lsp::translation_lookup::available_locales(r, key, map_ref) { let resolution = laravel_lsp::translation_lookup::resolve_translation_detailed( - r, key, locale, map_ref, + r, key, &locale, map_ref, ); let link = match &resolution { Some(res) => Some(self.source_link(&res.source_file, None).await), None => None, }; - // Translation values are PHP literals (`'foo'`) — strip the outer - // quotes and cap the length for in-block display. let value = resolution.as_ref().map(|res| { - let v = res.value.trim(); - let unquoted = v - .strip_prefix('\'') - .and_then(|s| s.strip_suffix('\'')) - .or_else(|| v.strip_prefix('"').and_then(|s| s.strip_suffix('"'))) - .unwrap_or(v); - hover::truncate_for_display(unquoted, 200) + hover::truncate_for_display(&Self::unquote_php_literal(&res.value), 200) }); - entries.push((locale.clone(), value, link)); + entries.push((locale, value, link)); } hover::translation_card_locales(key, &entries) } + /// Strip the outer quotes from a PHP string literal (`'foo'` / `"foo"`). + /// Translation values arrive as raw literals from the PHP-array walker. + fn unquote_php_literal(value: &str) -> String { + let v = value.trim(); + v.strip_prefix('\'') + .and_then(|s| s.strip_suffix('\'')) + .or_else(|| v.strip_prefix('"').and_then(|s| s.strip_suffix('"'))) + .unwrap_or(v) + .to_string() + } + /// Middleware — header is the alias's class FQN (the new info beyond /// the cursor's `'auth'` string). async fn hover_for_middleware(&self, name: &str) -> String { diff --git a/laravel-lsp/src/tests/mod.rs b/laravel-lsp/src/tests/mod.rs index 985f691b..15d442d2 100644 --- a/laravel-lsp/src/tests/mod.rs +++ b/laravel-lsp/src/tests/mod.rs @@ -47,6 +47,7 @@ mod routes_dir_gate; mod scan_dir_containment; mod slot_navigation_containment; mod slot_variable_resolution; +mod translation_locale_consistency; mod translation_namespace_check; mod translation_namespace_navigation; mod view_diagnostic_containment; diff --git a/laravel-lsp/src/tests/translation_locale_consistency.rs b/laravel-lsp/src/tests/translation_locale_consistency.rs new file mode 100644 index 00000000..98d91025 --- /dev/null +++ b/laravel-lsp/src/tests/translation_locale_consistency.rs @@ -0,0 +1,163 @@ +//! Hover, go-to-definition and diagnostics must agree about a translation key. +//! +//! All three used to hardcode `"en"`. #287 made hover multi-locale and left the +//! other two behind, so a key defined only in `de` rendered in the hover, +//! refused to navigate, and was still reported missing by diagnostics — the +//! comment claiming "navigation, diagnostics, and hover all agree on where a +//! key lives" was simply false (issue #288). +//! +//! These tests drive all three paths against one fixture and assert they land +//! on the same file, for both key shapes the issue names: a plain dotted key +//! and a namespaced one. + +use crate::LaravelLanguageServer; +use laravel_lsp::salsa_impl::TranslationReferenceData; +use std::path::{Path, PathBuf}; +use tempfile::TempDir; +use tower_lsp::{lsp_types::GotoDefinitionResponse, LspService}; + +fn backend() -> LaravelLanguageServer { + let (service, _socket) = LspService::new(LaravelLanguageServer::new); + service.inner().clone() +} + +fn reference(key: &str) -> TranslationReferenceData { + TranslationReferenceData { + key: key.to_string(), + line: 0, + column: 0, + end_column: key.len() as u32, + } +} + +/// The single file goto navigated to, or `None`. +fn goto_target(response: Option) -> Option { + match response? { + GotoDefinitionResponse::Link(links) => links.first()?.target_uri.to_file_path().ok(), + _ => None, + } +} + +/// A project whose ONLY locale is `de` — no `en` anywhere, which is what broke +/// the hardcoded paths. +fn de_only_project() -> (TempDir, PathBuf) { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().to_path_buf(); + let de = root.join("lang/de"); + std::fs::create_dir_all(&de).unwrap(); + std::fs::write( + de.join("contract.php"), + " ['failed_title' => 'Analyse fehlgeschlagen']];", + ) + .unwrap(); + (tmp, root) +} + +async fn assert_all_three_agree( + backend: &LaravelLanguageServer, + root: &Path, + key: &str, + expected_file: &Path, + expected_value: &str, +) { + let hover = backend.hover_for_translation(key, Some(root)).await; + assert!( + hover.contains(expected_value), + "hover must show the value; got:\n{hover}" + ); + assert!( + !hover.contains("not found"), + "hover must not report the key missing; got:\n{hover}" + ); + + let goto = goto_target( + backend + .create_translation_location_from_salsa(&reference(key)) + .await, + ); + assert_eq!( + goto.as_deref(), + Some(expected_file), + "goto must navigate to the same file hover sourced the value from" + ); + + let check = LaravelLanguageServer::check_translation_file(root, key, None); + assert!( + check.exists, + "diagnostics must not report a key the hover displays as missing" + ); +} + +#[tokio::test] +async fn dotted_key_defined_only_in_de_agrees_across_hover_goto_and_diagnostics() { + let (_tmp, root) = de_only_project(); + let backend = backend(); + *backend.root_path.write().await = Some(root.clone()); + + assert_all_three_agree( + &backend, + &root, + "contract.prefill.failed_title", + &root.join("lang/de/contract.php"), + "Analyse fehlgeschlagen", + ) + .await; +} + +#[tokio::test] +async fn namespaced_key_defined_only_in_de_agrees_across_hover_goto_and_diagnostics() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().to_path_buf(); + let de = root.join("lang/vendor/shop/de"); + std::fs::create_dir_all(&de).unwrap(); + std::fs::write( + de.join("messages.php"), + " 'Warenkorb'];", + ) + .unwrap(); + + let backend = backend(); + *backend.root_path.write().await = Some(root.clone()); + + assert_all_three_agree( + &backend, + &root, + "shop::messages.title", + &de.join("messages.php"), + "Warenkorb", + ) + .await; +} + +/// A lang file that exists but does not define the hovered key must not be +/// reported present by diagnostics, nor navigated to by goto — the divergence +/// that "check the file exists" produced. +#[tokio::test] +async fn a_locale_file_without_the_key_is_not_treated_as_a_definition() { + let tmp = TempDir::new().unwrap(); + let root = tmp.path().to_path_buf(); + let en = root.join("lang/en"); + std::fs::create_dir_all(&en).unwrap(); + // The file exists; the key does not. + std::fs::write(en.join("contract.php"), " 'x'];").unwrap(); + + let backend = backend(); + *backend.root_path.write().await = Some(root.clone()); + + let key = "contract.prefill.failed_title"; + let check = LaravelLanguageServer::check_translation_file(&root, key, None); + assert!( + !check.exists, + "an existing lang file without the key is not a definition" + ); + + let goto = goto_target( + backend + .create_translation_location_from_salsa(&reference(key)) + .await, + ); + assert_eq!( + goto, None, + "goto must not navigate to a file lacking the key" + ); +} diff --git a/laravel-lsp/src/translation_lookup.rs b/laravel-lsp/src/translation_lookup.rs index bf8f2a41..ae89196a 100644 --- a/laravel-lsp/src/translation_lookup.rs +++ b/laravel-lsp/src/translation_lookup.rs @@ -89,7 +89,19 @@ fn is_dotted_key(key: &str) -> bool { key.contains('.') && !key.contains(' ') } -/// Resolve a dotted key against `lang/{locale}/{file}.php`. +/// The directories a project may keep translations in, in priority order: +/// `lang/` (Laravel 9+) and `resources/lang/` (Laravel 8 and earlier). +/// +/// Both are checked everywhere, because the diagnostics path has always +/// checked both — resolving only `lang/` left hover unable to find any +/// translation on a Laravel-8-style project while diagnostics happily +/// resolved it, which is precisely the hover/diagnostics divergence issue +/// #288 exists to close. +pub fn project_lang_roots(root: &Path) -> [PathBuf; 2] { + [root.join("lang"), root.join("resources").join("lang")] +} + +/// Resolve a dotted key against `{lang_root}/{locale}/{file}.php`. fn resolve_dotted(root: &Path, key: &str, locale: &str) -> Option { let mut parts = key.split('.'); let file = parts.next()?; @@ -97,8 +109,9 @@ fn resolve_dotted(root: &Path, key: &str, locale: &str) -> Option Option { - let path = root.join("lang").join(format!("{}.json", locale)); - let content = std::fs::read_to_string(&path).ok()?; - let map: serde_json::Map = serde_json::from_str(&content).ok()?; - let value = map.get(key)?.as_str()?; - Some(ResolvedTranslation { - value: format!("'{}'", value), - source_file: path, + project_lang_roots(root).iter().find_map(|lang| { + let path = lang.join(format!("{}.json", locale)); + let content = std::fs::read_to_string(&path).ok()?; + let map: serde_json::Map = + serde_json::from_str(&content).ok()?; + let value = map.get(key)?.as_str()?; + Some(ResolvedTranslation { + value: format!("'{}'", value), + source_file: path, + }) }) } +/// The fallback locale when a project exposes none — Laravel's own default. +const DEFAULT_LOCALE: &str = "en"; + +/// Every locale that could define `key`, ordered with the project's configured +/// `APP_LOCALE` first and the rest alphabetically. +/// +/// Discovery looks at the lang directories the key could live in — the +/// published vendor override plus the registered namespace directory for a +/// namespaced key, the project lang roots otherwise — and treats both locale +/// *subdirectories* and `{locale}.json` catalogues as evidence of a locale. +/// The `vendor` subdirectory is excluded: it holds published package +/// translations, not a locale. +/// +/// Never returns empty. A project with no discoverable locales (no lang +/// directory at all, or one containing nothing) falls back to +/// `["en"]`, so callers always have something to resolve against. +/// +/// This is the single source of truth for "which locales matter for this key" — +/// hover, go-to-definition and diagnostics all resolve against the same set, so +/// a key defined only in `de` renders, navigates and validates consistently. +pub fn available_locales( + root: &Path, + key: &str, + vendor_map: Option<&HashMap>, +) -> Vec { + let mut dirs: Vec = Vec::new(); + if let Some((namespace, _)) = split_namespace(key) { + for lang in project_lang_roots(root) { + dirs.push(lang.join("vendor").join(namespace)); + } + if let Some(dir) = vendor_map.and_then(|m| m.get(namespace)) { + dirs.push(dir.clone()); + } + } else { + dirs.extend(project_lang_roots(root)); + } + + let mut locales: Vec = Vec::new(); + for dir in &dirs { + let Ok(entries) = std::fs::read_dir(dir) else { + continue; + }; + for entry in entries.flatten() { + let path = entry.path(); + let locale = if path.is_dir() { + path.file_name() + .and_then(|n| n.to_str()) + .map(str::to_string) + } else if path.extension().is_some_and(|e| e == "json") { + path.file_stem() + .and_then(|n| n.to_str()) + .map(str::to_string) + } else { + None + }; + // `vendor` is a namespace container, not a locale. Dedupe across + // dirs so a locale present in both the published and unpublished + // vendor directory is listed once. + if let Some(locale) = locale { + if locale != "vendor" && !locales.contains(&locale) { + locales.push(locale); + } + } + } + } + + if locales.is_empty() { + return vec![DEFAULT_LOCALE.to_string()]; + } + + locales.sort(); + // The project's own locale leads; everything else stays alphabetical. An + // APP_LOCALE that no directory defines simply doesn't appear, leaving the + // alphabetical order untouched. + if let Some(app_locale) = crate::config::read_env_value(root, "APP_LOCALE") { + if let Some(idx) = locales.iter().position(|l| *l == app_locale) { + let leading = locales.remove(idx); + locales.insert(0, leading); + } + } + locales +} + /// Shared PHP-file read + walk. Returns the bundled value + source path on hit. fn read_php_value(path: &Path, key_path: &[&str]) -> Option { let content = std::fs::read_to_string(path).ok()?; diff --git a/laravel-lsp/src/translation_lookup/tests.rs b/laravel-lsp/src/translation_lookup/tests.rs index 3991ea9e..51c40608 100644 --- a/laravel-lsp/src/translation_lookup/tests.rs +++ b/laravel-lsp/src/translation_lookup/tests.rs @@ -316,3 +316,193 @@ fn namespaced_dir_outside_root_is_refused() { "an out-of-root namespace directory must never be read" ); } + +// --------------------------------------------------------------------------- +// available_locales — the shared locale set (issue #288) +// --------------------------------------------------------------------------- + +/// A root with the given locale subdirectories under `lang/`. +fn root_with_locales(locales: &[&str]) -> TempDir { + let dir = TempDir::new().unwrap(); + for locale in locales { + fs::create_dir_all(dir.path().join("lang").join(locale)).unwrap(); + } + dir +} + +#[test] +fn available_locales_enumerates_locale_directories() { + let dir = root_with_locales(&["en", "de", "fr"]); + assert_eq!( + available_locales(dir.path(), "messages.welcome", None), + vec!["de", "en", "fr"] + ); +} + +#[test] +fn available_locales_extracts_json_catalogue_stems() { + let dir = TempDir::new().unwrap(); + let lang = dir.path().join("lang"); + fs::create_dir_all(&lang).unwrap(); + fs::write(lang.join("de.json"), "{}").unwrap(); + fs::write(lang.join("en.json"), "{}").unwrap(); + // A non-JSON file is not a locale. + fs::write(lang.join("README.md"), "").unwrap(); + + assert_eq!( + available_locales(dir.path(), "Welcome to our app", None), + vec!["de", "en"] + ); +} + +#[test] +fn available_locales_excludes_the_vendor_directory() { + let dir = root_with_locales(&["en", "de"]); + fs::create_dir_all(dir.path().join("lang/vendor/somepkg/en")).unwrap(); + + let locales = available_locales(dir.path(), "messages.welcome", None); + assert!( + !locales.contains(&"vendor".to_string()), + "vendor is a namespace container, not a locale: {locales:?}" + ); + assert_eq!(locales, vec!["de", "en"]); +} + +#[test] +fn available_locales_falls_back_when_the_lang_directory_is_missing() { + let dir = TempDir::new().unwrap(); + // No lang/ at all — read_dir errors on every candidate. + assert_eq!( + available_locales(dir.path(), "messages.welcome", None), + vec!["en"] + ); +} + +#[test] +fn available_locales_falls_back_when_the_lang_directory_is_empty() { + let dir = TempDir::new().unwrap(); + fs::create_dir_all(dir.path().join("lang")).unwrap(); + // Exists, but holds no locale subdirectories and no .json catalogues. + assert_eq!( + available_locales(dir.path(), "messages.welcome", None), + vec!["en"] + ); +} + +#[test] +fn available_locales_unions_published_and_unpublished_vendor_dirs() { + let dir = TempDir::new().unwrap(); + // Published override defines only `de`. + fs::create_dir_all(dir.path().join("lang/vendor/shop/de")).unwrap(); + // The package's own (unpublished) lang dir defines only `fr`. + let pkg_lang = dir.path().join("vendor/acme/shop/resources/lang"); + fs::create_dir_all(pkg_lang.join("fr")).unwrap(); + + let mut map = HashMap::new(); + map.insert("shop".to_string(), pkg_lang); + + assert_eq!( + available_locales(dir.path(), "shop::messages.title", Some(&map)), + vec!["de", "fr"], + "both directories contribute; neither is consulted in isolation" + ); +} + +#[test] +fn available_locales_deduplicates_overlapping_vendor_dirs() { + let dir = TempDir::new().unwrap(); + fs::create_dir_all(dir.path().join("lang/vendor/shop/de")).unwrap(); + fs::create_dir_all(dir.path().join("lang/vendor/shop/en")).unwrap(); + let pkg_lang = dir.path().join("vendor/acme/shop/resources/lang"); + fs::create_dir_all(pkg_lang.join("de")).unwrap(); + fs::create_dir_all(pkg_lang.join("en")).unwrap(); + + let mut map = HashMap::new(); + map.insert("shop".to_string(), pkg_lang); + + assert_eq!( + available_locales(dir.path(), "shop::messages.title", Some(&map)), + vec!["de", "en"], + "a locale in both directories is listed once, not doubled" + ); +} + +// --- APP_LOCALE ordering ---------------------------------------------------- + +/// `fr` is deliberately NOT alphabetically first among the discovered +/// locales — an implementation that ignored APP_LOCALE entirely, or that only +/// sorted, would still pass a fixture whose APP_LOCALE happened to sort first. +#[test] +fn available_locales_leads_with_app_locale_then_alphabetical() { + let dir = root_with_locales(&["en", "de", "fr", "es"]); + fs::write(dir.path().join(".env"), "APP_NAME=Test\nAPP_LOCALE=fr\n").unwrap(); + + assert_eq!( + available_locales(dir.path(), "messages.welcome", None), + vec!["fr", "de", "en", "es"], + "APP_LOCALE leads; the remainder stays alphabetical" + ); +} + +#[test] +fn available_locales_is_alphabetical_when_app_locale_is_unset() { + let dir = root_with_locales(&["en", "de", "fr", "es"]); + fs::write(dir.path().join(".env"), "APP_NAME=Test\n").unwrap(); + + assert_eq!( + available_locales(dir.path(), "messages.welcome", None), + vec!["de", "en", "es", "fr"] + ); +} + +#[test] +fn available_locales_ignores_an_app_locale_no_directory_defines() { + let dir = root_with_locales(&["en", "de", "fr", "es"]); + fs::write(dir.path().join(".env"), "APP_LOCALE=ja\n").unwrap(); + + assert_eq!( + available_locales(dir.path(), "messages.welcome", None), + vec!["de", "en", "es", "fr"], + "an APP_LOCALE outside the discovered set must not panic or reorder" + ); +} + +// --- resources/lang parity -------------------------------------------------- + +/// A Laravel-8-style project keeps translations under `resources/lang/`. +/// Discovery and resolution must agree there, or hover finds nothing while +/// diagnostics resolves happily — the divergence issue #288 closes. +#[test] +fn resources_lang_only_project_discovers_and_resolves() { + let dir = TempDir::new().unwrap(); + let lang = dir.path().join("resources/lang"); + fs::create_dir_all(lang.join("de")).unwrap(); + fs::write( + lang.join("de/contract.php"), + " 'Vertrag'];", + ) + .unwrap(); + + assert_eq!( + available_locales(dir.path(), "contract.title", None), + vec!["de"], + "discovery must see resources/lang" + ); + let resolved = resolve_translation_detailed(dir.path(), "contract.title", "de", None) + .expect("resolution must see resources/lang too"); + assert_eq!(resolved.value, "'Vertrag'"); + assert_eq!(resolved.source_file, lang.join("de/contract.php")); +} + +#[test] +fn resources_lang_json_text_key_resolves() { + let dir = TempDir::new().unwrap(); + let lang = dir.path().join("resources/lang"); + fs::create_dir_all(&lang).unwrap(); + fs::write(lang.join("de.json"), r#"{"Welcome":"Willkommen"}"#).unwrap(); + + assert_eq!(available_locales(dir.path(), "Welcome", None), vec!["de"]); + let resolved = resolve_translation_detailed(dir.path(), "Welcome", "de", None) + .expect("JSON catalogue under resources/lang must resolve"); + assert_eq!(resolved.value, "'Willkommen'"); +} From 1f7a9b8157d3305f6914f6cc7a6e93df6dbdbed4 Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Mon, 24 Aug 2026 20:09:44 -0700 Subject: [PATCH 2/4] =?UTF-8?q?fix:=20=F0=9F=94=92=EF=B8=8F=20Fence=20loca?= =?UTF-8?q?le=20discovery=20inside=20the=20project=20root.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `available_locales` enumerated the unpublished vendor lang directory without a containment check. That directory is built from a `loadTranslationsFrom` argument in project/vendor source — untrusted input that can point anywhere — so a namespaced key could `read_dir` a tree outside the project and render whatever subdirectories it found there as that key's locale list. `resolve_namespaced_in_dir` has fenced its own read since issue #248; this enumeration is a read site too and was the one that missed the guard. It now takes the same fail-closed `path_within_root` check. Verified by removing the guard: out-of-root `ja`/`ko` directories leak straight into the returned locales. Also corrects the module header, which this branch made false: all three key shapes now resolve under both `lang/` and `resources/lang/`, and the claim that the resolver "only checks the published path" had been untrue since the vendor_map fallback landed. Drops a leftover bare block in `create_translation_location_from_salsa`. --- laravel-lsp/src/main.rs | 62 ++++++++++----------- laravel-lsp/src/translation_lookup.rs | 41 ++++++++++---- laravel-lsp/src/translation_lookup/tests.rs | 29 ++++++++++ 3 files changed, 89 insertions(+), 43 deletions(-) diff --git a/laravel-lsp/src/main.rs b/laravel-lsp/src/main.rs index d47b8300..bebf18f2 100644 --- a/laravel-lsp/src/main.rs +++ b/laravel-lsp/src/main.rs @@ -16146,40 +16146,40 @@ return [ None }; - { - if let Ok(target_uri) = Url::from_file_path(&translation_path) { - let origin_selection_range = Range { - start: Position { - line: trans.line, - character: trans.column, - }, - end: Position { - line: trans.line, - character: trans.end_column, - }, - }; + if let Ok(target_uri) = Url::from_file_path(&translation_path) { + let origin_selection_range = Range { + start: Position { + line: trans.line, + character: trans.column, + }, + end: Position { + line: trans.line, + character: trans.end_column, + }, + }; - // Jump to the key's own line where we can locate it, else the - // top of the file (the file resolved, only the key didn't). - let target_range = match php_key_path.as_deref() { - // JSON text key: line of the `"key":` property. - None => Self::find_json_key_location(&translation_path, &trans.key) - .unwrap_or_default(), - // PHP nested array key: walk the array to the leaf's line. - Some(key_path) if !key_path.is_empty() => { - Self::locate_php_key_range(&translation_path, key_path).unwrap_or_default() - } - Some(_) => Range::default(), - }; + // Jump to the key's own line where we can locate it, else the + // top of the file (the file resolved, only the key didn't). + let target_range = match php_key_path.as_deref() { + // JSON text key: line of the `"key":` property. + None => { + Self::find_json_key_location(&translation_path, &trans.key).unwrap_or_default() + } + // PHP nested array key: walk the array to the leaf's line. + Some(key_path) if !key_path.is_empty() => { + Self::locate_php_key_range(&translation_path, key_path).unwrap_or_default() + } + Some(_) => Range::default(), + }; - return Some(GotoDefinitionResponse::Link(vec![LocationLink { - origin_selection_range: Some(origin_selection_range), - target_uri, - target_range, - target_selection_range: target_range, - }])); - } + return Some(GotoDefinitionResponse::Link(vec![LocationLink { + origin_selection_range: Some(origin_selection_range), + target_uri, + target_range, + target_selection_range: target_range, + }])); } + None } diff --git a/laravel-lsp/src/translation_lookup.rs b/laravel-lsp/src/translation_lookup.rs index ae89196a..60dab514 100644 --- a/laravel-lsp/src/translation_lookup.rs +++ b/laravel-lsp/src/translation_lookup.rs @@ -1,22 +1,30 @@ //! Resolve Laravel translation keys to their localized strings. //! +//! Every shape resolves under `{lang_root}/`, where `{lang_root}` is `lang/` +//! (Laravel 9+) or `resources/lang/` (Laravel 8 and earlier) — both are always +//! searched, in that order. See [`project_lang_roots`]. +//! //! Laravel supports three translation shapes: //! //! - **Dotted keys** (`__('validation.required')`) — resolved through PHP files -//! under `lang/{locale}/`. `validation.required` → `lang/en/validation.php`, -//! key `required`. +//! under `{lang_root}/{locale}/`. `validation.required` → +//! `lang/de/validation.php` on a `de` project, key `required`. //! //! - **Namespaced dotted keys** (`__('filament-tables::table.label')`) — resolved -//! through `lang/vendor/{namespace}/{locale}/{file}.php` (the published -//! location for package translations). Vendor packages that haven't been -//! published still hold their source translations under -//! `vendor/{vendor}/{package}/...` but this resolver only checks the -//! published path. Scanning unpublished package translations is a separate -//! piece of work tracked elsewhere. +//! through `{lang_root}/vendor/{namespace}/{locale}/{file}.php` (the published +//! location for package translations) first, then — when the caller supplies +//! the `vendor_map` built by [`crate::vendor_translations`] — the package's +//! own unpublished lang directory under `vendor/{vendor}/{package}/...`. +//! That directory comes from untrusted source, so every read against it is +//! fenced by [`crate::path_containment`] (issue #248). //! //! - **Text keys** (`__('Welcome to our app')`) — resolved through the single -//! JSON file `lang/{locale}.json`. The key IS the source string and the -//! value is the translated string. +//! JSON file `{lang_root}/{locale}.json`. The key IS the source string and +//! the value is the translated string. +//! +//! No shape assumes a locale. [`available_locales`] answers "which locales +//! could define this key", and hover, go-to-definition and diagnostics all +//! resolve against that one set so they cannot disagree (issue #288). //! //! All three shapes route to the same PHP-array walker from [`config_lookup`] //! since Laravel's `.php` translation files share their exact shape with @@ -195,7 +203,8 @@ const DEFAULT_LOCALE: &str = "en"; /// namespaced key, the project lang roots otherwise — and treats both locale /// *subdirectories* and `{locale}.json` catalogues as evidence of a locale. /// The `vendor` subdirectory is excluded: it holds published package -/// translations, not a locale. +/// translations, not a locale. A registered namespace directory that resolves +/// outside the project root is dropped before it is read (issue #248). /// /// Never returns empty. A project with no discoverable locales (no lang /// directory at all, or one containing nothing) falls back to @@ -214,8 +223,16 @@ pub fn available_locales( for lang in project_lang_roots(root) { dirs.push(lang.join("vendor").join(namespace)); } + // The unpublished vendor dir comes from a `loadTranslationsFrom` + // argument in project/vendor source — untrusted input that can point + // anywhere (issue #248). `resolve_namespaced_in_dir` already fences its + // read; this enumeration is a read site too, so it takes the same + // fail-closed guard rather than `read_dir`-ing an out-of-root directory + // and rendering whatever it finds there as this key's locales. if let Some(dir) = vendor_map.and_then(|m| m.get(namespace)) { - dirs.push(dir.clone()); + if crate::path_containment::path_within_root(dir, root) { + dirs.push(dir.clone()); + } } } else { dirs.extend(project_lang_roots(root)); diff --git a/laravel-lsp/src/translation_lookup/tests.rs b/laravel-lsp/src/translation_lookup/tests.rs index 51c40608..abe8742d 100644 --- a/laravel-lsp/src/translation_lookup/tests.rs +++ b/laravel-lsp/src/translation_lookup/tests.rs @@ -427,6 +427,35 @@ fn available_locales_deduplicates_overlapping_vendor_dirs() { ); } +/// The unpublished vendor dir is built from a `loadTranslationsFrom` argument +/// in untrusted source and can point anywhere. Enumerating it is a read, so it +/// takes the same fail-closed containment guard the namespaced *resolver* +/// already applies (issue #248) — an out-of-root directory must contribute no +/// locales, rather than having its subdirectory names rendered as this key's +/// locale list. +#[test] +fn available_locales_refuses_an_out_of_root_vendor_dir() { + let dir = TempDir::new().unwrap(); + // The published override contributes `de`, so a non-empty result can't be + // mistaken for the "no locales anywhere" fallback. + fs::create_dir_all(dir.path().join("lang/vendor/shop/de")).unwrap(); + + // A sibling tree entirely outside the project root, holding locales that + // must never surface. + let outside = TempDir::new().unwrap(); + fs::create_dir_all(outside.path().join("ja")).unwrap(); + fs::create_dir_all(outside.path().join("ko")).unwrap(); + + let mut map = HashMap::new(); + map.insert("shop".to_string(), outside.path().to_path_buf()); + + assert_eq!( + available_locales(dir.path(), "shop::messages.title", Some(&map)), + vec!["de"], + "an out-of-root vendor dir must contribute nothing" + ); +} + // --- APP_LOCALE ordering ---------------------------------------------------- /// `fr` is deliberately NOT alphabetically first among the discovered From 1e6108e0a650ac708fbc0ca2844a2078c08dd160 Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Mon, 24 Aug 2026 20:52:35 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fix:=20=F0=9F=94=92=EF=B8=8F=20Fence=20ever?= =?UTF-8?q?y=20translation=20read=20against=20the=20project=20root.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `vendor::` namespace of a translation key is lifted verbatim out of parsed PHP/Blade source — including from indexed `vendor/**.php`, so a dependency can choose it. Two sites joined it into a path with no containment check: `resolve_namespaced` read `lang/vendor/{namespace}/{locale}/{file}.php`, and `available_locales` enumerated `lang/vendor/{namespace}`. An absolute or `../`-bearing namespace escaped the tree, and a hit surfaced in the hover tooltip and as a goto-definition target. Rather than patch the two reported sites, route every file read in the module through one `read_in_root` choke point that fail-closes on a path it cannot prove inside the root. That covers the dotted-key and JSON text-key reads too, which had the same shape, and leaves one place a future read site can be forgotten. `resolve_namespaced_in_dir` drops its own inline guard, now redundant. Enumeration takes the same guard, moved to the `read_dir` loop so it applies to every directory rather than only the `vendor_map` one. Five tests pin the guard, each verified to redden when it is removed: absolute and traversing namespaces in the published path, dotted-key and text-key reads through a symlink that escapes the root, and the enumeration twin of the published-path case. --- laravel-lsp/src/translation_lookup.rs | 90 ++++++++---- laravel-lsp/src/translation_lookup/tests.rs | 144 ++++++++++++++++++++ 2 files changed, 207 insertions(+), 27 deletions(-) diff --git a/laravel-lsp/src/translation_lookup.rs b/laravel-lsp/src/translation_lookup.rs index 60dab514..2cebdc65 100644 --- a/laravel-lsp/src/translation_lookup.rs +++ b/laravel-lsp/src/translation_lookup.rs @@ -15,8 +15,6 @@ //! location for package translations) first, then — when the caller supplies //! the `vendor_map` built by [`crate::vendor_translations`] — the package's //! own unpublished lang directory under `vendor/{vendor}/{package}/...`. -//! That directory comes from untrusted source, so every read against it is -//! fenced by [`crate::path_containment`] (issue #248). //! //! - **Text keys** (`__('Welcome to our app')`) — resolved through the single //! JSON file `{lang_root}/{locale}.json`. The key IS the source string and @@ -29,6 +27,13 @@ //! All three shapes route to the same PHP-array walker from [`config_lookup`] //! since Laravel's `.php` translation files share their exact shape with //! config files. +//! +//! Every path this module builds joins segments taken verbatim from a +//! translation key in parsed PHP/Blade source — the `vendor::` namespace, the +//! dotted file segment — or from a `loadTranslationsFrom` argument. All of it +//! is untrusted, so **every** read and directory enumeration here is fenced by +//! [`crate::path_containment`]: reads through [`read_in_root`], enumeration in +//! [`available_locales`] (issue #248). use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -118,12 +123,21 @@ fn resolve_dotted(root: &Path, key: &str, locale: &str) -> Option Option { project_lang_roots(root).iter().find_map(|lang| { let path = lang.join(format!("{}.json", locale)); - let content = std::fs::read_to_string(&path).ok()?; + let content = read_in_root(&path, root)?; let map: serde_json::Map = serde_json::from_str(&content).ok()?; let value = map.get(key)?.as_str()?; @@ -203,8 +215,10 @@ const DEFAULT_LOCALE: &str = "en"; /// namespaced key, the project lang roots otherwise — and treats both locale /// *subdirectories* and `{locale}.json` catalogues as evidence of a locale. /// The `vendor` subdirectory is excluded: it holds published package -/// translations, not a locale. A registered namespace directory that resolves -/// outside the project root is dropped before it is read (issue #248). +/// translations, not a locale. Any directory that resolves outside the project +/// root — a published path whose `vendor::` namespace carries traversal, or a +/// registered namespace directory seeded by `loadTranslationsFrom` — is dropped +/// before it is read (issue #248). /// /// Never returns empty. A project with no discoverable locales (no lang /// directory at all, or one containing nothing) falls back to @@ -223,16 +237,8 @@ pub fn available_locales( for lang in project_lang_roots(root) { dirs.push(lang.join("vendor").join(namespace)); } - // The unpublished vendor dir comes from a `loadTranslationsFrom` - // argument in project/vendor source — untrusted input that can point - // anywhere (issue #248). `resolve_namespaced_in_dir` already fences its - // read; this enumeration is a read site too, so it takes the same - // fail-closed guard rather than `read_dir`-ing an out-of-root directory - // and rendering whatever it finds there as this key's locales. if let Some(dir) = vendor_map.and_then(|m| m.get(namespace)) { - if crate::path_containment::path_within_root(dir, root) { - dirs.push(dir.clone()); - } + dirs.push(dir.clone()); } } else { dirs.extend(project_lang_roots(root)); @@ -240,6 +246,16 @@ pub fn available_locales( let mut locales: Vec = Vec::new(); for dir in &dirs { + // Enumeration is a read site, so it takes the same fail-closed guard + // the resolver reads take (issue #248). Both namespaced dirs are built + // from untrusted input that can point anywhere: the published path + // joins the `vendor::` namespace lifted verbatim out of parsed source, + // and the unpublished dir comes from a `loadTranslationsFrom` argument. + // Without the guard, `read_dir` on an escaped directory would surface + // whatever subdirectories it found there as this key's locales. + if !crate::path_containment::path_within_root(dir, root) { + continue; + } let Ok(entries) = std::fs::read_dir(dir) else { continue; }; @@ -284,9 +300,29 @@ pub fn available_locales( locales } +/// Every *file* read in this module goes through here (directory enumeration +/// carries its own copy of the guard, in [`available_locales`]). Every path +/// here is built by joining segments lifted verbatim out of a translation key +/// in parsed PHP/Blade source — the `vendor::` namespace, the dotted file +/// segment — or, for the unpublished fallback, a `loadTranslationsFrom` +/// directory. All of that is untrusted and can carry `../` traversal, so +/// containment is checked here, once, for every read rather than at each +/// caller where it can be forgotten (issue #248). +/// +/// **Fail-closed**: a path that cannot be proven inside `root` is refused, not +/// read. `path_within_root` canonicalizes, so an under-root symlink pointing +/// out of the tree is refused too. +fn read_in_root(path: &Path, root: &Path) -> Option { + if !crate::path_containment::path_within_root(path, root) { + return None; + } + std::fs::read_to_string(path).ok() +} + /// Shared PHP-file read + walk. Returns the bundled value + source path on hit. -fn read_php_value(path: &Path, key_path: &[&str]) -> Option { - let content = std::fs::read_to_string(path).ok()?; +/// The read is fenced inside `root` by [`read_in_root`]. +fn read_php_value(root: &Path, path: &Path, key_path: &[&str]) -> Option { + let content = read_in_root(path, root)?; let value = config_lookup::resolve_in_source(&content, key_path)?; Some(ResolvedTranslation { value, diff --git a/laravel-lsp/src/translation_lookup/tests.rs b/laravel-lsp/src/translation_lookup/tests.rs index abe8742d..74255f98 100644 --- a/laravel-lsp/src/translation_lookup/tests.rs +++ b/laravel-lsp/src/translation_lookup/tests.rs @@ -317,6 +317,127 @@ fn namespaced_dir_outside_root_is_refused() { ); } +/// A sibling tree outside any project root, holding a `{locale}/{file}.php` +/// whose value must never surface. Returns the temp dir that owns it. +fn secret_tree_outside_any_root() -> TempDir { + let outside = TempDir::new().unwrap(); + fs::create_dir_all(outside.path().join("en")).unwrap(); + fs::write( + outside.path().join("en").join("invoice.php"), + " 'LEAKED'];\n", + ) + .unwrap(); + outside +} + +#[test] +fn absolute_namespace_in_the_published_path_is_refused() { + // `namespace` is the `vendor::` prefix lifted verbatim out of parsed source + // — including from an indexed `vendor/**.php` file, so a compromised + // dependency can choose it. An *absolute* namespace wins outright, because + // `Path::join` discards everything to its left: the published path + // `lang/vendor/{namespace}/en/invoice.php` collapses to + // `{namespace}/en/invoice.php`, straight out of the tree. + let outside = secret_tree_outside_any_root(); + let project = fake_project_with_lang(); + let key = format!("{}::invoice.total", outside.path().display()); + + // The fixture is only worth anything if the unguarded path really would + // land on the secret file — pin that, so this can never rot into a test + // that passes because the join went nowhere. + assert!( + project + .path() + .join("lang") + .join("vendor") + .join(outside.path()) + .join("en") + .join("invoice.php") + .exists(), + "fixture wiring: the published path must actually reach the secret file" + ); + + assert!( + resolve_translation_detailed(project.path(), &key, "en", None).is_none(), + "an absolute namespace must never escape the project root" + ); +} + +#[test] +fn traversing_namespace_in_the_published_path_is_refused() { + // The relative shape of the same hole: `../../../{sibling}` walks out of + // `lang/vendor/` into a tree beside the project root. + let outside = secret_tree_outside_any_root(); + let project = fake_project_with_lang(); + // `read_to_string` resolves `..` through real directories only, so the + // published prefix has to exist for the traversal to be live at all. + fs::create_dir_all(project.path().join("lang").join("vendor")).unwrap(); + + let sibling = outside.path().file_name().unwrap().to_str().unwrap(); + let namespace = format!("../../../{sibling}"); + let escaped = project + .path() + .join("lang") + .join("vendor") + .join(&namespace) + .join("en") + .join("invoice.php"); + assert!( + escaped.exists(), + "fixture wiring: the traversal must actually reach the secret file \ + (both temp dirs must share a parent)" + ); + + let key = format!("{namespace}::invoice.total"); + assert!( + resolve_translation_detailed(project.path(), &key, "en", None).is_none(), + "a traversing namespace must never escape the project root" + ); +} + +#[test] +fn dotted_key_read_through_an_escaping_symlink_is_refused() { + // Containment is canonical, not textual: `lang/en` is spelled inside the + // root but resolves outside it. The dotted-key read site takes the same + // guard as the namespaced ones. + let outside = secret_tree_outside_any_root(); + let project = TempDir::new().unwrap(); + fs::create_dir_all(project.path().join("lang")).unwrap(); + std::os::unix::fs::symlink( + outside.path().join("en"), + project.path().join("lang").join("en"), + ) + .unwrap(); + assert!( + project.path().join("lang/en/invoice.php").exists(), + "fixture wiring: the symlink must actually reach the secret file" + ); + + assert!( + resolve_translation(project.path(), "invoice.total", "en").is_none(), + "a lang directory symlinked out of the root must never be read" + ); +} + +#[test] +fn text_key_read_through_an_escaping_symlink_is_refused() { + // Same guard on the JSON catalogue read. + let outside = TempDir::new().unwrap(); + fs::write(outside.path().join("en.json"), r#"{"Welcome":"LEAKED"}"#).unwrap(); + + let project = TempDir::new().unwrap(); + std::os::unix::fs::symlink(outside.path(), project.path().join("lang")).unwrap(); + assert!( + project.path().join("lang/en.json").exists(), + "fixture wiring: the symlink must actually reach the secret catalogue" + ); + + assert!( + resolve_translation(project.path(), "Welcome", "en").is_none(), + "a lang directory symlinked out of the root must never be read" + ); +} + // --------------------------------------------------------------------------- // available_locales — the shared locale set (issue #288) // --------------------------------------------------------------------------- @@ -456,6 +577,29 @@ fn available_locales_refuses_an_out_of_root_vendor_dir() { ); } +#[test] +fn available_locales_refuses_an_escaping_published_namespace() { + // The enumeration twin of `absolute_namespace_in_the_published_path_is_refused`: + // the published dir is `lang/vendor/{namespace}`, and an absolute namespace + // collapses that join to the namespace itself. Unguarded, `read_dir` would + // list a directory outside the project and render its subdirectory names as + // this key's locales. + let outside = TempDir::new().unwrap(); + fs::create_dir_all(outside.path().join("ja")).unwrap(); + fs::create_dir_all(outside.path().join("ko")).unwrap(); + + let project = fake_project_with_lang(); + let key = format!("{}::messages.title", outside.path().display()); + + let locales = available_locales(project.path(), &key, None); + assert_eq!( + locales, + vec!["en"], + "an escaping published namespace must contribute no locales, leaving \ + only the default-locale fallback" + ); +} + // --- APP_LOCALE ordering ---------------------------------------------------- /// `fr` is deliberately NOT alphabetically first among the discovered From 650ccf13152fcc27c460894f11e7ecb59090e75d Mon Sep 17 00:00:00 2001 From: Mike Bronner Date: Mon, 24 Aug 2026 22:49:41 -0700 Subject: [PATCH 4/4] =?UTF-8?q?refactor:=20=E2=99=BB=EF=B8=8F=20Pair=20a?= =?UTF-8?q?=20locale's=20translation=20value=20with=20its=20source=20link.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit translation_card_locales took each locale as (locale, Option, Option), which admitted a locale that resolved a value but carried no link. That state cannot arise: a value exists only because a file was read to produce it, and that file's path is exactly what the link is built from — source_link returns a String unconditionally, falling back to a backtick-quoted display path when the URL conversion fails. Pair them as (locale, Option<(value, link)>) so the shape says so. The compiler now rejects the impossible combination instead of a test pretending to cover it. Drops the fixture that exercised the valued-but-linkless case, which was documenting a scenario the resolver has no way to produce. --- laravel-lsp/src/hover.rs | 35 ++++++++++++++++---------------- laravel-lsp/src/hover/tests.rs | 37 ++++++++++++++++++++-------------- laravel-lsp/src/main.rs | 20 +++++++++--------- 3 files changed, 50 insertions(+), 42 deletions(-) diff --git a/laravel-lsp/src/hover.rs b/laravel-lsp/src/hover.rs index 383751e6..8d4fabd9 100644 --- a/laravel-lsp/src/hover.rs +++ b/laravel-lsp/src/hover.rs @@ -226,9 +226,16 @@ pub fn translation_card( /// defines the key, each carrying its own source link inline, so a `de` + `en` /// catalogue shows both translations at once. /// -/// Three shapes, by how many locales actually *resolve* the key — which is not -/// the same as how many locale directories exist, since a project can define a -/// dozen locales and still have only one of them carry this key: +/// Each entry is a locale paired with `Some((value, source_link))` when that +/// locale defines the key, or `None` when it doesn't. Value and link travel +/// together because they cannot occur apart: a value only exists because a +/// file was read to produce it, and that file's path is what the link is built +/// from. Pairing them in the type keeps "resolved but unlinkable" — a state the +/// resolver cannot produce — out of the shape entirely. +/// +/// Three renderings, chosen by how many locales actually *resolve* the key — +/// not how many locale directories exist, since a project can define a dozen +/// locales and have only one carry this key: /// /// - **None** — the not-found trailer, naming that no locale had it. /// - **Exactly one** — collapses to [`translation_card`]'s dense single-block @@ -244,12 +251,12 @@ pub fn translation_card( /// ``` pub fn translation_card_locales( key: &str, - entries: &[(String, Option, Option)], + entries: &[(String, Option<(String, String)>)], ) -> String { let detail = format!("`{}`", leaf_segment(key)); - let resolved: Vec<&(String, Option, Option)> = entries + let resolved: Vec<(&String, &(String, String))> = entries .iter() - .filter(|(_, value, _)| value.is_some()) + .filter_map(|(locale, hit)| hit.as_ref().map(|hit| (locale, hit))) .collect(); match resolved.as_slice() { @@ -258,21 +265,13 @@ pub fn translation_card_locales( trailer: Some(TRANSLATION_NOT_FOUND_ANY_LOCALE_TRAILER), ..Default::default() }), - [(locale, value, source_link)] => { - translation_card(key, locale, value.as_deref(), source_link.as_deref()) - } + [(locale, (value, link))] => translation_card(key, locale, Some(value), Some(link)), _ => { let lines: Vec = resolved .iter() - .map(|(locale, value, source_link)| { - // Curly quotes delimit the value so it can't be mistaken - // for the key or a path — same rule as `translation_card`. - let quoted = value.as_deref().unwrap_or_default(); - match source_link { - Some(link) => format!("**{locale}** — “{quoted}” · {link}"), - None => format!("**{locale}** — “{quoted}”"), - } - }) + // Curly quotes delimit the value so it can't be mistaken for + // the key or a path — same rule as `translation_card`. + .map(|(locale, (value, link))| format!("**{locale}** — “{value}” · {link}")) .collect(); render(&HoverContent { detail: Some(&detail), diff --git a/laravel-lsp/src/hover/tests.rs b/laravel-lsp/src/hover/tests.rs index 3bd9af0b..f6fc0ff8 100644 --- a/laravel-lsp/src/hover/tests.rs +++ b/laravel-lsp/src/hover/tests.rs @@ -739,24 +739,32 @@ fn translation_card_locales_renders_one_line_per_locale_with_inline_links() { &[ ( "de".to_string(), - Some("Analyse fehlgeschlagen".to_string()), - Some("[lang/de/contract.php](file:///x)".to_string()), + Some(( + "Analyse fehlgeschlagen".to_string(), + "[lang/de/contract.php](file:///x)".to_string(), + )), ), ( "en".to_string(), - Some("Analysis failed".to_string()), - Some("[lang/en/contract.php](file:///y)".to_string()), + Some(( + "Analysis failed".to_string(), + "[lang/en/contract.php](file:///y)".to_string(), + )), + ), + ( + "fr".to_string(), + Some(( + "Analyse échouée".to_string(), + "[lang/fr/contract.php](file:///z)".to_string(), + )), ), - // A locale that resolved but whose source file couldn't be linked — - // it must still occupy exactly one line, not collapse or double. - ("fr".to_string(), Some("Analyse échouée".to_string()), None), ], ); assert!(card.starts_with("`failed_title`")); assert!(card.contains("**de** — “Analyse fehlgeschlagen” · [lang/de/contract.php](file:///x)")); assert!(card.contains("**en** — “Analysis failed” · [lang/en/contract.php](file:///y)")); - assert!(card.contains("**fr** — “Analyse échouée”")); + assert!(card.contains("**fr** — “Analyse échouée” · [lang/fr/contract.php](file:///z)")); // Three locales → three adjacent lines in one block, links inline. A // paragraph-delimited render would put a blank line between every row and @@ -776,11 +784,13 @@ fn translation_card_locales_collapses_when_only_one_locale_resolves() { let card = translation_card_locales( "messages.welcome", &[ - ("de".to_string(), None, None), + ("de".to_string(), None), ( "en".to_string(), - Some("Welcome".to_string()), - Some("[lang/en/messages.php](file:///y)".to_string()), + Some(( + "Welcome".to_string(), + "[lang/en/messages.php](file:///y)".to_string(), + )), ), ], ); @@ -805,10 +815,7 @@ fn translation_card_locales_collapses_when_only_one_locale_resolves() { fn translation_card_locales_uses_the_any_locale_trailer_when_none_resolve() { let card = translation_card_locales( "messages.welcome", - &[ - ("de".to_string(), None, None), - ("en".to_string(), None, None), - ], + &[("de".to_string(), None), ("en".to_string(), None)], ); assert_eq!( card, diff --git a/laravel-lsp/src/main.rs b/laravel-lsp/src/main.rs index bebf18f2..90446d32 100644 --- a/laravel-lsp/src/main.rs +++ b/laravel-lsp/src/main.rs @@ -20083,19 +20083,21 @@ return [ let vendor_map = self.vendor_translation_namespaces_for(r).await; let map_ref = vendor_map.as_ref().map(|m| m.as_ref()); - let mut entries: Vec<(String, Option, Option)> = Vec::new(); + // A locale either defines the key — yielding both a value and a link to + // the file it was read from — or it does not. The two cannot occur + // apart, so they travel as one. + let mut entries: Vec<(String, Option<(String, String)>)> = Vec::new(); for locale in laravel_lsp::translation_lookup::available_locales(r, key, map_ref) { - let resolution = laravel_lsp::translation_lookup::resolve_translation_detailed( + let hit = match laravel_lsp::translation_lookup::resolve_translation_detailed( r, key, &locale, map_ref, - ); - let link = match &resolution { - Some(res) => Some(self.source_link(&res.source_file, None).await), + ) { + Some(res) => Some(( + hover::truncate_for_display(&Self::unquote_php_literal(&res.value), 200), + self.source_link(&res.source_file, None).await, + )), None => None, }; - let value = resolution.as_ref().map(|res| { - hover::truncate_for_display(&Self::unquote_php_literal(&res.value), 200) - }); - entries.push((locale, value, link)); + entries.push((locale, hit)); } hover::translation_card_locales(key, &entries) }