Problem/Motivation

Globals are bad to use as it means there's no scope and restricts us from being able to instantial two Drupal instances at the same time. The $language_content global and LANGUAGE_TYPE_CONTENT constant is referenced in many places and we should clean that up.

Proposed resolution

Following #1497230: Use Dependency Injection to handle object definitions , this issue is to remove the $language_content global, and LANGUAGE_TYPE_CONTENT with a "language.content" service.

Remaining tasks

User interface changes

None.

API changes

<?php
// Before
global $language_content;
// After
$language_content = drupal_container()->get(LANGUAGE_TYPE_CONTENT);
?>
Files: 
CommentFileSizeAuthor
#43 1512308_0.patch2.93 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 36,813 pass(es).
[ View ]
#40 drupal-fix-lang-injection-1512310-0.patch563 bytesdisasm
PASSED: [[SimpleTest]]: [MySQL] 36,816 pass(es).
[ View ]
#32 1512308.patch2.93 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 36,850 pass(es).
[ View ]
#29 langague_type_content_06.patch26.52 KBAugust1914
FAILED: [[SimpleTest]]: [MySQL] 36,261 pass(es), 330 fail(s), and 451 exception(s).
[ View ]
#29 language_type_content_03_06_interdiff.txt963 bytesAugust1914
#27 language_type_content_05.patch26.53 KBAugust1914
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#27 language_type_content_03_05_interdiff.txt603 bytesAugust1914
#25 langauge_type_content_04.patch26.46 KBAugust1914
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#25 language_type_content_03_04_interdiff.txt915 bytesAugust1914
#23 language_type_content_03.patch26.54 KBAugust1914
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#18 langague_type_content_02.patch26.44 KBAugust1914
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch langague_type_content_02.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 language_type_content.patch17.16 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language_type_content.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 1512308.patch12.24 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 35,452 pass(es).
[ View ]
#5 1512308_5_di_language_url.patch1.67 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 35,428 pass(es).
[ View ]

Comments

Issue tags:+D8MI, +language-base

Status:Postponed» Active

Now that the dependency injection patch is in we can actively work on this.

StatusFileSize
new1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 35,428 pass(es).
[ View ]

In this patch I was able to find a few remaining uses of language_content as a global variable. This patch finds and fixes them using a variant of what is described above.

Status:Active» Needs review

Did you check how often $language_content is used in those functions? Ideally, if possible, we should move the DI code to the place where the variable is used -- if there is only one usage throughout the function. (if there are multiple uses, keep it at the top)

StatusFileSize
new12.24 KB
PASSED: [[SimpleTest]]: [MySQL] 35,452 pass(es).
[ View ]

Let's get rid of the constant while we're at it. The constant itself might not be available if the code is called early enough.

Did you check how often $language_content is used in those functions? Ideally, if possible, we should move the DI code to the place where the variable is used -- if there is only one usage throughout the function. (if there are multiple uses, keep it at the top)

Fixed.

Status:Needs review» Needs work
Issue tags:-D8MI, -language-base, -Dependency Injection (DI)

The last submitted patch, 1512308.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+D8MI, +language-base, +Dependency Injection (DI)

#8: 1512308.patch queued for re-testing.

Clean and excellent thanks Rob, recommend RTBC.

Why, instead of removing the constant which has a reason to exist, don't we move it to the language object?

On second thought, I recommend the patch in #5. The conversion of the use of constants to some other system needs more discussion and can be followed up later. That issue shouldn't hold up this patch.

There's a lot of discussion on how to handle the loading of this default config for Language and we're trying to put together a better proposal here: #1550866: Remove obsolete drupal_language_initialize(). Further work to improve the handling of the language system should go there.

This issue is simply for the intermediate step of using the di for things we used to use globals for. Reviewers please review patch #5.

StatusFileSize
new17.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language_type_content.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I like the idea of moving the constant into the Language class. The less stuff in bootstrap.inc, the better.

