Skip to content

Conversation

JulianGlueck
Copy link
Contributor

@JulianGlueck JulianGlueck commented Aug 16, 2025

This PR allows developers to obtain localisation information of the application and the system via the App facade.

It adds three new methods to the facade that are proxies for the Electron methods app.getLocale(), app.getLocaleCountryCode(), and app.getSystemLocale() (see details on the Electron documentation). This allows developers to improve the localisation of their application, e.g. by automatically setting or suggesting a language that matches the system language when a NativePHP application is first launched.

Electron PR: NativePHP/electron#247
Docs PR: NativePHP/nativephp.com#198

If there's anything you'd like me to tweak or improve in this PR, please let me know. 😊

@gwleuverink
Copy link
Contributor

Great! Thanks for your second contribution in row! You're on a roll 👀

This is a great addition 👍🏻

One thing, which is not blocking, we might consider; Having this feature under the NativePHP App facade is symmetrical with the Electron API, as a bonus the getLocale method is symmetrical with Laravel's own App facade. So I get this is the most obvious choice to put this feature.

It does pose a naming overlap when you need to set Laravel's locale to be the same as the Electron app or system locale. See this simplified example:

// -----------------------------------------------------------
// Since we're using two App facades we need to alias one

use Illuminate\Support\Facades\App;
use Native\Laravel\Facades\App as NativeApp;

App::setLocale(NativeApp::getLocale());

// -----------------------------------------------------------
// Alternatively we might avoid the naming overlap by using the app helper,
// But this might make it a little less obvious that the `getLocale` being invoked
// belongs to NativePHP, since to the trained eye this will be recognized as Laravel's `App::getLocale()`

use Native\Laravel\Facades\App;
app()->setLocale(App::getLocale());

We could consider moving it to the System facade. This breaks symmetry with the Electron API but would make it more obvious what's going on in a glance.

use Illuminate\Support\Facades\App;
use Native\Laravel\Facades\System;

App::setLocale(System::getLocale());

I know this is nitpicky, but I'm curious as to your thoughts @JulianGlueck @NativePHP/contributors

@gwleuverink gwleuverink requested a review from a team August 16, 2025 23:44
@JulianGlueck
Copy link
Contributor Author

Thanks @gwleuverink for supporting that. 🙌

You have a valid point here. Frankly, I'm quite undecided on that. Personally, I think I would lean towards letting the method in Native's App facade as to me that makes sense from a semantic point of view. I was thinking about renaming Native's methods to address the ambiguity you described but my mind can't come up with something sensible other than App::getAppLocale() which one might not exactly consider a very nice or creative naming. 😅

On a side note: From a semantic perspective, I think it might be worth considering to map Electron's app.getSystemLocale() to NativePHP's System::getLocale() (instead of App). I think that would be the easiest to understand without requiring to refer to the documentation. However, I am unsure how strictly you guys want to adhere to Electron's schema.

Copy link
Member

@SRWieZ SRWieZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind the colliding names :)

@gwleuverink gwleuverink merged commit e4ec3de into NativePHP:main Aug 18, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants