Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

This could move to settings as well..not really sure
See also here #1813762-27: Introduce unified interfaces, use dependency injection for interface translation

ParisLiakos’s picture

Title: Convert locale_custom_strings_* to config » Convert locale_custom_strings_* to settings

i will roll a patch for settings

ParisLiakos’s picture

Status: Active » Needs review
FileSize
6.64 KB
tstoeckler’s picture

Can we add an example code to default.settings.php, please? If I'm not mistaken, we have done this for all new settings so far, and I think it really helps site builders / developers.

tstoeckler’s picture

Status: Needs review » Needs work

Forgot to say, that it looks great already!!! Found one other little nit-pick:

+++ b/core/includes/bootstrap.inc
@@ -1539,7 +1539,7 @@ function t($string, array $args = array(), array $options = array()) {
+    $custom_strings[$options['langcode']] = settings()->get('locale_custom_strings_' . $options['langcode'], array());

This should be \Drupal::settings()

ParisLiakos’s picture

Status: Needs work » Postponed

#4: there is already one:)
#5: better wait for #1813762: Introduce unified interfaces, use dependency injection for interface translation, since it is rtbc and then inject settings to CustomStrings translator

tstoeckler’s picture

Re #6: There is the following code in default.settings.php:

/**
 * String overrides:
 *
 * To override specific strings on your site with or without enabling the Locale
 * module, add an entry to this list. This functionality allows you to change
 * a small number of your site's default English language interface strings.
 *
 * Remove the leading hash signs to enable.
 */
# $conf['locale_custom_strings_en'][''] = array(
# 'forum' => 'Discussion board',
# '@count min' => '@count minutes',
# );

The example code should be changed to something like:

# $settings['locale_custom_strings_en'][''] = array(
# 'forum' => 'Discussion board',
# '@count min' => '@count minutes',
# );

Also:
1. I think, but I'm not sure, the [''] key is bogus.
2. We should move the entire block up to where the rest of the $settings are.
3. I think the docs could be improved, in particular they should mention that the _en part is in fact dynamic and works for any (enabled) language.

ParisLiakos’s picture

Status: Postponed » Needs work

as per #6

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

ook, injected settings into CustomStrings object, move the settings part together with rest of them, and added documentation about the langcode part.

I agree that '' is bogus, but not sure what to do here

Status: Needs review » Needs work

The last submitted patch, drupal-locale_custom_strings_settings-1975490-9.patch, failed testing.

ParisLiakos’s picture

meh, the installer needs parameteters as well

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI

The last submitted patch, drupal-locale_custom_strings_settings-1975490-11.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Configuration system, +D8MI

The last submitted patch, drupal-locale_custom_strings_settings-1975490-11.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
543 bytes
0 bytes
ParisLiakos’s picture

oh sigh, empty patch...interdiff is correct

Status: Needs review » Needs work
Issue tags: -Configuration system, -D8MI

The last submitted patch, drupal-locale_custom_strings_settings-1975490-17.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI
tstoeckler’s picture

Looks very good to me, but I'm not into the locale system enough to RTBC this myself.

aspilicious’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system, +D8MI

The last submitted patch, drupal-locale_custom_strings_settings-1975490-17.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
10.23 KB

ok lets reroll this one

Status: Needs review » Needs work

The last submitted patch, drupal-locale_custom_strings_settings-1975490-23.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
543 bytes
10.44 KB

oh, forgot a line during reroll

dawehner’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/FormatDateTest.php
    index 0a94658..2e4b7cf 100644
    --- a/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php
    @@ -7,13 +7,12 @@
    +use Drupal\system\Tests\System\CustomStringsTestBase;
    ...
    -class MenuRouterTest extends WebTestBase {
    +class MenuRouterTest extends CustomStringsTestBase {
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/System/CustomStringsTestBase.php
    @@ -0,0 +1,59 @@
    +abstract class CustomStringsTestBase extends WebTestBase {
    

    This baseclass really feel wrong. Can't we not just add this to the WebTestBase?

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php
    @@ -7,13 +7,12 @@
    -use PDO;
    

    A little be out of scope, but fine.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/System/CustomStringsTestBase.php
    --- a/sites/default/default.settings.php
    +++ b/sites/default/default.settings.php
    
    +++ b/sites/default/default.settings.php
    +++ b/sites/default/default.settings.php
    @@ -463,6 +463,23 @@
    
    @@ -463,6 +463,23 @@
     # $settings['session_write_interval'] = 180;
     
     /**
    + * String overrides:
    + *
    + * To override specific strings on your site with or without enabling the Locale
    + * module, add an entry to this list. This functionality allows you to change
    + * a small number of your site's default English language interface strings.
    + *
    + * Remove the leading hash signs to enable.
    + *
    + * The "en" part of the variable name, is dynamic and can be any langcode of
    + * any enabled language. (eg locale_custom_strings_de for german).
    + */
    +# $settings['locale_custom_strings_en'][''] = array(
    +#   'forum'      => 'Discussion board',
    +#   '@count min' => '@count minutes',
    +# );
    +
    +/**
      * Base URL (optional).
      *
      * If Drupal is generating incorrect URLs on your site, which could
    @@ -589,20 +606,6 @@
    
    @@ -589,20 +606,6 @@
     # $conf['system.performance']['js']['gzip'] = FALSE;
     
     /**
    - * String overrides:
    - *
    - * To override specific strings on your site with or without enabling the Locale
    - * module, add an entry to this list. This functionality allows you to change
    - * a small number of your site's default English language interface strings.
    - *
    - * Remove the leading hash signs to enable.
    - */
    -# $conf['locale_custom_strings_en'][''] = array(
    -#   'forum'      => 'Discussion board',
    -#   '@count min' => '@count minutes',
    -# );
    -
    -/**
    

    Is there a reason why this was moved in the file?

catch’s picture

Priority: Normal » Critical
dawehner’s picture

Status: Needs review » Needs work

.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
9.48 KB

Is there a reason why this was moved in the file?

So it is grouped with the rest of the settings section. also see #7

ParisLiakos’s picture

FileSize
5.24 KB

meh, the interdiff ;)

Status: Needs review » Needs work

The last submitted patch, custom_strings_settings-1975490-29.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
10.31 KB
dawehner’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -918,6 +925,41 @@ protected function writeSettings($settings) {
+   * Adds custom translations.

I think we should describe that the custom translations are added to the settings object.

Otherwise this looks really good.

ParisLiakos’s picture

probably something like that

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-ui
catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks great. Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

Gábor Hojtsy’s picture

Added a quick change notice at https://drupal.org/node/2109883

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