Status:Needs review» Needs work
Issue tags:+D8MI, +language-base, +Dependency Injection (DI)

The last submitted patch, language_type_content.patch, failed testing.

Assigned:Unassigned» August1914

StatusFileSize
new26.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch langague_type_content_02.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Resolved conflicts with 8.x, this patch applies cleanly at 467a825, patch from comment 14 applies cleanly at 252e84ec590e15132cf2bdb635b28d1a229ba167

Assigned:August1914» Unassigned

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, langague_type_content_02.patch, failed testing.

Assigned:Unassigned» August1914

Assigned:August1914» Unassigned
Status:Needs work» Needs review
StatusFileSize
new26.54 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Rerolled from #14, this patch applies cleanly at 5b52cea

Status:Needs review» Needs work

The last submitted patch, language_type_content_03.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new915 bytes
new26.46 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Changed references to (7.x) DrupalWebTestCase to (8.x) WebTestCase.
Reroll from #14, with changes in attached interdiff.
This patch applies cleanly at 5b52cea.

Status:Needs review» Needs work

The last submitted patch, langauge_type_content_04.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new603 bytes
new26.53 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Added simpletest includes.
Changed references to (7.x) DrupalWebTestCase to (8.x) WebTestCase.
Reroll from #14, with changes in attached interdiff.
This patch applies cleanly at 5b52cea.

Status:Needs review» Needs work

The last submitted patch, language_type_content_05.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new963 bytes
new26.52 KB
FAILED: [[SimpleTest]]: [MySQL] 36,261 pass(es), 330 fail(s), and 451 exception(s).
[ View ]

patch 05 incorrectly stated "WebTestCase" instead of "WebTestBase". Same as above, rerolling from #14

Status:Needs review» Needs work

The last submitted patch, langague_type_content_06.patch, failed testing.

OK, the reroll is complete with langague_type_content_06.patch; the patch applies and the testcases execute. The failures now are in various of the testcases.

Status:Needs work» Needs review
StatusFileSize
new2.93 KB
PASSED: [[SimpleTest]]: [MySQL] 36,850 pass(es).
[ View ]

Let's scale back the scope of this issue a bit. This is to just replace $language_content with calling drupal_container().

Moving the retrieval of it around is over at #1515196: Standardize retrieval of "global" variables to be at beginning of function.
Moving the constants around is over at #1620010: Move LANGUAGE constants to the Language class.

Status:Needs review» Reviewed & tested by the community
Issue tags:+sprint

Looks simple and right.

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -24,5 +24,8 @@ class ContainerBuilder extends BaseContainerBuilder {
+
+    // Register the default language content.
+    $this->register(LANGUAGE_TYPE_CONTENT, 'Drupal\\Core\\Language\\Language');

I ain't sure this is actually needed. I don't think content language needs to be available before language initialization.

It's okay if it's there, calling $container->register() doesn't actually instantiate the object. It's just the initial definition. In a follow up, we'd hopefully replace all three language objects with one language system object, allowing us to replace the language bootstrap entirely.

Ok, let's go then :)

Yes the redundancy leaves a terrible code smell. Rob can you create a follow up issue to implement the LanguageSystem and reference it here (if you haven't done that already).

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated the API changes section to use the constant as in the patch

I believe that #1550866: Remove obsolete drupal_language_initialize() is it. I'll add it to the issue summary.

Issue summary:View changes

Updated the remaining tasks section.

RTBC++

StatusFileSize
new563 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,816 pass(es).
[ View ]

Attached patch fixes the issue

Patch to be committed in #32.

Oops, posted to wrong issue queue. Ignore this patch.

StatusFileSize
new2.93 KB
PASSED: [[SimpleTest]]: [MySQL] 36,813 pass(es).
[ View ]

I've done the same thing so many times :-) . Same patch from #32!

Status:Reviewed & tested by the community» Fixed

Looks fine. Committed/pushed to 8.x.

Issue tags:-sprint

Done, moving off sprint.

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

Issue summary:View changes

Updated issue summary.