Split from #1827038: Remove stale references to language_content_type variable.

The previous issue dealt with converting a variety of language_* variables to CMI but it has got confusing, so I've split this off to focus on converting language_default only.

Changes required in:

core/includes/bootstrap.inc	
2571   $info = variable_get('language_default', array(

core/includes/update.inc	
326       $language_default = variable_get('language_default');
335         variable_set('language_default', (array) $language_default);
569     $language_default = variable_get('language_default');
581       variable_set('language_default', $language_default);

core/lib/Drupal/Core/Language/LanguageManager.php	
181     $default_info = variable_get('language_default', array(

core/modules/language/lib/Drupal/language/Tests/Condition/LanguageConditionTest.php	
52     // @todo remove this when language_default() no longer needs variable_get().

core/modules/language/language.module	
511     variable_set('language_default', (array) $language);

core/modules/language/lib/Drupal/language/Tests/LanguageDependencyInjectionTest.php	
74     variable_set('language_default', $new_language_default);

core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php	
111     variable_set('language_default', (array) $languages['vi']);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk’s picture

Issue summary: View changes

Listed instances of language_default

ianthomas_uk’s picture

OK, I'm unsure where these should go. They are required by shared code, so belong under system.* somewhere.

#1571632: Convert regional settings to configuration system was going to move the other variables on this form to system.regional, but that got switched to system.date, even though one of the options was country (pointed out in #84 but not followed up).

We could revive system.regional, and perhaps move country back there? Or maybe system.locale?

What about the variable itself? I think language on it's own is too vague and implies active language, so I'd suggest default_language (which happens to be consistent with #2078511: Rename getLanguageDefault() to getDefaultLanguage()).

I think my preference would be system.locale.default_language, which should be a sister of system.locale.country. What do people think before I write a patch?

ianthomas_uk’s picture

Status: Active » Postponed

These changes will conflict / not be possible without #1862202: Objectify the language system. This may be fixed in the final patch for that issue.

ianthomas_uk’s picture

Issue summary: View changes

Forgot the variable_sets

xjm’s picture

Issue summary: View changes
Issue tags: +beta blocker
xjm’s picture

ianthomas_uk’s picture

#1862202: Objectify the language system has landed, but it may now be simpler to fix this together with #2108599: Convert language_default to CMI. Leaving postponed, but now against #2108599: Convert language_default to CMI.

Gábor Hojtsy’s picture

alexpott’s picture

Status: Postponed » Needs review
FileSize
25.39 KB

So the patch here is converting default language to CMI. But it is also ensuring that the ConfigFactory always has a language set. In order to resolve the circular dependency the default language is stored on the container and there is a service to retrieve it.

I don't think it makes sense to tackle both language negotiation variables and default language together because default language is so fundamental and can be set in the installed even when the language module is not enabled.

Status: Needs review » Needs work

The last submitted patch, 7: 2108599.7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

7: 2108599.7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2108599.7.patch, failed testing.

Gábor Hojtsy’s picture

  1. +++ b/core/includes/install.core.inc
    @@ -301,23 +301,23 @@ function install_begin_request(&$install_state) {
    +            ->addArgument('%language.default%');
    ...
    +  $container->register('language.default', 'Drupal\Core\Language\Language')
    ...
    +  $container->setParameter('language.default', $default_language_values);
    

    Looks a bit possibly confusing that a parameter and a service has the same name. I know the params and services have different syntax as arguments, but it may end up being confusing nonetheless.

  2. +++ b/core/includes/install.core.inc
    @@ -385,13 +387,19 @@ function install_begin_request(&$install_state) {
    +    $container->setParameter('language.default', Language::$defaultValues);
    +    $container->register('language.default', 'Drupal\Core\Language\Language')
    +      ->addArgument('%language.default%');
    ...
    +    $container->register('language_manager', 'Drupal\Core\Language\LanguageManager')
    +      ->addArgument(new Reference('language.default'));
    

    You do these same things multiple times in the code/patch. It would be good to add comments as to why we need to redo these.

  3. +++ b/core/includes/install.inc
    @@ -651,6 +651,13 @@ function drupal_install_system() {
    +      ->set('default_language', $install_state['parameters']['langcode'])
    

    We attempted to make really sure that wherever you encounter 'language' in Drupal 8 that would be an object, wherever you encounter 'langcode', that would be the language code. So default_langcode would be more appropriate.

    The default_language variable that this patch replaces was an object, so it conformed to this naming convention :)

  4. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -650,5 +661,44 @@ public function getRawData() {
    +  /**
    +   * Gets original data from this configuration object.
    +   *
    +   * @see \Drupal\Core\Config\Config::get()
    +   *
    +   * @return mixed
    +   *   The data that was requested.
    +   */
    +  public function getOriginal($key = '') {
    +    if (!$this->isLoaded) {
    +      $this->load();
    +    }
    +
    +    // Apply overrides.
    +    $original_data = $this->originalData;
    

    Original data applies overrides?!?! Also looks like this duplicates logic from the config factory?

  5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -516,6 +517,13 @@ protected function buildContainer() {
    +    if ($default_language['id'] != $system['default_language']) {
    +      $default_language = array('id' => $system['default_language'], 'default' => TRUE);
    

    This is where the types of the two default_language values are entirely different (and btw neither of them is an object :/).

    I know we may not be able to instantiate a Language object here, although it should not need other runtime components to instantiate one. But at least the inconsistency between this array and scalar representation in naming should be resolved.

    Ideally the default language would be an instance. Eg. Language::defaultObject() would return an object or new Language() with the default values like it was in the language manager before.

  6. +++ b/core/modules/language/lib/Drupal/language/EventSubscriber/ConfigSubscriber.php
    @@ -0,0 +1,41 @@
    +      // Trigger a container rebuild by deleting compiled from PHP storage.
    +      PhpStorageFactory::get('service_container')->deleteAll();
    ...
    +    if ($saved_config->getName() == 'system.site' && $saved_config->get('default_language') != $saved_config->getOriginal('default_language')) {
    ...
    +    }
    

    This is very clever. Would need review from the container folks though :D

  7. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageDependencyInjectionTest.php
    @@ -50,29 +50,29 @@ function testDependencyInjectedNewLanguage() {
    +    $this->rebuildContainer();
    ...
    +    language_save($new_language_default);
    ...
    +    $this->rebuildContainer();
    ...
    +    language_delete('fr');
    
    +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
    @@ -106,18 +111,12 @@ function testUILanguageNegotiation() {
    +    $this->rebuildContainer();
    ...
    -    variable_set('language_default', (array) $languages['vi']);
    ...
    +    \Drupal::config('system.site')->set('default_language', 'en')->save();
    +    $this->rebuildContainer();
    

    Are these explicit container rebuilds still needed given that language_save() would do this already via the config event listener?

    Or in the later case the config save would result in the event listener called.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.39 KB
25.51 KB

1. Sure - parameter is now language.default_values
2. Actually we don't do it as much and existing comments cover it I think.
3. We're now using the langcode in the system.site file. This was being set to the default language ID at the end of the install process but after discussing with @Gábor Hojtsy and @marcvangend on IRC we agreed that if this langcode was not the site's default language the situation would be very contrived.
4. I thought long and hard about this and yes I think so. If overrides are disabled then there will no overrides here. If they are enabled and you want to react to change then you need to be comparing the value the system has and what you've just set.
5. Fixed when dealing with an array the variables are *_values and when dealing with just a language id we're using langcode.
6. :) Improved comment to say when the container will be rebuilt - which is the next request.
7. The reason why we have to the rebuild the container is because just deleting the PHP storage will not refresh the in memory implementation.

This patch also fixes the installer - at the moment if you install in a language other than english the steps on the left hand side revert to english on the installing profile step!

Gábor Hojtsy’s picture

I think the changes look good.

The reason I'm uncomfortable with getOriginal() is if you compare the newly set value with the original, the newly set value get() will apply the override while getOriginal() will also apply it, so they will be equal even if the underlying value changed. This is currently not a direct problem with the comparison in the patch since we compare langcode and we currently don't put a langcode in the override files (although several people argued that would make total sense, since then you could tell the actual language of the config by looking at the langcode and the override langcode would proberly apply, now you have no way to actually tell the langcode of the current runtime config value). But it will be a problem at least generally for whoever wants to do similar comparisons no?

Status: Needs review » Needs work

The last submitted patch, 12: 2108599.12.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
29.74 KB

The DUBT and testbase environments needed the language.default_values set up so that tests and t() in the tests will work.

Status: Needs review » Needs work

The last submitted patch, 15: 2108599.15.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
20.23 KB
41.63 KB

We need to be able to change the default language mid request and have it take effect so I have changed the language.default service on the container from being just an instance of Language to a new LanguageDefault service which is a simple getter and setter wrapper around a Language object.

Added a new exception to ensure that language_delete() does not delete the default language as this is totally unsupported. This business rule is currently only enforced through the UI.

What is very interesting is that many of our multilingual tests run in an out of sync environment. This is because once additional languages are added the LanguageServiceProvider alters the container and adds a path processor. But in HEAD url_generator service is persistent so no amount of container rebuilding will ever get this path processor to fire in the instance of the test environment that is actually running the test. However the path processor will be active for any requests made via drupalGet, drupalPost etc... This patch removes the persist tag from url_generator - it depends on way too much stuff to be persistent!

Status: Needs review » Needs work

The last submitted patch, 17: 2108599.17.patch, failed testing.

Gábor Hojtsy’s picture

I think these changes look great. But the update tests are failing :) However we discussed that the original value problem may not be this easy to solve, and would be better possibly in another issue separately, so we can just rebuild the container whenever the language default is saved. Is that happening too often?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
41.33 KB

Just re-roll as the tests failing in #17 are green locally.

P.S not related to this ticket, but the failing tests needs message update

$this->assertNoLinkByHref('update.php', 0);
$this->assertNoLinkByHref('update.php', 1);

Where the 2nd param is message. It can be just

$this->assertNoLinkByHref('update.php');
Gábor Hojtsy’s picture

@vijaycs85: yes, that fail was due to #2176669: Stop persisting the url_generator which was recently fixed. Also I think slightly conflicted with this patch since @alexpott included part of that here originally :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Reviewed again! Looks good except (some non-major stuff):

  1. +++ b/core/includes/install.inc
    @@ -613,7 +613,7 @@ function drupal_verify_profile($install_state) {
      */
    -function drupal_install_system() {
    +function drupal_install_system($install_state) {
    

    New argument needs docs.

  2. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -650,5 +661,44 @@ public function getRawData() {
    +  /**
    +   * Gets original data from this configuration object.
    +   *
    +   * @see \Drupal\Core\Config\Config::get()
    +   *
    +   * @return mixed
    +   *   The data that was requested.
    +   */
    +  public function getOriginal($key = '') {
    

    So it took me some time earlier to realize original != raw. So it would be good to document that original means "in the form it was last saved in" and/or "without unsaved runtime changes" or somesuch. Also I'd mention overrides are still applied.

    I see the override application makes no difference for the default language practically, so I leave it to your judgement to include this method/approach here or move to a different issue. I'm actually fine either way (if we make the purpose of this method cleaner with docs).

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
41.95 KB

Thanks Gabor!

Feeback in #22 addressed.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

No other concerns left, looks good to me :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Actually this is not going to work in its current form - there is no need to set the language on the configuration factory in regular runtime if the Language module is not enabled. Doing so incurs unnecessary costs. The exception to this during the installer which is the only time when Drupal can have a default language other than English and not have the Language module enabled.

How to progress?

  • The LanguageDefault service should not required on the ConfigFactory and it should not be injected during on a site without Language enabled.
  • It needs to be injected during the installer

Patch coming.

alexpott’s picture

Status: Needs work » Needs review
FileSize
14.12 KB
47.74 KB

With the patch attached the config factory only ever has the language set when the Language module is enabled. During the installer the default language can still be something other than english even though the language module is not enabled. However during the installer the config factory will never have the language set unless the language module is enabled. If the Language module is enabled then the LanugageServiceProvider adds a method call to the config factory definition to set the language to the default language the moment the config factory service is instantiated. That means that if the site is monolingual and has the Language module enabled language overrides still apply (as they should). Currently in HEAD this would actually be broken so I've added a test for this situation.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
47.95 KB
3.78 KB

I think those changes look good. The bug solved there was not introduced by earlier patches in this issue so its really a "side-thing" I think :) Its good to have that resolved nonetheless and made it clearer for us how to approach this. I think it would be good to get in this step ASAP (today before sprint weekend? :), so that we can move on with the remaining variable_get removal in the language negotiation system.

I noticed several grammar issues in comments but those are the only things changed (and there were some offsets when applying), so moving to RTBC as well :)

webchick’s picture

Assigned: Unassigned » catch

Since alex wrote some of this patch, assigning to catch.

alexpott’s picture

FileSize
565 bytes
48.47 KB

Notice a comment in one of the language tests that says we can remove more code :)

Gábor Hojtsy’s picture

Issue tags: +sprint

How come not on the sprint :)

catch’s picture

Title: Convert language_default to CMI » Change notice: Convert language_default to CMI
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed/pushed to 8.x, thanks!

michaellenahan’s picture

michaellenahan’s picture

Title: Change notice: Convert language_default to CMI » Convert language_default to CMI
Status: Active » Fixed
Issue tags: -Needs change record
michaellenahan’s picture

Status: Fixed » Needs review

Sorry I think I was too fast in setting this as fixed. I need a review for the change report, please.

michaellenahan’s picture

I came across this todo while writing the change report, so I guess this needs a follow-up.

class Language {

  /**
   * The values to use to instantiate the default language.
   *
   * @todo Remove once the default language is converted to config. See
   *  https://drupal.org/node/2108599.
   *
   * @var array
   */
  public static $defaultValues = array(
    'id' => 'en',
    'name' => 'English',
    'direction' => 0,
    'weight' => 0,
    'locked' => 0,
    'default' => TRUE,
  );
vijaycs85’s picture

penyaskito’s picture

Assigned: catch » Unassigned
Status: Needs review » Fixed

I reviewed the Change Record and edited for clarifying that LanguageDefault is a service.
I think we can set this as fixed now.

Gábor Hojtsy’s picture

Priority: Major » Critical
Issue tags: -SprintWeekend2013, -sprint +SprintWeekend2014

Woot, yay!

Status: Fixed » Closed (fixed)

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