locale_storage() has been replaced with \Drupal::service('locale.storage');

https://drupal.org/node/2016569, change notice for locale_storage().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
6.72 KB

Initial patch...

YesCT’s picture

So you're injecting it.

Looks like you got all of the locale_storage()'s.

---

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationUITest.phpundefined
@@ -56,7 +64,7 @@ class ConfigTranslationUITest extends WebTestBase {
+    $this->localeStorage = $this->container->get('locale.storage');

I get the rest of this, except for this bit in the tests. Is it like this because it's a test?

vijaycs85’s picture

sorry don't get your question here.. but there can be 2 questions out of this line :)

1. why aren't we using \Drupal::service('locale.storage) - Yes in test cases we use it's own container.
2. why assigning in ->localStorage, we can call $this->container->get('locale.storage'); all the place where we need it. but thought would be nice to have as property.

YesCT’s picture

OK.

I had started to take out locale_storage() and inject it while looking at cleaning up the manage form issue, but thought it should be it's own issue. Then I saw this issue already. Most of this is the same as what I had started, but probably better.

I talked with @vijaycs85 in irc too, to clarify some things about this patch.
I also see similar patterns used else where.

I'm going to run this and #2020347: Temporary fix for config_translation_enter_context until language context becomes an option, and if the tests pass, I think they are ok to commit, so that we can unbreak the config translation head. Then we can move on and try and improve things from there.

[update] Not all tests are passing, so I'll wait for that or word from Gabor.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

This looks like a good code change as-is. Some minor notes:

+++ b/lib/Drupal/config_translation/Controller/ConfigTranslationController.phpundefined
@@ -163,7 +163,8 @@ class ConfigTranslationController implements ControllerInterface {
+    $locale_storage = \Drupal::service('locale.storage');
+    return drupal_get_form(new ConfigTranslationManageForm($locale_storage), $group, $language, $base_config);

+++ b/lib/Drupal/config_translation/Form/ConfigTranslationManageForm.phpundefined
@@ -46,6 +47,22 @@ class ConfigTranslationManageForm implements FormInterface {
+   * Storage object.
...
+   * Creates manage form object with storage.

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationUITest.phpundefined
@@ -30,6 +31,13 @@ class ConfigTranslationUITest extends WebTestBase {
+   * Storage object.

Are we supposed to invoke this as \Drupal...? Not Drupal:: (without backslash)?

Would be good to qualify what storage are these? :) Eg. "String translation storage object" for the variable docs and argument.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
575 bytes
6.74 KB

As explained by @YesCT on IRC, we need to have \Drupal:: in class and Drupal:: in procedural code. so updating just comment from #5

Gábor Hojtsy’s picture

Status: Needs review » Needs work

String translation storage object only changed at one place instead of 3.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.78 KB
1.32 KB

Fixing all 3 instances.

Gábor Hojtsy’s picture

Status: Needs review » Fixed

Thanks, committed this one!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

adding link to change notice.