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

// Before
global $language_content;

// After
$language_content = drupal_container()->get(LANGUAGE_TYPE_CONTENT);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

plach’s picture

Issue tags: +D8MI, +language-base
cosmicdreams’s picture

Status: Postponed » Active

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

sun’s picture

cosmicdreams’s picture

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.

cosmicdreams’s picture

Status: Active » Needs review
sun’s picture

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)

RobLoach’s picture

FileSize
12.24 KB

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.

Tor Arne Thune’s picture

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

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

cosmicdreams’s picture

Clean and excellent thanks Rob, recommend RTBC.

plach’s picture

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

cosmicdreams’s picture

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.

RobLoach’s picture

FileSize
17.16 KB

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

cosmicdreams’s picture

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

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

August1914’s picture

Assigned: Unassigned » August1914
August1914’s picture

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

August1914’s picture

Assigned: August1914 » Unassigned
Tor Arne Thune’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

August1914’s picture

Assigned: Unassigned » August1914
August1914’s picture

Assigned: August1914 » Unassigned
Status: Needs work » Needs review
FileSize
26.54 KB

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.

August1914’s picture

Status: Needs work » Needs review
FileSize
915 bytes
26.46 KB

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.

August1914’s picture

Status: Needs work » Needs review
FileSize
603 bytes
26.53 KB

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.

August1914’s picture

Status: Needs work » Needs review
FileSize
963 bytes
26.52 KB

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.

August1914’s picture

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.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
2.93 KB

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.

Gábor Hojtsy’s picture

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

Looks simple and right.

plach’s picture

+++ 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.

RobLoach’s picture

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.

plach’s picture

Ok, let's go then :)

cosmicdreams’s picture

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).

cosmicdreams’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

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

RobLoach’s picture

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

RobLoach’s picture

Issue summary: View changes

Updated the remaining tasks section.

cosmicdreams’s picture

RTBC++

disasm’s picture

Attached patch fixes the issue

plach’s picture

Patch to be committed in #32.

disasm’s picture

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

RobLoach’s picture

FileSize
2.93 KB

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks fine. Committed/pushed to 8.x.

Gábor Hojtsy’s picture

Issue tags: -sprint

Done, moving off sprint.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.