Skip to content
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

Normalize language codes, updated strings #14059

Merged
merged 19 commits into from
Dec 20, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Dec 19, 2023

I had not intended for this to be a Whole Ass Thing, but this is an issue that's been bugging me and causing a ton of friction in our release cycle for years. Basically, when Laravel 4 was released, they (and therefore we) were using 2 letter codes for language. As time went on, we never updated to use a "more" correct 5-6 character code instead.

This meant that every time I downloaded translations from Crowdin, I had to rename directories, etc - which made releasing take hours longer than it should have. With this change, we should be able to just download those translations and go, which I cannot tell you how much I'm excited about. (Truly.)

This also adds a migration to update the settings table and the users table to use the new codes, and updates the dropdown.

Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
@snipe snipe changed the title WIP Normalize language codes, updated strings Normalize language codes, updated strings Dec 19, 2023
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

I'm going to have to take a deeper look at this from my local, since it's too hard to view the changes from GitHub. So far it looks great, but the one thing I think I'd like to ask for is the down() migration in case we run into some kind of horrible problem that we need to back ourselves out of. I'll give a formal review once I've pulled it down though.

app/Helpers/Helper.php Show resolved Hide resolved
app/Http/Middleware/CheckLocale.php Show resolved Hide resolved
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

I was able to get a handle on the contents of this by doing:

git diff --name-status develop |grep -v resources/lang

Once I did that, I was able to make sure I at least had a handle on what was changing.

I am so glad we are FINALLY un-screwing-up our language files - that's amazing!!!

This all looks extremely well-thought-out and thorough. It's a nasty change, but a necessary one.

I think the one change I'd like to ask for is the down() migration as I mentioned previously. I feel like, especially with such a huge change, the ability to go 'back' to an old, known-good state should be critical, in case something goes terribly wrong. And with so much changing that is a distinct possibility. Regardless of that one small request, I think this looks great and is ready to go! Yay! I'm terrified!

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

😮‍💨 this was a heavy lift!

I have some questions about potential safe-guards.

.env.example Show resolved Hide resolved
app/Helpers/Helper.php Show resolved Hide resolved
app/Helpers/Helper.php Show resolved Hide resolved
uberbrady and others added 7 commits December 19, 2023 20:06
…ocalization_refactor

Added back-migration for Big Locale Refactoring
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
@snipe
Copy link
Owner Author

snipe commented Dec 19, 2023

I added a \Log::warning() in the middleware to let people know they need to update their env as well.
Screenshot 2023-12-19 at 8 34 26 PM

Signed-off-by: snipe <snipe@snipe.net>
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

Looks great! And wow, what a gorgeous back-migration we have - wow, that's some really brilliant code-writing, whoever did that! I think you addressed everything I was concerned with. I'm still a little worried about the rollout plan, but that's outside of the scope of this PR.

@snipe
Copy link
Owner Author

snipe commented Dec 19, 2023

judgey

@snipe
Copy link
Owner Author

snipe commented Dec 19, 2023

@uberbrady

I'm still a little worried about the rollout plan, but that's outside of the scope of this PR.

We default down to en-US, so I don't think there's too much to worry about? We can merge this to develop, see what happens, then update the env there.

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

A little scary to say since the diff is massive but this looks good to me.

@snipe snipe merged commit 4ee5ba4 into develop Dec 20, 2023
7 checks passed
@snipe snipe deleted the localizations/new_strings_pre-6.3.0 branch December 20, 2023 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants