Problem

The filter named "Current user's language" is very misleading. Those who know the language system in Drupal core would assume this filter will apply the language preference of the user which is not at all the case. What it's really doing is going through language negotiation using the "Content" negotiation settings for the page where the view is displayed. So the language used is not from the user profile but determined dynamically for the page (and may depend on URL components, session values, etc.)

This is especially misleading since now for most entity types, you have a settings page that lets you default new instances of that entity type to a specific language. It has dynamic options like "Site default language", "Current interface language", "Author's preferred language". The author's preferred language actually means the language of the user authoring the entity there, unlike in views, where the user's language may or may not be the user's preferred language.

Proposed resolution

- Figure out a better label for this, and rename this filter to that.
- Add an option to choose the "UI text" negotiated language, as an alternative to the "Content" negotiated language. This will need a sensible label as well.
- Regularize the machine code strings used to represent these language options in views configs, as well as for the site default language option.

Proposed names: "Selected language for Content' for content, and "Selected language for User interface text' for UI ("Content" and "User interface text" are the 'name' elements of hook_language_type_info() for these two types).

Proposed machine codes: The strings already defined for them in other contexts (language_content, site_default, etc.), with prefixes/suffixes to avoid possible problems in query substitution.

The function for listing languages has also been moved out of views.module and into the base Views plugin class.

Files: 

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
17.7 KB
FAILED: [[SimpleTest]]: [MySQL] 56,807 pass(es), 4 fail(s), and 0 exception(s). View

