Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the FilamentTranslatablePlugin getters to evaluate closure-based configuration values (locales and default locale) at read time, aligning them with the existing evaluate() usage for other plugin options.
Changes:
getLocales()now evaluates$this->localesinstead of returning the raw value.getDefaultLocale()now evaluates$this->defaultLocalebefore falling back toconfig('app.fallback_locale', 'en').- Return types for both getters were narrowed to concrete evaluated values.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function getLocales(): ?array | ||
| { | ||
| return $this->locales; | ||
| return $this->evaluate($this->locales); | ||
| } |
There was a problem hiding this comment.
getLocales() now evaluates $this->locales but returns array|null. If the configured closure returns an Illuminate\Support\Collection (e.g., Laravel pluck() as shown in the README), this will trigger a runtime TypeError because the method will return a Collection. Consider normalizing the evaluated value (convert Collection to array via all()) or widening the return type to include Collection.
| @@ -115,9 +115,9 @@ public function getFlagWidth(): string | |||
| /** | |||
| * @return array<string> | |||
There was a problem hiding this comment.
The @return array<string> PHPDoc for getLocales() no longer matches the signature/behavior (array|null). Either update the docblock to include null or ensure the method always returns an array (e.g., default to []) to keep the contract consistent.
| * @return array<string> | |
| * @return array<string>|null |
| public function getDefaultLocale(): string | ||
| { | ||
| return $this->defaultLocale ?? config('app.fallback_locale', 'en'); | ||
| return $this->evaluate($this->defaultLocale) ?? config('app.fallback_locale', 'en'); |
There was a problem hiding this comment.
getDefaultLocale() is declared to return string, but config('app.fallback_locale', 'en') can still be null if the config key exists and is explicitly set to null. Add a final fallback (e.g., null-coalesce/empty check) so this method always returns a string and can’t throw a TypeError in that scenario.
| return $this->evaluate($this->defaultLocale) ?? config('app.fallback_locale', 'en'); | |
| return $this->evaluate($this->defaultLocale) | |
| ?? config('app.fallback_locale') | |
| ?? 'en'; |
| @@ -127,8 +127,8 @@ public function defaultLocale(string | Closure $locale): static | |||
| return $this; | |||
| } | |||
|
|
|||
| public function getDefaultLocale(): string | Closure | |||
| public function getDefaultLocale(): string | |||
| { | |||
| return $this->defaultLocale ?? config('app.fallback_locale', 'en'); | |||
| return $this->evaluate($this->defaultLocale) ?? config('app.fallback_locale', 'en'); | |||
There was a problem hiding this comment.
Both getLocales() and getDefaultLocale() have had their return types narrowed (previously allowed returning Closure, now array|null / string). Since these are public methods on the plugin, this is a backwards-incompatible API change for any consumers calling these getters directly. If BC is required, consider keeping the previous return types (or introducing new “evaluated” getters) and deprecating the old behavior.
No description provided.