LanguageFormatter is not used for nodes because it's still hard coded in NodeViewBuilder

Don't know why would this happen. I think we do have plenty tests around the language formatter. Hm. Maybe we only test it in views.

Wow, this is priceless, the NodeViewBuilder class still has a check on the 'langcode' component, overriding whatever is set in any formatter for this component ... :)

Hahaha. Definitely needs tests.

Changing the title

Funny, so there's a test that sets the field on the manage display screen. And it fails without the code in NodeViewBuilder because the asserts are checking on the 'field-language-display' ID.

Now, it's actually impossible to fix this without breaking markup. The original code has a wrapper which wraps the label and the value. A formatters returns the value, but can't set a prefix for the whole field. The label will have different wrappers too.

I honestly don't think a lot of sites display this element anyway, and if they want to alter it, it's kind of tedious to do. Also, additional formatters simply don't run or change the behavior of what's on the page. I think, IMHO, we can live with removing the field-language-display wrapper. But that's not my call of course :)

Patch attached fixes the tests.

Here's an explicit test for the fact that the 'Link to content' settings doesn't work :)

Could we do something like this for BC (and remove in D9) in NodeViewBuilder?:

if (isset($build[$id]['langcode'])) {
  $build[$id]['langcode']['#prefix'] = '<div id="field-language-display">';
  $build[$id]['langcode']['#suffix'] = '</div>';
}

That would still retain the flexibility of choosing another formatter, etc.

Not sure if it would a dealbreaker to lose the wrapper either, just thinking out loud here.

Regarding #13, yeah, that would work indeed. We could even add this as a setting in the formatter and default to TRUE ?

@tstoeckler/@swentel: depending on the formatter used, are we sure the prefix/suffix would work like that? It sounds overkill to add that to the formatter settings, but maybe an "Add field-language-display class for backwards compatibility" or some other obscure settings just to be strictly backwards compatible could work. Would be good to get some committer feedback as to how strong they feel about the need to keep that.

Talked to alex about it - tldr - we need an upgrade path to remove the 'link to content' in case someone saved it. And a way to keep the wrapper for BC. I think the formatter setting would work fine here.

<alexpott> swentel: 8.1 ... there's a question about upgrade path
<alexpott> swentel: given that it is not at all related to security I think we should ensure that all sites look the same before and after applying the patch
<alexpott> swentel: so I think we need a hook_update_N to ensure this
<swentel> alexpott, k, but, the update hook maybe isn't even necessary
<swentel> formatter settings defaults are merged, so should work out of the box
<alexpott> swentel: but what happens if someone has done the steps to reproduce?
<alexpott> swentel: won't applying the patch suddenly make the language field link to content?
<swentel> alexpott, no, because that one is FALSE by default
<swentel> alexpott, and it wouldn't work right now anyway
<alexpott> swentel: I mean if they've checked that now - don't we save that?
<alexpott> swentel: and then it doesn't work... but they leave it checked
<alexpott> swentel: then we fix it and suddenly the site has changed
<swentel> alexpott, fair point
<swentel> alexpott, although usage of the field will be pratically zero
<swentel> but oh well
<swentel> and who on earth made that setting, it's useless too :(
<alexpott> swentel: :)
<alexpott> swentel: existing sites are a pain-in-the-...
<swentel> alexpott, k, so, keep the wrapper as BC, and an update hook then to remove the link to content ?
<swentel> alexpott, in case they checked it
<alexpott> swentel: I think so - so fix the bug but don't change existing sites.
<swentel> alexpott, works for me, pasting this in the issue ok ?
<swentel> alexpott, oh, you say 8.1.x - do we only do update hooks in minor release, no patch ?
<alexpott> swentel: unless absolutely necessary yes

Looking at LanguageFormatter::viewValue() it just inherits entity linking from StringFormatter, its parent. Maybe it was a bad idea to inherit from that... A more unique setting for language formatters is whether to display with native name.

