-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Laravel] Defer Translator + Simplify Provider #1777
Conversation
Also I should probably note that I have not extensible tested it, but 'it works on my device' ;) |
The locale is not a constant, it can change using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Unit tests are broken: https://travis-ci.org/briannesbitt/Carbon/jobs/555104782#L508
- If imports are no longer used, they should be removed: https://github.styleci.io/analyses/qo3jnP?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
- I feel uncomfortable to add 1 method + 1 property to get a couple of milliseconds boost in Laravel, Carbon is a standalone library and should not have Laravel-specific code outside the
Laravel
directory - I will also run benchmarks and will only merge if it's signifiant (50% of the Carbon boot time)
Yes I understand the sync, but if you don't have the event dispatcher (for some reason, but mostly you have it anyways?) You can still get the current locale. Currently you are bailing out before setting it. And the translator doesn't have to be loaded just to get the Locale. |
You are right about the benchmark/micro optimization. I think it would be required to test further to see what real life would change. It's a bit hard to test with opcache and all. It seems to switch a bit all the time, but it seems like it's a bit faster for non-localized requests and the same for localized requests. But need to find a good benchmark still.
|
If you want I can create a PR for just the Laravel changes (eg. use the app locale directly instead of the translator) |
I extracted it: #1779 Tests pass and it still work in my Laravel app. Next step will be to make getLocale and setLocale to not trigger the lazy-load of the translator. So I close this, but feel free to comment the other PR and propose the optimization you put in |
Thanks. The reason I created a new method was that I don't know the return value yet. |
As suggested in ##1776 , maybe we can wait to load the actual Translator until it's used?
I'm also not sure why we need the Laravel Translator or the Event to get the current Locale. The getLocale() is available from the App container already.