Problem/Motivation

      '#markup' => SafeMarkup::format('<p>Translations will be downloaded from the <a href="http://localize.drupal.org">Drupal Translation website</a>.
      If you do not want this, select <a href="!english">English</a>.</p>', array(
          '!english' => install_full_redirect_url(array('parameters' => array('langcode' => 'en'))),

History

Was changed

  1. From a plain string to SafeMarkup::format() in this issue. #2185583: Allow to select "English" language via mouse click in installer language selection screen
  2. Lost translation in this issue #1848490-45: Import translations automatically during installation

Proposed resolution

Remove translation and use of SafeMarkup::format(). No translation for the string will ever be used, because it is displayed before translations are downloaded.

Remaining tasks

User interface changes

API changes

Data model changes

Follow-up to #2564353: Remove !placeholder usage from SafeMarkup::format()

CommentFileSizeAuthor
#20 2565981.14.patch3.09 KBGábor Hojtsy
#14 2565981.14.patch3.09 KBalexpott
#14 12-14-interdiff.txt648 bytesalexpott
#12 2565981.12.patch3.1 KBalexpott
#9 translate_instead_of-2565981-9.patch1.49 KBjoelpittet
#9 interdiff.txt1.33 KBjoelpittet
#6 interdiff.txt1.3 KBjoelpittet
#6 translate_instead_of-2565981-6.patch1.39 KBjoelpittet
#3 translate_instead_of-2565981-2.patch1.14 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
translate_instead_of-2565981-2.patch1.14 KB
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes

Added history context to the issue summary.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Component: base system » install system
Issue summary: View changes

Aha! I found the issue that this was lost it's translation in! #1848490-45: Import translations automatically during installation

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
translate_instead_of-2565981-6.patch1.39 KB
interdiff.txt1.3 KB

Suggestion from @alexpott, moved the <p> outside the translated string into prefix/suffix.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

+1 to this

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Well you can't really say the translation was "lost" as these strings will never actually be translated in the actual sense of the word as we don't have any translations yet at this point. That doesn't make this fix any less correct, though.

The weird thing is that with this the string will end up on localize.drupal.org so people will translate it, but they will never actually see it translated, so maybe that's a usability problem then? On the other hand, if people ship distributions with pre-set translations then they would want this to be translated? Maybe we could even ship "Pre-translation-download-strings" in a bunch of languages with D8 core in the future? Hmm...
Maybe we should run this by @Gábor Hojtsy?

Since we're already touching this, IMHO we should fix the "multiline-ness" of the translated string. That's very non-standard in core, and results in weird markup (and weird translator's experience).

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Assigned: Unassigned » Gábor Hojtsy
FileSize
interdiff.txt1.33 KB
translate_instead_of-2565981-9.patch1.49 KB

@tstoeckler thanks for the review and notes. I've removed that weirdness in multi-lineness. @Gábor Hojtsy has been pinged by @alexpott and I'll assign this to him.

"lost" translates to removed by accident;) Or what I meant anyway.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Yay, thanks a lot. Looks good to me, but will let @Gábor Hojtsy RTBC in case there are no reservations.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Agreed with

The weird thing is that with this the string will end up on localize.drupal.org so people will translate it, but they will never actually see it translated

As for

On the other hand, if people ship distributions with pre-set translations then they would want this to be translated? Maybe we could even ship "Pre-translation-download-strings" in a bunch of languages with D8 core in the future? Hmm...

I don't think the .po file would be read at this time even if available. Also it would need to be in a sites/X/files/translations directory. Not sure how distros could ship with that (due to its dynamic site specific location). Also even distro PHP code does not have a chance to run at the time of this form (to download a .po file to that dynamic location). So I don't know how would that work for this form. As for doing in the future, we can add t() when/if we get there. Now its a lie.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2565981.12.patch3.1 KB

So let's go the other way and remove t() completely and add a comment - we can also improve security by removing the SafeMarkup::format() and letting XSS filter do it's job.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Title: Translate instead of SafeMarkup::format() in the installer. » Remove $this->t() and SafeMarkup::format() in SelectLanguageForm
Issue summary: View changes
Status: Needs review » Needs work

Retitled. Looks good except:

+++ b/core/lib/Drupal/Core/Installer/Form/SelectLanguageForm.php
@@ -16,6 +15,9 @@
+ * Note that none of the hardcoded text provided by this form is not translated

none is not translated

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
12-14-interdiff.txt648 bytes
2565981.14.patch3.09 KB

Thanks @Gábor Hojtsy

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +D8MI, +language-ui, +sprint

Yay, looks good.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Couldn't I put this in a settings.php like config in a distribution?

I've included hard coded translations like this in d7 in my settings.php.

Also there are other translations in this file that also should be removed if this solutions is correct?
Edit Alex removed them all(on phone, didn't see code)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@joelpittet if you've got a settings.php why are you in the installer?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Touché, glad I'm reviewing code when I wake up next to my phone...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2565981.14.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs work » Needs review
FileSize
2565981.14.patch3.09 KB

Not sure why this broke all the installer forms. Reuploading as a retest, so we have the results up for posterity.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

ReRTBCing this.

I feel we should keep the translation and hopefully make the strings translatable on the installer in the future. But I don't want to hold up the fix here, so no need to block.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

So, currently, the summary says that the string should be translated. But the comment in this patch and the discussion indicate the opposite, because the translations aren't available yet when the form is displayed. So I've updated the summary to clarify that.

Committed and pushed to 8.0.x. Thanks!

  • xjm committed ed3bc9b on 8.0.x 
    Issue #2565981 by joelpittet, alexpott, Gábor Hojtsy, tstoeckler: Remove...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.