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

[UX] Installer: Allow using .po translation files from b.org without having to rename them. #3674

Closed
olafgrabienski opened this issue Apr 16, 2019 · 24 comments · Fixed by backdrop/backdrop#2583

Comments

@olafgrabienski
Copy link

olafgrabienski commented Apr 16, 2019

The translation files available from https://localize.backdropcms.org/translate/downloads are in the format backdropcms-VERSION.LANGCODE.po. As in #2345 for files from d.org, we should be accounting for these filenames when scanning for language files during installation, instead of expecting people to rename files to match the pattern install.LANGCODE.po.

Steps To Reproduce

  1. Download a language file from the translation server.
  2. Move the downloaded .po file, e.g. backdropcms-1.12.2.es.po, inside the directory files/translations of your Backdrop directory.
  3. Install Backdrop as per usual.
  4. During the first installation step, you will be prompted to choose a language.

Actual behavior
You can't choose another language but English.

Expected behavior
You can choose your language, e.g. Spanish.

Additional information
The issue had already been fixed for Drupal files before we started to work with our own language server.


PR by @olafgrabienski: backdrop/backdrop#2583

@olafgrabienski
Copy link
Author

@klonos I recall you had made the PR for the respective d.org files issue. Are you up to make the PR for b.org files as well?

@olafgrabienski
Copy link
Author

I've added the label needs - documentation because in https://backdropcms.org/installing-in-other-languages we're currently mentioning that people have to rename a language file downloaded from b.org:

Rename the file to install.(languagecode).po.
Example: Rename backdropcms-1.12.2.es.po to install.es.po.

After fixing this issue, we can remove that step from the instructions.

@herbdool
Copy link

herbdool commented Apr 16, 2019

@olafgrabienski it's a pretty simple PR if you want to take a stab at it:

install|drupal-\d+\.\d+ should be install|drupal-\d+\.\d+|backdropcms-\d+\.\d+\.\d+.

See https://github.com/backdrop/backdrop/pull/1649/files

@klonos
Copy link
Member

klonos commented Apr 17, 2019

@olafgrabienski
Copy link
Author

install|drupal-\d+\.\d+ should be install|drupal-\d+\.\d+|backdropcms-\d+\.\d+\.\d+.

@herbdool Thanks, very helpful! I'm not familiar with such patterns. Will try to take a stab at it.

(Btw, what does the \d+ part mean: any number?)

@herbdool
Copy link

Correct, it means a number one or more times. There are handy websites like this https://regex101.com/ where you can play around with regular expressions to see what they mean and if they'll allow a sample file name.

@klonos
Copy link
Member

klonos commented Apr 17, 2019

@olafgrabienski if you type d, it means d. If you type \d it means "1 digit". If you type \d+ it means "one or more digits". \d? means "0 or more digits". | means "or". . means "any 1 character". \. means "dot".

See https://medium.com/factory-mind/regex-tutorial-a-simple-cheatsheet-by-examples-649dc1c3f285

@klonos
Copy link
Member

klonos commented Apr 17, 2019

...or what @herbdool said 😄

@klonos
Copy link
Member

klonos commented Apr 17, 2019

...or could use (install|drupal|backdropcms)(-[\d\.]+)?, which produces the same. It means install or drupal or backdropcms, followed by zero or more occurrences of: a dash, and then any number of digits and dots.

@olafgrabienski
Copy link
Author

...or could use (install|drupal|backdropcms)(-[\d\.]+)?, which produces the same ...

Hm, not exactly ... At least according to https://regex101.com, the expressions treat the dot before the language code differently:

screen-regex-tester-a

screen-regex-tester-b

@olafgrabienski
Copy link
Author

I suggest milestone 1.12.7, as #3560 which is related.

@olafgrabienski
Copy link
Author

Tested the PR locally: put backdropcms-1.12.2.de.po into files/translations, ran the installer and was able to choose "Deutsch" in the Choose language dialog.

To reproduce, the language files are available on https://localize.backdropcms.org/translate/downloads.

@herbdool
Copy link

Looks great! Thanks @olafgrabienski!

@olafgrabienski
Copy link
Author

Thanks for the review! What do you think about the milestone?

@herbdool herbdool added this to the 1.12.7 milestone Apr 18, 2019
@herbdool
Copy link

I don't know if I can add a milestone but I think it's safe to make it 1.12.7.

@olafgrabienski
Copy link
Author

Reminder @olafgrabienski :

In https://backdropcms.org/installing-in-other-languages we're currently mentioning that people have to rename a language file downloaded from b.org.

When this PR becomes part of a new Backdrop release, we should remove that step from the instructions.

@jenlampton
Copy link
Member

I don't know if I can add a milestone

@herbdool yes, if you agree you can add it :) nicely done!

@quicksketch
Copy link
Member

Looks good. I see that the .po files you download from https://localize.backdropcms.org/ match this file name structure, but it raised my eyebrows that the name of the downloaded file was "backdropcms" instead of just "backdrop". Inside our own codebase, we always use the shortened version without "cms" for function names, settings, etc. If we can change the string indicator on the localize site, we should adjust this code to accept just "backdrop" as well.

Perhaps we could go ahead and make that change (allowing "backdrop" with no "cms") so we can later adjust the localization server whenever we want?

@quicksketch
Copy link
Member

@klonos
Copy link
Member

klonos commented Apr 21, 2019

Suggestion made at https://github.com/backdrop/backdrop/pull/2583/files#r277144890

LGTM 👍

@olafgrabienski
Copy link
Author

I've applied @quicksketch's suggestion. Looks good to me, but one test in backdrop/backdrop#2583 failed:

zenci/test/php70/pr-2583 — 1 of 43594 tests failed

@herbdool
Copy link

I think that's a random error. Looks good!

@quicksketch
Copy link
Member

Thanks @olafgrabienski, @herbdool, and @klonos! Merged backdrop/backdrop#2583 into 1.x and 1.12.x.

@olafgrabienski
Copy link
Author

In https://backdropcms.org/installing-in-other-languages we're currently mentioning that people have to rename a language file downloaded from b.org.

When this PR becomes part of a new Backdrop release, we should remove that step from the instructions.

  • Tested again with a fresh Backdrop 1.13.0 and a current language file from the translation server.
  • Changed the documentation page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants