Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because entity and field language settings are hidden at different places and even shown when does not make sense (fields setting shown even on views where entities are not translatable)
Issue priority Major because it makes very hard to create language based views while we modified many pages and blocks to be views and promote better multilingual features.
Prioritized changes The main goal of this issue is usability improvements. Further usability improvements are planned in #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage.
Disruption Changes where entity view language rendering settings are stored which may be modified again in #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage (or may be merged with field language settings, in which case those would change) but this separation of issues makes it easier to review / digest.

Problem/Motivation

As per #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language" we need to move and unify the language settings for different types of views. Now entity views have a row setting and field views have a display level setting. We need to unify those two. As a first step we should move the row setting to the display level and then move on to rework the two settings to apply to language handling regardless of whether on an entity or fields based view.

Proposed resolution

Move entity row based language settings to the display level. Ensure tests still work.

Remaining tasks

Patch. Review. Commit.

User interface changes

Entity views will have a display level setting for language instead of row level. Two language settings moved together to one place.

(Opened #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage to unify them).

API changes

Language settings for an entity based view will now be on the display not the row plugin.

CommentFileSizeAuthor
#6 interdiff.txt1.51 KBGábor Hojtsy
#6 2394041-display-language-setting-6.patch18.31 KBGábor Hojtsy
#4 interdiff.txt3.64 KBGábor Hojtsy
#4 2394041-display-language-setting-4.patch18 KBGábor Hojtsy
#3 interdiff.txt3.28 KBGábor Hojtsy
#3 2394041-display-language-setting-3.patch16.08 KBGábor Hojtsy
#1 2394041-display-language-setting.patch12.8 KBGábor Hojtsy
#1 Frontpage (Content) | drupal8.local 2014-12-16 12-01-35.png283.88 KBGábor Hojtsy
#1 Frontpage (Content) | s9270c6412a5bc03.s3.simplytest.me 2014-12-16 11-58-52.png157.02 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
Status: Active » Needs review
FileSize
Frontpage (Content) | s9270c6412a5bc03.s3.simplytest.me 2014-12-16 11-58-52.png157.02 KB
Frontpage (Content) | drupal8.local 2014-12-16 12-01-35.png283.88 KB
2394041-display-language-setting.patch12.8 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2394041-display-language-setting.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2394041-display-language-setting-3.patch16.08 KB
interdiff.txt3.28 KB

The field langcode edit test was assuming that the option will display if the site is not multilingual which is silly and not anymore the case in the test. Fixing that assumption.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
2394041-display-language-setting-4.patch18 KB
interdiff.txt3.64 KB

Unify the entity base table translatability checking which applies the same way to the rendering language setting. This makes it sure that these will not appear either for non-entity based views, where they don't make sense.

Status: Needs review » Needs work

The last submitted patch, 4: 2394041-display-language-setting-4.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
2394041-display-language-setting-6.patch18.31 KB
interdiff.txt1.51 KB

Of course the test view is not entity based so the field language selector should not show up.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Assigned: Unassigned » Gábor Hojtsy
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Got this feedback, will take a short look:

[3:57pm] dawehner: GaborHojtsy: don't you think that ideally $row->_entity should contain already the entity in the language you like it to be?
[3:57pm] dawehner: GaborHojtsy: so we could maybe move the logic in RendererBase::preRender to be used all the time

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

This patch looks very straightforward. Not sure about dawehner's feedback, but I do not see any problems with the patch in #6, and it passes the test, so I would be +1 for RTBC at this point...

Because it looks to me as though the patch here does exactly what the issue summary proposes: Moves the Row-based language setting for Entity views to the Display, and puts that and the Field language setting into a new Language area on the view. Nothing more. Nothing less. I am not sure what #8 means but... is it a separate issue?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Well, this feedback was regarding solving the problem that field rendering still works entirely different.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes

Submitted #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage, postponed on this issue to unify them. Let's see if a separation of steps like this works well with committers. It means more manageable change but also two consecutive API changes at least in terms of where the plugins source their data from in the views structure.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

We will need both a Beta Evaluation and a Change Record before this can be committed.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes

Added beta evaluation, please update if you don't agree.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Draft change notice at https://www.drupal.org/node/2394917

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Beta and change notice look good to me, this should be ready to commit I hope!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -1267,14 +1274,21 @@ public function optionsSummary(&$categories, &$options) {
+    if (\Drupal::languageManager()->isMultilingual() && $this->isBaseTableTranslatable()) {
+      $rendering_language_options = $this->buildRenderingLanguageOptions();
+      $options['rendering_language'] = array(
+        'category' => 'language',
+        'title' => $this->t('Entity Language'),
+        'value' => $rendering_language_options[$this->getOption('rendering_language')],
+      );
+      $language_options = $this->listLanguages(LanguageInterface::STATE_ALL | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED);
+      $options['field_langcode'] = array(
+        'category' => 'language',
+        'title' => $this->t('Field Language'),
+        'value' => $language_options[$this->getOption('field_langcode')],
+        'desc' => $this->t('All fields that support translations will be displayed in the selected language.'),
+      );
+    }

shouldn't we be setting #access on these elements depending on Drupal::languageManager()->isMultilingual() && $this->isBaseTableTranslatable() instead of making them conditional.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Reviewed & tested by the community

@alexpott: if you look at the existing code in that optionsSummary() method, there are no use of #access on ANY of the items, however there are dozens of conditionally added items like:

    if ($style_plugin_instance->usesRowPlugin()) { ... }
    if ($this->usesAJAX()) { ... }
    if ($this->usesAttachments()) { ... }
    if (!isset($this->definition['contextual links locations']) || !empty($this->definition['contextual links locations'])) { ... }
    if ($this->usesMore()) { ... }
    if ($this->view->query->getAggregationInfo()) { ... }
    if ($this->usesLinkDisplay()) { ... }
    if ($this->usesExposedFormInBlock()) { ... }

It is true that these option summaries seem to be ending up in a render array later on, so theoretically #access could work but it would be *very* different from how the rest of this method is written.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

#19 okay - fine by me. Committed 145b53c and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed 145b53c on 8.0.x 
    Issue #2394041 by Gábor Hojtsy: Row language settings from entity views...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Thanks, published change notice. Onwards to #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage.

Status: Fixed » Closed (fixed)

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