Converting the language_count variable to the state system.

Files: 
CommentFileSizeAuthor
#35 drupal-1856976-35.patch11.29 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,164 pass(es).
[ View ]
#28 interdiff.txt1.71 KBandypost
#28 language-state-1856976-28.patch11.41 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language-state-1856976-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 interdiff.txt6.7 KBandypost
#24 language-state-1856976-24.patch10.73 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 60,527 pass(es).
[ View ]
#22 interdiff.txt2.33 KBandypost
#22 language-state-1856976-22.patch8.8 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,801 pass(es), 289 fail(s), and 14 exception(s).
[ View ]
#19 language-state-1856976-19.txt2.02 KBandypost
#21 interdiff.txt756 bytesandypost
#21 language-state-1856976-21.patch7.33 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,937 pass(es), 289 fail(s), and 14 exception(s).
[ View ]
#18 interdiff.txt7.12 KBandypost
#18 language-state-1856976-18.patch7.28 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,676 pass(es), 1,585 fail(s), and 561 exception(s).
[ View ]
#14 1856976-language_count_to_state-drupal8-14.patch5.5 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#11 drupal_8-language_count_var_into_state-1856976-4.patch5.6 KBSoul88
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in drupal_8-language_count_var_into_state-1856976-4.patch.
[ View ]
#8 drupal_8-language_count_var_into_state-1856976-3.patch5.91 KBSoul88
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in drupal_8-language_count_var_into_state-1856976-3.patch.
[ View ]
#5 install_fails.txt7.63 KBaspilicious
#1 1856976-language_count_to_state-drupal8-1.patch5.98 KBACF
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

ACF’s picture

Status:Active» Needs review
StatusFileSize
new5.98 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

add patch

Status:Needs review» Needs work
Issue tags:-Configuration system, -Config novice, -State system

The last submitted patch, 1856976-language_count_to_state-drupal8-1.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system, +Config novice, +State system

The last submitted patch, 1856976-language_count_to_state-drupal8-1.patch, failed testing.

aspilicious’s picture

StatusFileSize
new7.63 KB

This is failing because we need a memory keyvalue store in early bootstrap. I added that and than the installer proceeds to the database settings page. After filling in the correct data it blow up again with the message: "keyvalue.memory" is not available.

Thats because we had to change the default to keyvalue.memory to make the installer work but when we enter the database settings a new container will be build that contains the database keyvalue store (*I think*) so we have to clear the keyvalue.memory default so that it takes the databse one.

I tried that but it still tells me "keyvalue.memory is not defined". If someone else can figure out where we have to add an aditional check or where we have to reset/rebuild something please let me know (or fix the patch yourself :p).

heyrocker’s picture

They had similar problems in #1804380: convert color variables to config/state, see the patch there.

Soul88’s picture

Am working on this on the CodeSprint: https://drupal.org/node/2009672

Soul88’s picture

StatusFileSize
new5.91 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in drupal_8-language_count_var_into_state-1856976-3.patch.
[ View ]

try to reroll

Soul88’s picture

Status:Needs work» Needs review
Issue tags:+CodeSprintUA

Adding tag and changing status

andypost’s picture