Oh that might be a reason people got confused already in drupal 7?

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1215,7 +1215,8 @@ public function optionsSummary(&$categories, &$options) {
     $languages = array(
-        '***CURRENT_LANGUAGE***' => t("Current user's language"),
+        '***CURRENT_LANGUAGE_CONTENT***' => t('Current content language'),
+        '***CURRENT_LANGUAGE_INTERFACE***' => t('Current interface language'),
         '***DEFAULT_LANGUAGE***' => t("Default site language"),
         Language::LANGCODE_NOT_SPECIFIED => t('Language neutral'),
     );

There can be more types of languages negotiated. Also the content language may not be customized from the interface language. So I'd fill in this list dynamically with options based on language negotiation types if at all possible. See how only certain language types get blocks exposed for example. It may be a quick fix to just rename this option for now and introduce more language types in a followup instead of doing them both at the same time?

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.31 KB
FAILED: [[SimpleTest]]: [MySQL] 55,370 pass(es), 1,261 fail(s), and 1,972 exception(s). View

This should be better, as it supports all types.

Status: Needs review » Needs work

The last submitted patch, vdc-2037979-3.patch, failed testing.

Gábor Hojtsy’s picture

Looks good except (apart from the fails):

@@ -22,9 +23,14 @@ class LanguageFilter extends InOperator {
+        Language::LANGCODE_NOT_SPECIFIED => t('Language neutral'),

@@ -1229,8 +1229,11 @@ public function optionsSummary(&$categories, &$options) {
         Language::LANGCODE_NOT_SPECIFIED => t('Language neutral'),

Drupal 8 does not have language neutral anymore. The special languages like 'Not specified' and 'Not applicable' that are built into Drupal 8 are config entities like the configured ones. Contrib can extend with more. So adding this one only manually would be incorrect. language_list() would return all the special languages if you ask it to do so.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
4.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,664 pass(es), 317 fail(s), and 1,519 exception(s). View

Ah good to know.

This should also fix most of the test failures.

Gábor Hojtsy’s picture

Call language_list(Language::STATE_ALL) to get all the special languages *as well as* the configured languages. I'm not sure that is used yet in the code, its not visible in the patch. Sorry I was on the road yesterday and could not look up specific API tips.

The last submitted patch, vdc-2037979-6.patch, failed testing.

Gábor Hojtsy’s picture

jhodgdon’s picture

jhodgdon’s picture

Issue summary: View changes

So... I took a look at the proposed patch from @dawehner vs. the issue summary.

The issue summary is saying that the "Current user's language" label in the UI is not clear, because what it really means is "the language that is negotiated, using the 'Content' negotiation settings".

The proposed patch doesn't actually fix this issue. Instead, it adds an additional choice (you can also get "the language that is negotiated, using the 'User Interface' negotiation settings"), and it changes the displayed names of these two choices to "Content" and "User interface text". In the context of this being a Views setting for the user to choose which language to display things in, I do not think that these two labels make any sense.

And besides that, the current patch would not work, because it is old and language_types_info() no longer exists. Probably need to call ConfigurableLanguageManager::getDefinedLanguageTypesInfo() instead? That is where hook_language_types_info() is invoked.

Also, it needs to skip any language types that are don't have names. There are 'fixed' entries in the list, and since they do not have names, I do not think they should be shown.

So... I think it's a fine idea to add more choices, but I don't think just using the name of the language type as the label for the Views choice is a good idea at all. It should probably be something like "Language negotiated for [name]" and have a field description that explains something about language negotiation and/or links to the admin page for that.

Sweetchuck’s picture

Issue tags: +Drupalaton 2014

I am starting to work on this issue.

Gábor Hojtsy’s picture

Reviewed this yesterday and wanted to post this at least :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -1229,10 +1229,12 @@ public function optionsSummary(&$categories, &$options) {
-        Language::LANGCODE_NOT_SPECIFIED => t('Language neutral'),

@@ -1598,10 +1600,12 @@ public function buildOptionsForm(&$form, &$form_state) {
-            Language::LANGCODE_NOT_SPECIFIED => t('Language neutral'),

I was a little puzzled by these two lines. They are not language types, BUT now language_list() returns these special languages as well, and the code later on uses array_merge() so these items are overwritten with the right name. So what these two lines did is to put this one special language in the wrong place... So these two lines are already superfluous prior to this patch, it is nothing else in the patch that make them superfluous, they already are.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Issue tags: -Drupalaton 2014

Well, this didn't get done at Drupalaton. I'm going to take it on.

jhodgdon’s picture

I'm making progress on this.

I found an interesting bug in the LanguageManager class: if you repeatedly call LanguageManager::getLanguages() with $flags containing the option to get the site default language...

On the first call, it sets the name of the default language to
Site's default language (Spanish)
And then on the next call while the language manager is still instantiated, it does:
Site's default language (Site's default language (Spanish))
And so on. Oops. I'll have to fix that in this patch; it's really really really ugly otherwise.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
12.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,944 pass(es), 584 fail(s), and 2,896 exception(s). View

OK, here's a patch. It might work... seems to work in manual testing. Let's see what the test bot says.

Note: There are a lot of views in Core that have the old ***CURRENT_LANGUAGE*** or ***DEFAULT_LANGUAGE*** hard-coded in them. If this patch works, we should also patch existing views and take out the @todo lines.

Anyway... let's get this up and see how it goes. No interdiff; the previous patch was VERY old and didn't come close to applying.

Sweetchuck’s picture

FileSize
18.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,789 pass(es). View

This is what I made on the code sprint.

Sweetchuck’s picture

Status: Needs review » Needs work

Why these patches are postponed?

Sweetchuck’s picture

Status: Needs work » Needs review

The last submitted patch, 16: 2037979-language-options-views-16.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Drupalaton 2014

So this was obviously worked on at Drupalaton (see #17). Not sure why is it so wrong to keep the tag either way BTW :)

I think a solution somewhere between the two proposed is right. Eg. one of the problems @Sweetchuck uncovered while working on this is the negotiated language types only provide meaningful descriptions for the UI if the configurable language manager is available. That is when language module is enabled. I see @jhodgdon you worked around that problem by not including the negotiated options in the list of options when the configurable language manager is not available. That does not really work for shipped views, which currently do depend on a negotiated language in their configuration. When they are edited, they would not be valid, since without a language module that is not an option.

Also @Sweetchuck fixed the actual views, while @jhodgdon left a todo item and some backwards compatibility code in. Unless we want to postpone fixing the views to a followup issue, I think we should fix them here. When they are re-saved, the correct language code is used for negotiated languages, so they will not be deployable as-is unless we break with backwards compatibility.

jhodgdon’s picture

OK. I have reviewed Sweetchuck's patch carefully, and I think we should start from that patch and add a few things from mine, plus a couple more fixes... Here are some thoughts:

a) Generally I think we should prefer class methods to adding functions in views.module, and we should do as I did in my patch, if possible, and move functions from views.module to being methods.

b) I think I also like the idea of making the listLanguages() function handle all the options, similar to how I did it in my patch.

c) The patch hunk in my patch for LanguageManager should be added.

d) I'm undecided on the names; probably either scheme would be OK.

e) I like the idea of adding the pieces to the default Language Manager interface/class so that the "negotiated" languages would be defined even without the Language module, but we probably shouldn't call them "negotiated" if there is not actually any negotiation taking place.