For the languagefield formatter, I copied the setting from Core language, and indeed, subclassing String introduces a content link. Which is a 'feature' for my use case. However, is this also the case for core language? It will always point to itself.
Perhaps when building Views showing all translations....

Right, if you are creating view with the languages as links to the translations, eg. a custom language selector block on an entity page with only the available translations listed.

Removing from sprint so it reflects what is currently being worked on.

The core committers and entity system maintainers agreed this issue can be handled as normal priority. (Criteria for major bugs: https://www.drupal.org/core/issue-priority#major-bug). Thanks everyone!

I also think it would be great to have a formatter that simply display the source language of the content!

What is left to do on this issue? I tried update issue summary.

Hi all,
I`ve rerolled the patch for 8.4 branch and fixed some coding standard issues.

+++ b/core/modules/node/tests/src/Functional/NodeTypeInitialLanguageTest.php
@@ -117,10 +114,17 @@ public function testLanguageFieldVisibility() {
+    $this->DrupalPostForm(NULL, [], "langcode_settings_edit");
...
+    $this->DrupalPostForm(NULL, $edit, "langcode_plugin_settings_update");

This should be a lowercase "d" in "drupal".

Thank you @tstoeckler, my bad.

The patch in #28 is working for me!

When I apply this patch finally this module is working;
https://www.drupal.org/project/flags
I also try make a another project for different language display formatters, but it is also not working without this patch;
https://www.drupal.org/sandbox/matsbla/2901140

Is there anyway to work around this bug?

To me it seems like there is no work-around on this issue so I put back to major according to "Priority levels of issues" (Block contributed projects with no workaround.)

Re-rolled the #28 patch against 8.6.x branch.

how about a patch for 8.5？

Re-rolled the #34 patch against the 9.1.x branch.

Fixed test case failure issue in #40, please review.

Re-roll against 9.3.x

Made a small mistake in the previous patch. Another attempt.

This does make sense, BC as always is tricky. Render array changes are allowed. But what about template placeholders/customizations?

We should create a change record at least to explain the structure change.

Steps to reproduce are clear so I performed to following manual test:

• Tested patch #46 on 9.5.0-dev and it worked as expected.
• Tested patch #46 on 10.1.0-dev and it didn't work.

I guess just removing the hardcoded part is not enough on Drupal 10 versions, I didn't have the time to troubleshoot.

Would be nice to have it resolved, but based in the infrequent participation on this issue I would say this not a blocker issue for most of Drupal users so I'm changing the priority to normal.

Rerolled Patch against 10.1x.

I fixed the test.

Previously, I was testing with drupalpod and for some reason it was not working for D10, but after doing a manual test with a local environment I can confirm this patch also works for that version.

Sorry, last patch had some issues.

I think it needs upgrade path for existing sites

I would say in this case a possible upgrade path could be to set existing language form display components to show the label and language as plain text.

What do you think?

Yes, so existing node displays will continue to work and new ones will get the same defaults as before

Added hook update on language module.

It only iterates through content types because the only entity type affected by this patch is node.

The inline comments (get ..., iterate) are a little bit to verbose. The update hook comment is enough for this function.
Just my 2cents. ;-)

Uploading test only patch.

Red/green as expected, @jidrone just a tip for next time if you upload the test only patch first it won't push the issue back to NW like it did here.

Minor review changes:

1. +++ b/core/modules/node/tests/src/Functional/NodeTypeInitialLanguageTest.php
   @@ -136,7 +136,17 @@ class NodeTypeInitialLanguageTest extends NodeTestBase {
   +    $this->submitForm([], t('Save'));
   

   We shouldn't wrap test strings in t()

2. +++ b/core/modules/node/tests/src/Functional/NodeTypeInitialLanguageTest.php
   @@ -136,7 +136,17 @@ class NodeTypeInitialLanguageTest extends NodeTestBase {
   +    $this->assertSession()->linkExists('English', 0);
   

   We don't need the second parameter here.

Also not sure if we need an update test?

I have uploaded the patch
Addressed #65 both points.

Fixing custom command failure and some comment typos

@Anchal_gupta you can test your patch before uploading using the core scripts.

Looks good to me and the points in #65 appear to be addressed.

1. +++ b/core/modules/language/language.install
   @@ -11,3 +11,38 @@
   +function language_update_10100() {
   +  $entity_display_repository = \Drupal::service('entity_display.repository');
   +  $entity_view_display = \Drupal::entityTypeManager()->getStorage('entity_view_display');
   +  $entity_type_id = 'node';
   +  // Get all content types.
   +  $content_types = \Drupal::entityTypeManager()
   +    ->getStorage('node_type')
   +    ->loadMultiple();
   +  // Iterate through content types.
   

   This needs to be a post update hook, we can't rely on the entity API to be available/error proof in an update hook.

2. +++ b/core/modules/language/language.install
   @@ -11,3 +11,38 @@
   +    $view_modes = $entity_display_repository->getViewModeOptionsByBundle($entity_type_id, $content_type_id);
   +    foreach ($view_modes as $view_mode => $view_mode_info) {
   +      /** @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface $view_display */
   +      $view_display = $entity_view_display->load("$entity_type_id.$content_type_id.$view_mode");
   +      if ($view_display) {
   +        if ($langcode = $view_display->getComponent('langcode')) {
   

   Let's use the config entity updater here

3. +++ b/core/modules/language/language.install
   @@ -11,3 +11,38 @@
   +          $langcode['label'] = 'above';
   

   Note to other reviewers, this is correct, in HEAD even if you set it to hidden the title still displays

Changed hook update to post update as suggested, and also using config entity updater.

Thanks @larowlan.

Sorry, last patch had some paths wrong.

Tests are all green
Patch applies cleanly
Running tests without the fix fail as expected
Update runs without issue
Code looks clean to me.

But will still need a change record

Can we get a change record/release note here? Fine to self RTBC after adding

Took a shot at the change record. Mostly borrowed from the issue summary since the IS was so well done.

Not sure what's needed for a release note.

I did some changes to the change record to make it more clear.

Thanks! So can this go back to RTBC

We still need a release note, though. (Ideally, the release note will link the change record.)

Done.

+++ b/core/modules/language/language.post_update.php
@@ -13,3 +16,32 @@ function language_removed_post_updates() {
+function language_post_update_language_component_plain_text(&$sandbox = []) {
...
+    // Update language component if present.
+    if ($view_display->getComponent('langcode')) {
...
+      $langcode['label'] = 'above';
...
+      $langcode['settings'] = [
+        'link_to_entity' => FALSE,
+        'native_language' => FALSE,
+      ];
+      $view_display->setComponent('langcode', $langcode);

it needs upgrade path test

Added test for the post_update.

Git was recognizing one file as binary when it generates the patch, but I found a workaround.

Fixed CS error.

Those dump files for update tests produce a lot of weird errors, hopefully I fixed all.

Maybe disabling the Spell check on some config imports.

Interdiff agains 83 because 84 didn't improve the issue.

+1 for me but will let someone else see what they think.

I tested manually the patch from #85.

Test #1
1. I used an empty Drupal 10.1.x
2. Applied the patch
3. Installed Drupal
4. Followed the patch from the IS

It worked as expected.

Test #2:
1. I used an empty Drupal 10.1.x
2. Installed Drupal
3. Followed the steps from IS
4. Applied the patch from the IS
5. Ran "drush updb".

It worked as expected, but I had to repeat the step 4: Check formatter option "Link to the Content", because it was unchecked after the update.

Let me know if I did something wrong.

Hi rpayanm,

That's the expected behavior after running the update, because it sets the formatter to match the hardcoded values to avoid unintended results on existing sites using that formatter.

Oh ok, thank you.

#2226493: Apply formatters and widgets to Node base fields says it covered this, but looks like it didn't?

1. +++ b/core/modules/language/language.post_update.php
   @@ -13,3 +16,32 @@ function language_removed_post_updates() {
   +
   +/**
   + * Set language form display component as plain text and with label above.
   + */
   +function language_post_update_language_component_plain_text(&$sandbox = []) {
   +  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
   +
   +  $callback = function (EntityViewDisplayInterface $view_display) {
   +    // Only update node view modes.
   +    if ($view_display->getTargetEntityTypeId() !== 'node') {
   +      return FALSE;
   +    }
   +    // Update language component if present.
   +    if ($view_display->getComponent('langcode')) {
   +      // Set label above as it was.
   +      $langcode['label'] = 'above';
   +      // Set to show it as plain text.
   +      $langcode['settings'] = [
   +        'link_to_entity' => FALSE,
   +        'native_language' => FALSE,
   +      ];
   +      $view_display->setComponent('langcode', $langcode);
   +      return TRUE;
   +    }
   +    return FALSE;
   +  };
   +
   +  $config_entity_updater->update($sandbox, 'entity_view_display', $callback);
   

   The logic here also needs to run in config entity presave so that it's applied to configuration shipped with modules and install profiles. Quite a lot of examples of this in 9.5 and 10.1, especially but not exclusively in views.

2. +++ b/core/modules/node/src/NodeViewBuilder.php
   @@ -41,16 +41,6 @@ public function buildComponents(array &$build, array $entities, array $displays,
    
   -      // Add Language field text element to node render array.
   -      if ($display->getComponent('langcode')) {
   -        $build[$id]['langcode'] = [
   -          '#type' => 'item',
   -          '#title' => t('Language'),
   -          '#markup' => $entity->language()->getName(),
   -          '#prefix' => '<div id="field-language-display">',
   -          '#suffix' => '</div>',
   -        ];
   -      }
        }
   

   Does the configured formatter end up in the same place in the render array?

#90 still needs actioning and this needs to go into an MR.

It might be a tricky reroll as well since update fixtures will need fixing up.

Reroll looks good @mohit_aghera just one question about the wrapper there, the IS also has conflicting statements about this (whether we're removing it or not)

Functional test failure also seems related.

Was perviously tagged for IS update so moving NW for that. Believe there is 1 question on the MR too.

The IS is up to date now that we're keeping the wrapper.

We've been using patch #46 on a project. However that patch no longer applies to D11.2+. The MR contains an update hook, which we rather don't use in patches. Attached patch only removes the langcode field formatter override in NodeViewBuilder, for safe usage with composer patches.

This MR is going to need a rebase. When that happens, the #[RunTestsInSeparateProcesses] attribute must be added to NodeLanguageFormatterUpdateTest or it's going to fail.

Sorry appears to need a rebase.

I added a commit because there appeared to be an issue with the MR.

This was in my view display config file:

  langcode:
    type: language
    label: inline
    settings:
      link_to_entity: false
      native_language: false
    third_party_settings: {  }
    weight: 0
    region: content

Yet, on the manage display page (/admin/structure/types/manage/article/display) I was getting the following error:

Warning: Undefined array key "region" in Drupal\field_ui\Form\EntityDisplayFormBase->buildFieldRow() (line 359 of core/modules/field_ui/src/Form/EntityDisplayFormBase.php).

Drupal\field_ui\Form\EntityDisplayFormBase->buildFieldRow() (Line: 41)
Drupal\field_ui\Form\EntityViewDisplayEditForm->buildFieldRow() (Line: 213)
Drupal\field_ui\Form\EntityDisplayFormBase->form() (Line: 108)
Drupal\Core\Entity\EntityForm->buildForm()
call_user_func_array() (Line: 563)
Drupal\Core\Form\FormBuilder->retrieveForm() (Line: 303)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 83)
Drupal\entity_confirmation\HtmlEntityFormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 634)
Drupal\Core\Render\Renderer::Drupal\Core\Render\{closure}()
Fiber->start() (Line: 635)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 54)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 745)
Drupal\Core\DrupalKernel->handle() (Line: 19)

The commit makes it so that the EntityViewDisplayConfigUpdater only changes the config (label/settings) when not yet present.