Status:Needs review» Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2514,7 +2514,9 @@ function language_multilingual() {
+  $language_count = state()->get('language.count');
+  $language_count = !empty($language_count)? $language_count: 1;

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -135,7 +135,9 @@ public function reset($type = NULL) {
+    $language_count = state()->get('language.count');
+    $language_count = !empty($language_count)? $language_count: 1;

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageListTest.phpundefined
@@ -119,7 +119,9 @@ function testLanguageList() {
+    $language_count = state()->get('language.count');
+    $language_count = !empty($language_count)? $language_count: 1;

@@ -133,7 +135,9 @@ function testLanguageList() {
+    $language_count = state()->get('language.count');
+    $language_count = !empty($language_count)? $language_count: 1;

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.phpundefined
@@ -115,7 +115,8 @@ function testUninstallProcess() {
+    $language_count = state()->get('language.count');
+    $language_count = !empty($language_count)? $language_count: 1;

replace with $language_count = state()->get('language.count') ?: 1;

Soul88’s picture

StatusFileSize
new5.6 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in drupal_8-language_count_var_into_state-1856976-4.patch.
[ View ]

Fixed those changes.

As for the KeyValue storage problems, I suppose that they could've already been solved by the patch from https://drupal.org/node/1804380 that was commited in feb.2013. If not, I'll add some code to "core/includes/install.core.inc"

andyceo’s picture

Status:Needs work» Needs review
Soul88’s picture

Status:Needs review» Needs work

Unforunately it still needs work. I've ran into coke and egg issue. I can't make a change to the LanguageManager.php file.

/**
* Whether already in the process of language initialization.
*
* @todo This is only needed due to the circular dependency between language
* and config. See http://drupal.org/node/1862202 for the plan to fix this.
*
* @var bool
*/

But even besides that some tests fail for now.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new5.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-rolling #11

Status:Needs review» Needs work
Issue tags:-Configuration system, -Config novice, -State system, -CodeSprintUA

The last submitted patch, 1856976-language_count_to_state-drupal8-14.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system, +Config novice, +State system, +CodeSprintUA

The last submitted patch, 1856976-language_count_to_state-drupal8-14.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new7.28 KB
FAILED: [[SimpleTest]]: [MySQL] 53,676 pass(es), 1,585 fail(s), and 561 exception(s).
[ View ]
new7.12 KB

Re-roll after #1947720: Use Drupal::state() to replace state()
Fixed install, @todo language manager requires 'state' service injection

andypost’s picture

StatusFileSize
new2.02 KB

This kind of injection does not work.

EDIT we can use optional injection

Status:Needs review» Needs work

The last submitted patch, language-state-1856976-18.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new7.33 KB
FAILED: [[SimpleTest]]: [MySQL] 57,937 pass(es), 289 fail(s), and 14 exception(s).
[ View ]
new756 bytes

The container is not accessible somehow

andypost’s picture

StatusFileSize
new8.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,801 pass(es), 289 fail(s), and 14 exception(s).
[ View ]
new2.33 KB

Another approach

Status:Needs review» Needs work

The last submitted patch, language-state-1856976-22.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new10.73 KB
PASSED: [[SimpleTest]]: [MySQL] 60,527 pass(es).
[ View ]
new6.7 KB

Fixed a bit of cache troubles.
The state service is optional

Status:Needs review» Needs work
Issue tags:-Configuration system, -Config novice, -State system, -CodeSprintUA

The last submitted patch, language-state-1856976-24.patch, failed testing.

Tor Arne Thune’s picture

Status:Needs work» Needs review
Issue tags:+Configuration system, +Config novice, +State system, +CodeSprintUA

#24: language-state-1856976-24.patch queued for re-testing.

dawehner’s picture

+++ b/core/core.services.ymlundefined
@@ -185,6 +185,7 @@ services:
+    arguments: ['@?state']

I try to figure out why we couldn't use a memory based key value store during validation.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -46,6 +54,16 @@ class LanguageManager {
+   *
+   * @param KeyValueStoreInterface $state

Should be the full namespace.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -46,6 +54,16 @@ class LanguageManager {
+   *   The state keyvalue store.

Let's also document that it is optional.

andypost’s picture

StatusFileSize
new11.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language-state-1856976-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.71 KB

No reason for conversion to add new service for install time.
Memory based storage would be added in #1862202: Objectify the language system because it changes a way like language manager registred.
So for now just simple check for that

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -136,7 +154,11 @@ public function reset($type = NULL) {
   public function isMultilingual() {
-    return variable_get('language_count', 1) > 1;
+    if (!isset($this->state)) {
+      // No state service in install time.
+      return FALSE;
+    }
+    return ($this->state->get('language_count') ?: 1) > 1;

Here's a check for install time and absence of the state

dawehner’s picture

Well, isn't it a reason that we hack around stuff which is actually not needed if we do it properly,
especially because we already know how to do it properly?

andypost’s picture

@dawehner Because this task is independent from #1862202: Objectify the language system so just trying to not duplicate a work

penyaskito’s picture

Issue tags:+D8MI
jibran’s picture

Status:Needs review» Needs work
Issue tags:+Configuration system, +D8MI, +Config novice, +State system, +CodeSprintUA

The last submitted patch, language-state-1856976-28.patch, failed testing.

dawehner’s picture

Issue tags:+Needs reroll
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new11.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,164 pass(es).
[ View ]

There we go.

aspilicious’s picture

$javascript_parsed_count = count($this->container->get('state')->get('system.javascript_parsed') ?: array());

slightly unrelated but I get why you're doing this in there as the service is injected now. Looks good to me otherwise...

aspilicious’s picture

Status:Needs review» Reviewed & tested by the community

Good to go!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 566ebfd and pushed to 8.x. Thanks!

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