Anyway. I will work on the next patch. Nice work @Sweetchuck!!!

jhodgdon’s picture

So...

This is very confusing.

There are three similar methods on the Language Manager classes/interfaces:

getLanguageTypes()
getDefinedLanguageTypes()
getDefinedLanguageTypeInfo()

The first two return just the machine names of languages, and the 3rd returns an array of information.

In the ConfigurableLanguageManager class, these methods do the following (without any patching):
- getLanguageTypes ==> gets config object language.types, returns the "configurable" component, so it returns only the defined language types that are configurable. At install, this config is only language_content.
- getDefinedLanguageTypes ==> gets config object language.types, returns the "all" component, so it returns all of the available language types. At install, this config is language_content, language_interface, and language_url.
- getDefinedLanguageTypeInfo() ==> invokes hook_language_type_info() and hook_language_type_info_alter(), and returns the result. language.module is the only core implementer of the info hook, and it returns info for the three default language types (with all of them set to "locked", i.e. not configurable). content_translation is the only core implementer of the alter hook, and it makes language_content configurable.

On the parent LanguageManager class (which is active if Language module is not installed):
- getLanguageTypes() returns a fixed list of language_content, language_interface, and language_url.
- Without any patch, the other two methods are not defined.
- With Sweetchuck's match, the getDefinedLanguageTypeInfo() method is defined, and it returns the Language module's implementation of hook_language_type_info(), without the Content Translation module's alteration. Also, getLanguageTypes is changed to return the array keys of getDefinedLanguageTypeInfo(), which results in the same information.

So... This is kind of an inconsistent mess. I agree with Gabor that we need the "negotiation" options to be defined even if the Language module is not installed, but I'm not sure this is the right way to do it, and I think we shouldn't be mixing up getLanguageTypes() with getDefaultLanguageTypeInfo().

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
32.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,772 pass(es). View
25.62 KB
28.71 KB

OK... All of that considered, here is a new patch. I've included interdiffs between my patch in #16 as well as Sweetchuck's patch in #17. I think/hope I've taken the best parts from both... Also all of the views have been converted (only some of them were in Sweetchuck's patch). Let's see what the test bot thinks, and go from there.

Status: Needs review » Needs work

The last submitted patch, 24: 2037979-language-options-views-24.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Test bot says "OK!". Reviews?

(By the way the first failure was due to a failed Git checkout on the bot, nothing to do with this patch or the code in general at all, I'm assuming some kind of network fail.)

Gábor Hojtsy’s picture

Issue tags: +sprint

Generally looks great IMHO. Happy to see that the work from @Sweetchuck and you came together. Looks like @Sweetchuck did modify getLanguageTypes() on the LanguageManager to return the very same values it did before but rely on less baked-in data. Why did you decide to not take that over? Now the LanguageManager has the language types baked in twice.

  1. +++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_field_filters.yml
    @@ -301,7 +301,7 @@ display:
    +      field_langcode: 'language_content'
    

    I personally don't have a problem for using the language type as-is but there was certainly some value in the special cases marked with *** and such. I guess best responded to by a Views maintainer.

  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -439,4 +448,87 @@ public function getDependencies() {
    +          $list[ $id] = t('Negotiated !type language', array('!type' => $type['name']));
    

    I'd love to use some more accessible wording here. The word "negotiate" only appears once on the configuration page for configuring negotiators because it was deemed non-accessible language. So maybe "Language selected for !type" maybe?

  3. +++ b/core/modules/views/views.api.php
    @@ -569,8 +569,8 @@ function hook_views_query_substitutions(ViewExecutable $view) {
    +    'site_default' => \Drupal::languageManager()->getDefaultLanguage()->id,
    

    Should we have a constant for this? Looks odd that it is not consistent.

  4. +++ b/core/modules/views/views.views_execution.inc
    @@ -7,17 +7,20 @@
    + * Substitute current time; this works with cached queries. Also substitute
    + * Drupal version. Also figure out the special language codes; see
    + * \Drupal\views\Plugin\views\PluginBase::listLanguages().
    

    Too much "also"?

jhodgdon’s picture

The reason I didn't think it was a good idea to mix getLanguageTypes() and getDefaultLanguageTypeInfo() is that semantically they are different, as noted in comment #23. If a hypothetical child class were to override getDefaultLanguageTypeInfo(), then it would no longer provide the correct return value for getLanguageTypes() -- really it's providing the correct return value for getDefaultLanguageTypes() on the Configurable interface, not the correct return value for getLanguageTypes(). So ... I really think they should be kept separate, even though hard-wiring it twice is slightly less desirable for the base LanguageManager class. (Take a look at ConfigurableLanguageManager -- the return value for getLanguageTypes does *not* agree with the return value for getDefaultLanguageTypeInfo()).

Agreed on the rest; I'll make another patch shortly.

Gábor Hojtsy’s picture

Ok, for theoretic class extensions, ok. It is not a lot of duplication, so fine. :)

Gábor Hojtsy’s picture

BTW wanted to upload this demonstration of how we don't use "negotiate":

jhodgdon’s picture

Great. Looks like it's called "detection" for the steps and "selection" for the result, so we should call it "selected/selection". Thanks!

Gábor Hojtsy’s picture

Yeah although "detection" appears more there, I think it shows that the "detection methods" are used to "select" a language so wording it "Language selected for !type" would be accurate in this terminology.

jhodgdon’s picture

FileSize
32.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,078 pass(es), 1 fail(s), and 0 exception(s). View
1.5 KB

Agreed on #34...

Regarding #29...

As far as the hard-coding of 'site_default', it is all over Core already, and doesn't have a constant... I think we should file a separate issue for that rather than trying to fix this here.

Item 1: I don't see a compelling reason not to use the negotiation codes as-is. They are unlikely to be confused with language codes, since they are things like 'language-content', not 'fr', 'es', etc. It is just a lot simpler to do it that way.

Here's a new patch that fixes the "Negotated" wording and the docs in views_execution.inc.

Status: Needs review » Needs work

The last submitted patch, 35: 2037979-language-options-views-35.patch, failed testing.

jhodgdon’s picture

FileSize
32.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,110 pass(es). View
833 bytes

Forgot to update the UI test for the new "Negotiated" => "Selected" language.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -439,4 +448,87 @@ public function getDependencies() {
+          $list[ $id] = t('Language selected for !type', array('!type' => $type['name']));

+++ b/core/modules/views_ui/src/Tests/ViewEditTest.php
@@ -94,7 +94,7 @@ public function testEditFormOtherOptions() {
+    $this->assertLink(t('Language selected for !name', array('!name' => t('Content'))));

Now this looks almost perfect, except you used differently named placeholders in the test and the live code. While this would only become a problem if for some reason people run this test on a localized site, best to use 100% the same string.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
835 bytes
32.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,119 pass(es). View

I can fix that minor thing. Other than that it was/is RTBC :) This is so minor that I don't think I would not be eligible to RTBC.

jhodgdon’s picture

+1 on your minor fix. Good catch! Thanks for the very careful review; always makes me feel more confident about a patch when someone actually looks at every line of code for stuff like this. :)

jhodgdon’s picture

As a note, we also will need to update the ***CURRENT_LANGUAGE*** codes on the patch in #2323899: Provided default Node views need language filtering depending on which issue lands first.

Gábor Hojtsy’s picture

Assigned: jhodgdon » Unassigned
FileSize
708 bytes
32.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es). View

Well, if you call my attention line by line, then we surely need this whitespace fixed :D

Also unassign so committers don't believe you are going to commit this :) The title sounds like a textual fix. LOL.

jhodgdon’s picture

Good one. :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -2,13 +2,15 @@
    +use Drupal\Core\Language\LanguageManagerInterface;
    

    Not used

  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -439,4 +448,87 @@ public function getDependencies() {
    +  function listLanguages($flags = 0) {
    ...
    +  static function queryLanguageSubstitutions() {
    

    Public?

  3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -439,4 +448,87 @@ public function getDependencies() {
    +    $changes['site_default'] = $default;
    ...
    +    $types = $manager->getDefinedLanguageTypesInfo();
    

    I think we only have test coverage of TYPE_CONTENT... we don't have site_default or the new interface language tested.

  4. +++ b/core/modules/views/views.views_execution.inc
    @@ -7,17 +7,22 @@
     use Drupal\Core\Language\LanguageInterface;
    

    Can be removed.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
32.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,111 pass(es), 3 fail(s), and 0 exception(s). View
1.69 KB

Hm. In comment #45...

1. LanguageInterface is used in the listLanguages method:

    if (!$flags) {
      $flags = (LanguageInterface::STATE_ALL | LanguageInterface::STATE_SITE_DEFAULT);
    }

2. Right. Done. (new patch)

3. Tests... yeah, see below...

4. Right. Done. (new patch)

I'm not sure what to do for the test. We have lots of implicit test coverage for the 'language_content' type from $types = $manager->getDefinedLanguageTypesInfo() [several default views including /node home page use this]. And there's a test in the Views UI module as well that covers this somewhat.

But 'language_interface' is new, and I didn't write a test for that. And you are right, we do not have coverage for "site default language" either -- previously views allowed for ***DEFAULT_LANGUAGE***, but there were zero views configs in core or tests that used it. It's really not all that useful... but yeah probably should have a test added.

This new centralized language list is being used in:

a) The "field display language" setting on the Display. That is what the current test in core/modules/views_ui/src/Tests/ViewEditTest.php that was edited for the previous patches is testing, but this setting really doesn't work for actual multilingual sites and is going to be revamped completely -- see #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language". So there is no point in adding a test there right now for the additional language options, in my opinion. (That is the code in the patch in core/modules/field/src/Plugin/views/field/Field.php and core/modules/views/src/Plugin/views/display/DisplayPluginBase.php).

b) language arguments: core/modules/views/src/Plugin/views/argument/LanguageArgument.php -- the current patch just calls the language listing function with its default flags, so it is allowing any language as well as 'site_default' as a language argument. Hm. That seems a bit odd, actually, and previously this wasn't the case -- the function that was being called before only listed actual languages, not "site default". There's no user interface for the list; it's just for validating the value... So I'll fix that class so it only allows actual languages. Hence, no new tests needed there. (new patch)

c) language filters: core/modules/views/src/Plugin/views/filter/LanguageFilter.php -- this is what we should test. It allows regular languages, 'site_default', and negotiated language types; the latter should be tested with and without the Language/Locale/Content Translation modules installed I guess. Currently only Node is using this filter effectively... but the filters are only defined if the Language module is enabled... so this is going to be difficult to test. See item #3 in the Views Multilingual meta -- Node, Comment, User, and Taxonomy all have problems with their language filters currently. #2313159: [meta] Make multilingual views work

So... yes I agree this should have tests, but ... it's kind of problematic to actually make good tests until we have filters using this plugin available.

So for now I have fixed the above issues. Can we do a followup for the tests once we have a viable entity to make views with, or add Remaining Tasks to the Node, Comment, User, and Taxonomy filtering issues to test all of them using the 4 possible language options for filtering (actual languages, 'site default', 'language_content', and 'language_interface')?

jhodgdon’s picture

FileSize
32.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,113 pass(es), 3 fail(s), and 0 exception(s). View

Oh, doh! in #45 - 1 you mentioned the LanguageManagerInterface, not LanguageInterface. Here we go. Removed that 'use'.

use Drupal\Core\Language\LanguageManagerInterface;

didn't make interdiff file.

The last submitted patch, 46: 2037979-language-options-views-46.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 47: 2037979-language-options-views-47.patch, failed testing.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
31.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es). View
2.34 KB

doh, forgot a use statement. Actually, since LanguageArgument is the only current user of the more limited language list, let's just make that the default for the method on the base plugin class.

Also updated the summery.

Gábor Hojtsy’s picture

@jhodgdon: sounds like we could actually add tests for the dynamic options using dynamic alterations of the frontpage view or an equivalent? :) Eg. display nodes in site default, interface language, etc. and check which nodes appear. We have an existing test or two for it with test nodes, so we would only need to modify the language condition to test.

jhodgdon’s picture

The problem isn't the UI for the language filter plugin, which this patch fixes. The problem is in the hook_views_data() or Views data controller equivalent, which says "here's a database field that should be filterable using the LanguageFilter plugin". Each of the translatable core entities with Views integration (Node, Comment, Taxonomy, User) has an issue in the language part of the views data. Until we fix those issues, writing a comprehensive test that checks whether the language filter is working properly with and without the Language module installed is problematic.

Node, for instance, doesn't add the langcode database column with Language filter to its views data, currently, unless the Language module is enabled. Comment and User don't even have a filter on translations (only on original language). I guess we could use Taxonomy... its only problem is that it doesn't have the "original language" filter. But I think it would be better/clearer/easier to write a test on Node?

Gábor Hojtsy’s picture

My point is node already has a language filter for original language and translation language that we can base tests off of. I believe that would be "more comprehensive" testing on site default language (that changed name in this patch) and interface language (that was added in this patch), than not doing any of those (ie. no tests). We could always extend on that test coverage elsewhere. Since @alexpott is missing tests, I think its easier to defend a position where we add some tests with the caveat that other issues are blocking adding more vs. where we add none. I think its worse if we need to postpone this on the other issues so we can add even more tests. :/

dawehner’s picture

  1. +++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_tid_field.yml
    --- a/core/modules/views/src/Plugin/views/PluginBase.php
    +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    
    +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -44,6 +45,13 @@
     
       /**
    +   * Include negotiated languages when listing languages.
    +   *
    +   * @see \Drupal\views\Plugin\views\PluginBase::listLanguages()
    +   */
    +  const INCLUDE_NEGOTIATED = 16;
    +
    
    @@ -439,4 +447,79 @@ public function getDependencies() {
    +  public static function queryLanguageSubstitutions() {
    

    What is the reason that this is part of the PluginBase class, would you ever want to extend the behaviour?

  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -439,4 +447,79 @@ public function getDependencies() {
    +  function listLanguages($flags = LanguageInterface::STATE_ALL) {
    

    is this a public or protected method?

  3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -439,4 +447,79 @@ public function getDependencies() {
    +
    +    // Handle negotiated languages.
    +    $types = $manager->getDefinedLanguageTypesInfo();
    +    foreach ($types as $id => $type) {
    +      if (isset($type['name'])) {
    +        $changes[$id] = $manager->getCurrentLanguage($id)->id;
    +      }
    +    }
    
    +++ b/core/modules/views/views.api.php
    @@ -569,8 +569,8 @@ function hook_views_query_substitutions(ViewExecutable $view) {
    -    '***CURRENT_LANGUAGE***' => \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->id,
    -    '***DEFAULT_LANGUAGE***' => \Drupal::languageManager()->getDefaultLanguage()->id,
    +    LanguageInterface::TYPE_CONTENT => \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->id,
    

    I kinda dislike the fact that we no longer have a special prefix/suffix for these placeholders. They help to understand that they are not just strings but are replaced during runtime.

jhodgdon’s picture

Status: Needs review » Needs work

RE #53 - OK. Yeah, with Node we can at least test with Language installed.

RE #54 -

item 1: I put the queryLanguageSubstitutions function on the PluginBase class because the code generating the options is also on that class. It seemed like a good idea to keep them together... where do you think this should be?

item 2: should be protected, I guess -- forgot that. It's for use in plugins, no compelling reason to make it public.

item 3: The Language module is already not putting a prefix on 'site_default', and we get that directly from them. It didn't make sense to me to put prefixes on the others. And... umm... who is seeing these that needs to understand what they are? The machine names are only exposed in the Views configs, right? They don't look like languages anyway, because they're things like 'language_content'... and putting arbitrary prefixes on them makes them harder to deal with in the code. So Sweetchuck's patch had removed the prefixes, and I thought it was cleaner to do that as well.

So... obviously we aren't going to put this patch in if dawehner objects to it. We need to come to an agreement on points 1 and 3, and then I'll make a patch to fix point 2 plus whatever other changes we need for points 1 nad 3, and add some tests for Node with Language module enabled.

jhodgdon’s picture

Talked with dawehner in IRC today... I have come around to agree with putting *** on the placeholders:
- We want queries to be cacheable, so we want to put the fake language codes into the queries themeselves.
- So we need to use global query substitution to make them into real languages before the queries are run.
- So we want them to be as obscure as possible so it is less likely they would possibly match anything else in a query.
===> wrap them in *** ... *** so that they are extremely unlikely to be matched. This also makes them similar to the other Views query substitutions.

So I'll make another patch sometime soon...

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
35.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,223 pass(es), 2 fail(s), and 371 exception(s). View
15.92 KB

OK, here's a new patch.

As a note, I was thinking of trying to move the static query substitution method out of DisplayPluginBase, but then I encountered that the Field module's views Field plugin class is using this method... so I decided I should leave it there:

core/modules/field/src/Plugin/views/field/Field.php

+        $langcode = $this->view->display_handler->options['field_langcode'];
+        $substitutions = static::queryLanguageSubstitutions();
+        if (isset($substitutions[$langcode])) {
+          $langcode = $substitutions[$langcode];
+        }

I'd really rather avoid having this plugin call out to get the language substitutions in some obscure Views include file. So I hope that's OK to leave the static method where it is.

Meanwhile, this patch changes the machine names of the site default and negotiated/selected languages to be wrapped in ***LANGUAGE_ ... *** so that there is nearly zero chance they will be mistaken for anything else in the query, as requested by dawehner.

And I've added tests. There were already tests in the NodeLanguageTest class testing filtering node views by specific languages. There were also tests in that same class using the front page view with different URL language prefixes, so it was testing the filter for the content selected/negotiated language.

So following the suggestion by Gabor above, I modified the front page view config to also test the site default language and the UI language.

So at least we have tests showing this works OK with the Language module enabled, for language filtering on nodes, when the views config is set programmatically to the special codes.

Hope that is sufficient to address all the concerns... here's a new patch.

Status: Needs review » Needs work

The last submitted patch, 57: 2037979-language-options-views-57.patch, failed testing.

Gábor Hojtsy’s picture

  1. +++ b/core/modules/node/config/install/views.view.frontpage.yml
    @@ -130,7 +130,7 @@ display:
    -            '***CURRENT_LANGUAGE***': '***CURRENT_LANGUAGE***'
    +            ***LANGUAGE_language_content***: '***LANGUAGE_language_content***'
    

    Should this not have a quote still?

  2. +++ b/core/modules/views_ui/src/Tests/ViewEditTest.php
    @@ -106,7 +106,7 @@ public function testEditFormOtherOptions() {
    -    $this->assertFieldByName('field_langcode', '***CURRENT_LANGUAGE***');
    +    $this->assertFieldByName('field_langcode', 'language_content');
    

    Was not updated to assert the right thing?

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
36.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,265 pass(es). View
3.74 KB

Right! Looked very very carefully through the previous patch and found a couple more spots... hopefully this will make the tests pass. It's odd -- the NodeLanguage test (the one I added to in the previous patch) passed tests locally even though it was using the frontpage view. ??!? Anyway.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -81,6 +81,31 @@ public function getLanguageTypes() {
    +    // This needs to have the same return value as
    +    // language_language_type_info(), so that even if the Language module is
    +    // not defined, users of this information, such as the Views module, can
    +    // access names and descriptions of the default language types.
    +    return array(
    

    It seems to be that the difference is having fixed. Could the other method use this method to retrieve the basic information?

  2. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
    @@ -81,6 +81,31 @@ public function getLanguageTypes() {
    +        'name' => t('User interface text'),
    +        'description' => t('Order of language detection methods for user interface text. If a translation of user interface text is available in the detected language, it will be displayed.'),
    ...
    +        'name' => t('Content'),
    +        'description' => t('Order of language detection methods for content. If a version of content is available in the detected language, it will be displayed.'),
    

    You can directly use $this->t() if you like.

  3. +++ b/core/modules/comment/config/install/views.view.comments_recent.yml
    @@ -228,7 +228,7 @@ display:
    -      field_langcode: '***CURRENT_LANGUAGE***'
    +      field_langcode: '***LANGUAGE_language_content***'
    

    Thank you for taking into account my feedback.

  4. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -439,4 +447,86 @@ public function getDependencies() {
    +  protected function listLanguages($flags = LanguageInterface::STATE_ALL) {
    ...
    +  public static function queryLanguageSubstitutions() {
    
    +++ b/core/modules/views/src/Plugin/views/filter/LanguageFilter.php
    @@ -16,16 +19,13 @@
    +      $this->value_options = $this->listLanguages(LanguageInterface::STATE_ALL |LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED);
    

    It would be kind of cool to have these methods somehow be extract into a trait, so it is not needed for every handler.

  5. +++ b/core/modules/views/views.api.php
    @@ -557,20 +557,26 @@ function hook_field_views_data_views_data_alter(array &$data, \Drupal\field\Fiel
    + *   surrounded with '***', as illustrated in the example implementation, to
    + *   avoid collisions with other values in the query.
    

    nice!

Gábor Hojtsy’s picture

@dawehner: Indeed, we did not yet discuss it being invoked from language_language_type_info(), but that would require we call this method statically on this concrete language manager since the language manager may be swapped. Not sure if a static method can be overridden with a non-static method in a subclass?

jhodgdon’s picture

FileSize
36.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,604 pass(es). View
1.42 KB

Yeah, #61-1/#62 - The other method is returning hook implementation collections... it's problematic no matter what. I think it's best to leave it as it is. At least the current state is consistent within Core; if someone installs a module that has a hook/alter implementation that changes the language codes, Core views will break... but at least as things are now, Core views will work consistently with Language installed or not. But I don't think we should have the hook/function linked directly.

RE #61-2 -- oops, yeah should do $this->t().

RE #61-4 -- the static method needs to be on an actual class, as the method needs to be called from the views hook for query substitutions... separating it from the constant and the language list method seems undesirable to me too.... I guess we could do the static call on one of the specific handlers that uses the trait, but that's a bit weird in the query substitution function... would prefer to keep it as is?

Anyway, I edited the patch file to change t() to $this->t() in LanguageManager...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I really like the better unification done by this issue.

Gábor Hojtsy’s picture

Yay, thanks for all the feedback!

jhodgdon’s picture

Thanks both of you for all the careful reviews! I was just going to post a summary of what might possibly still need to be done; glad the list is "nothing".

We should have a follow-up to fix the site default language constant though... #2328573: 'site_default' needs to be a language constant

jhodgdon’s picture

Issue summary: View changes

I also created two separate draft change notices for this:
https://www.drupal.org/node/2328585
https://www.drupal.org/node/2328581

Gábor Hojtsy’s picture

jhodgdon’s picture

Note that if #2323899: Provided default Node views need language filtering lands before this, we will need to apply the special language code changes from this issue to the updated default node views from that issue (they will have the CURRENT_LANGUAGE code in them).

Gábor Hojtsy’s picture

Priority: Normal » Major
webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks harder to re-roll than #2323899: Provided default Node views need language filtering so went with this one first.

Apart from ***MIXED_case_constants*** freaking me out a lot :P this looks fine. Definitely a lot less confusing than before. But since dawehner marked it RTBC, presumably they did not throw him. And I see why we do this, since it allows us to use the machine name directly.

Nothing else much to complain about really, so...

Committed and pushed to 8.x. Thanks!

  • webchick committed 6f9fc44 on 8.0.x
    Issue #2037979 by Gábor Hojtsy, Sweetchuck, jhodgdon, dawehner: Fixed '...
Gábor Hojtsy’s picture

Thanks a lot! Published the change records.

dawehner’s picture

Note: Most change records for views are actually filled against views 8.x-3.x at the moment, as we thought that this is the place where people will look at.

jhodgdon’s picture

dawehner... interesting about the Views change records... not sure where people will look, really.

Gábor Hojtsy’s picture

Issue tags: -sprint

This is superb. Thanks a lot!

Status: Fixed » Closed (fixed)

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