Problem/Motivation

When creating a node view with both translatable and non translatable fields, the view filter on language adds a join condition on the langcode of all fields (even those that have not been translated). This causes no results to be found in the languages that are other than the default language the non translated field were saved in.

For the same reason sorting does not work on untranslated fields. Things are sorted as expected in on language, but not in another.

Proposed resolution

Before applying the join to langcode on the fields, check whether the field is translatable.

Remaining tasks

Add the watchdog error mentioned in #193.
Open a followup issue to Improve the query. See #157-#158

User interface changes

None.

API changes

None, I guess.

Original report by @vitalie

Files: 

Comments

plach’s picture

Upchuk’s picture

FileSize
7.86 KB

Steps to reproduce:

  • Enable the content translation and languages module.
  • Add a new language (FR)
  • Edit the article content type and add a new text field called test
  • Edit the content translation settings and make the article nodes translatable (by default EN). The title field is the only field that is translatable (field_test should not be)
  • Create a single article node filling in both the title and the test field. Create a translation for the node with a different title.
  • Then import the attached view and run the preview to see the query.

If you are in the default site language (EN), you should see the node you created with the title and test field value. But if you switch languages in the URL, the view won't find anything because of the join I mentioned in the issue summary. What it should do is show the title in FR with the test field value that is for all languages.

dawehner’s picture

Issue tags: +VDC

/

dawehner’s picture

Priority: Normal » Major

... its pretty major, honestly

plach’s picture

Issue tags: +D8MI, +language-content, +sprint

Major, at least. Testing this right now.

plach’s picture

@Upchuck:

Ok, the issue is pretty clear. The main problem I see to fix it is that a view may be listing more than one bundle, hence the same field might be translatable in some cases and not translatable in others.

My proposal is to adapt the current code to do the following:

  • if the field is translatable for every bundle, we keep the current behavior;
  • if the field is untranslatable for every bundle, we skip the langcode condition;
  • if the field is translatable for some bundles but not for others, we use a more complex (and possibly less performant) join, taking bundles where the field is not translatable into account, eg:
    LEFT JOIN node__field_test node__field_test ON
      node_field_data.nid = node__field_test.entity_id AND
      node__field_test.deleted = '0' AND (
        node__field_test.langcode = node_field_data.langcode OR
        node__field_test.bundle IN ('article')
      )
    
Upchuk’s picture

Hey @plach.

Thanks for the input. Working on a patch.

jhodgdon’s picture

Are we talking about a views filter here, or the formatter? I'm confused about what is going wrong where.

Upchuk’s picture

This is it the level of the join caused by adding a views filter on the respective field (the questionably translated field) :)

jhodgdon’s picture

Reread the issue summary. It is a good issue summary. Sorry for getting confused. ;)

dawehner’s picture

FileSize
3.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,768 pass(es), 1 fail(s), and 1 exception(s). View

@Upchuk asked me to write a test, here is not even a proper start yet.

Upchuk’s picture

Status: Active » Needs review

Let's see what the bot says about it though.

Status: Needs review » Needs work

The last submitted patch, 11: 2451657-11-test.patch, failed testing.

Upchuk’s picture

Status: Needs work » Needs review
FileSize
10.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,120 pass(es). View

Alright, so here we have a start for a patch that we can work on.

I think I found a bug though related to the join build that happens inside JoinPluginBase::buildJoin():

if (is_array($info['value'])) {
  // With an array of values, we need multiple placeholders and the
  // 'IN' operator is implicit.
  $local_arguments = array();
  foreach ($info['value'] as $value) {
    $placeholder_i = ':views_join_condition_' . $select_query->nextPlaceholder();
    $local_arguments[$placeholder_i] = $value;
  }

  $operator = !empty($info['operator']) ? $info['operator'] : 'IN';
  $placeholder = '( ' . implode(', ', array_keys($local_arguments)) . ' )';
  $arguments += $local_arguments;
}
else {
  // With a single value, the '=' operator is implicit.
  $operator = !empty($info['operator']) ? $info['operator'] : '=';
  $placeholder = ':views_join_condition_' . $select_query->nextPlaceholder();
}
// Set 'field' as join table field if available or set 'left field' as
// join table field is not set.
if (isset($info['field'])) {
  $join_table_field = "$join_table$info[field]";
  // Allow the value to be set either with the 'value' element or
  // with 'left_field'.
  if (isset($info['left_field'])) {
    $placeholder = "$left[alias].$info[left_field]";
  }
  else {
    $arguments[$placeholder] = $info['value'];
  }
}

The problem happens when inside the extra key we add a join condition with a value as an array. In that case, $arguments += $local_arguments; correctly hydrates the $arguments array with the relevant argument => value. However, if you read down, there is a check if the 'field' key is contained. In this block, the absence of a 'left_field' (which due to the presence of the 'value' key I think is logical) the placeholder gets added again to the arguments array ('$placeholder' remaining set from the previous block). And down the line this causes an error in the Connection::expandArguments() because it misses brackets at the end of the placeholder AND that you have too many placeholders.

huu, I hope this is clear :) I'm not sure if this is 100% a bug, hence I didn't open an issue. But if it is, we need to fix it before we can fix the current issue. In the patch, I manually remove the array value argument from the main $arguments array to fix this:

foreach ($arguments as $key => $arg) {
  if (is_array($arg)) {
    unset($arguments[$key]);
  }
}
dawehner’s picture

The reason why i'm a bit hesitate here is, that the implementation of the child class looks really similar to the parent class.
I'm wondering whether we could change the parent class in a way, so subclassing is easy.

Upchuk’s picture

Yeah, it's almost the same. But I think it's outside the scope of this issue no? And do you agree about the bug I found or that is how it should work? :)

dawehner’s picture

Yeah, it's almost the same. But I think it's outside the scope of this issue no?

I think it would be a bad idea to get a patch in, which makes core more complex to maintain.

Upchuk’s picture

FileSize
8.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,581 pass(es), 0 fail(s), and 2 exception(s). View

So here's a patch responsible for two things:

1 Refactor a bit the buildJoin() method of the JoinPluginBase class so that it can be more easily extended
2 Fix a bug I encountered and discussed with @dawehner on ICR related to a mismatch in the number of placeholders and condition values (In the next comment I will zoom into the small fix for this)

Regarding the refactoring, I'm not so sure. It's done through some logic delegation to other methods but a combination of returning data and updating it through reference. The $arguments array is being built has to always be passed by reference. If you have any better alternative, please!! :)

Upchuk’s picture

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -267,96 +267,149 @@ public function buildJoin($select_query, $table, $view_query) {
+        // We are adding this placeholder only if the value is not an array
+        // because the individual placeholders got added earlier through the
+        // local arguments already.
+        if (!is_array($extra['value'])) {
+          $arguments[$placeholder] = $extra['value'];
+        }

This is the bug fix I mentioned in the previous comment. It prevents the placeholder for the array of values to be added again (since it's already added earlier via the local arguments)

Status: Needs review » Needs work

The last submitted patch, 18: 2451657-18.patch, failed testing.

Upchuk’s picture

Status: Needs work » Needs review
FileSize
8.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,453 pass(es). View

Let's try this again.

dawehner’s picture

I really like the split you made:

a) build a single extra condition
b) build the join string out of the multiple extra conditions.

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -267,96 +268,149 @@ public function buildJoin($select_query, $table, $view_query) {
+  protected function joinAddExtra(&$arguments, &$condition, $table, $left = NULL, $select_query) {

we could improve the parameter name in a follow up from $left to $left_table for example. ... I'm curious, can we add a typehint for the select query? On top of that, $left is not really optional if there is a parameter after it, which isn't.

Upchuk’s picture

FileSize
9.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,455 pass(es). View

Hey @dawehner, thanks for the review.

  • I changed the $left to $left_table across the board (hopefully) :)
  • I type hinted the select_query (let's hope it works)
  • And dooh, optional variable cannot be where I put it (so I moved it to the end)

Sorry for no interdiff...

I guess we can move on with the actual fix for this issue no?

dawehner’s picture

I guess we can move on with the actual fix for this issue no?

+1

Gábor Hojtsy’s picture

@Upchuk: still working on this one?

Upchuk’s picture

Hey Gabor,

Yes, I am. A lot of the work is already done and can be found in the previous patch. I just need to re-apply the work on top of these changes we did in the last comments.

I've just been a bit swamped these days.

D

Upchuk’s picture

FileSize
6.73 KB
16.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,185 pass(es). View

So here we go. The interdiff reflects changes from #23.

plach’s picture

Overall looks good to me, however I'm not sure checking for field translatability in the views data handler is correct. I realize it makes things more performant, but I'm wondering whether we can ensure data is rebuilt every time field translatability changes.

@dawehner:

Would it be possible/make sense to do this at runtime? Or, does the current approach look correct to you? Also, you were mentioning test coverage might not be complete, right?

Quick code review:

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -267,96 +269,149 @@ public function buildJoin($select_query, $table, $view_query) {
+    // Figure out the table name. Remember, only use aliases provided
+    // if at all possible.
...
+      // If we're aware of a table alias for this table, use the table
+      // alias instead of the table name.
...
+    // Convert a single-valued array of values to the single-value case,
+    // and transform from IN() notation to = notation
...
+      // With an array of values, we need multiple placeholders and the
+      // 'IN' operator is implicit.
...
+    // Set 'field' as join table field if available or set 'left field' as
+    // join table field is not set.

+++ b/core/modules/views/views.views.inc
@@ -338,6 +339,38 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
+  // If the field is translatable on all the bundles, there will be a join on the langcode.
...
+  // If the field is translatable only on certain bundles, there will be a join on langcode OR bundle name.

Comments are not wrapping at column 80 correctly.

Upchuk’s picture

Hey @plach,

Daim, you are right. If you change field translatability without cache clear the changes won't be picked up. Where could we then handle this? In the join plugin? That seems to be called all the time.

plach’s picture

As we discussed, we should add a subscriber for ConfigEvents::SAVE and ConfigEvents::IMPORT to detect translatability changes for the field.field and core.base_field_override config objects, and invalidate views data cache via the views_data cache tag.

dawehner’s picture

FileSize
12.94 KB
12.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,353 pass(es), 3 fail(s), and 0 exception(s). View
+++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
index 051955e..fdcc37f 100644
--- a/core/modules/views/src/Plugin/views/join/JoinPluginBase.php

--- a/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -7,6 +7,7 @@

I really like the extracting into the submethods!

Here is the promised test ... one has a odd result.

Upchuk’s picture

FileSize
14.71 KB
20.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2451657-32.patch. Unable to apply patch. See the log in the details link for more information. View

The interdiff is from #27 and the patch contains also the test in #31. In #31 it should fail and hopefully here it would pass.

+++ b/core/modules/views/src/Tests/FieldApiDataTest.php
@@ -43,7 +155,7 @@ protected function setUp() {
-  function testViewsData() {
+  function ptestViewsData() {

I'm guessing that was forgotten there :)

Upchuk’s picture

@plach,

I forgot to mention in the previous comment. I had started implementing the listener but then i realized that whenever the language settings are saved at admin/config/regional/content-language, the views data gets cleared already using a hook (views_field_config_update()). So there is no more need to add the listener. Also works when you import a field config.

plach’s picture

Translatability may be changed also by programmatically saving a config field definition/override, we need to account for that, I think.

Upchuk’s picture

Well, I suppose that programatically saving field configs would trigger the hook.

Status: Needs review » Needs work

The last submitted patch, 32: 2451657-32.patch, failed testing.

The last submitted patch, 31: 2451657-31.patch, failed testing.

plach’s picture

Yes, but that works only with configurable fields, we need to do the same for the base_field_override config entity.

YesCT’s picture

Assigned: Upchuk » Unassigned

I talked to @Upchuk in IRC. They will not be able to get to this today, so unassigning.

Looks like the next thing is to look at the tests.... ? Anyway, someone can jump on this if they like. And @Upchuk can rejoin when available. :)

Upchuk’s picture

A couple of things are left to do here I think:

  1. #38 needs to be looked into in terms of the actual fix (see if the views data cache gets cleared when programatically updating base_field_override entities)
  2. The test needs to be finalized

Maybe also a reroll might be necessary since there's been so much progress during the sprint :)

I might check in tomorrow for 1.

Upchuk’s picture

Assigned: Unassigned » Upchuk

Looking into this a bit.

Upchuk’s picture

FileSize
29.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,566 pass(es). View

I think the patch in #32 was broken a bit...didn't apply. This one should work.

skyredwang’s picture

Status: Needs work » Needs review

Update the status, since there is a new patch in #42

Upchuk’s picture

Assigned: Upchuk » Unassigned
FileSize
1.69 KB
30.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,433 pass(es). View

So here we go.

The interdiff contains the work needed to address what @plach mentioned in #38 and that is basically the only change from #42.

plach’s picture

Status: Needs review » Needs work

Great work, we are almost there!

  1. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,59 @@
    +          $condition .= ' AND ' . array_shift($extras);
    +        }
    +        else {
    +          $condition .= ' AND (' . implode(' ' . $this->extraOperator . ' ', $extras) . ')';
    +        }
    +
    +        // Tack on the langcode OR bundle join condition extra.
    +        if (!empty($language_bundle_conditions)) {
    +          $condition .= ' AND (' . implode(' OR ', $language_bundle_conditions) . ')';
    +        }
    +      }
    +    }
    +    elseif ($this->extra && is_string($this->extra)) {
    +      $condition .= " AND ($this->extra)";
    +    }
    

    I know similar code is already existing in HEAD but I'm wondering whether we should escape variables to avoid SQL injections or whether we are sure those are safe.

  2. +++ b/core/modules/views/src/Tests/FieldApiDataTest.php
    @@ -43,7 +155,7 @@ protected function setUp() {
    +  function ptestViewsData() {
    

    hm

  3. +++ b/core/modules/views/src/Tests/FieldApiDataTest.php
    @@ -88,4 +200,119 @@ function testViewsData() {
    +      // Why is this one returned?
    

    Mmmh, why? :)

  4. +++ b/core/modules/views/views.views.inc
    @@ -338,6 +339,38 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
    +  $bundles_names = $field_storage->getBundles();
    +  $translation_join_type = FALSE;
    +  $field_configs = [];
    +  $translatable_configs = [];
    +  $untranslatable_configs = [];
    +  $untranslatable_config_bundles = [];
    +
    +  foreach ($bundles_names as $bundle) {
    +    $field_configs[$bundle] = FieldConfig::loadByName($entity_type->id(), $bundle, $field_name);
    +  }
    +  foreach ($field_configs as $bundle => $config) {
    +    if ($config->isTranslatable()) {
    +      $translatable_configs[$bundle] = $config;
    +    }
    +    else {
    +      $untranslatable_configs[$bundle] = $config;
    +    }
    +  }
    +
    

    This code should use the entity manager to retrieve and process all field definitions, not just configurable fields. We can use the table mapping (see SqlContentEntityStorage::getTableMapping()) to determine which ones are stored in dedicated tables.

  5. +++ b/core/modules/views/views.views.inc
    @@ -338,6 +339,38 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
    +  // If the field is translatable on all the bundles, there will be a join on the langcode.
    ...
    +  // If the field is translatable only on certain bundles, there will be a join on langcode OR bundle name.
    

    Comments do not wrap at column 80.

  6. +++ b/core/modules/views/views.module
    @@ -459,23 +459,32 @@ function views_add_contextual_links(&$render_element, $location, ViewExecutable
    + * Implements hook_entity_create().
    

    This hook is invoked when instantiating a new entity object, I think we need hook_entity_insert() instead.

  7. +++ b/core/modules/views/views.module
    @@ -459,23 +459,32 @@ function views_add_contextual_links(&$render_element, $location, ViewExecutable
    +  if ($entity->getEntityTypeId() !== 'field_config' && $entity->getEntityTypeId() !== 'base_field_override') {
    

    Given how much often these would be called, perhaps we should go back to the entity type specific form and implement it also for base field overrides.

Upchuk’s picture

Hey @plach,

Thanks for the feedback. I will defer to @dawehner for 1,2 and 3 (though 2 is just a typo). And maybe he can also consult on the rest :)

As for the rest:

4. A yes. Just realize I am only loading field configs and not base_field_overrides as well. Though I will bug you for a minute on IRC for some details.
5. Hm...thought I checked all the comments. Will take another look :)
6. I think you are right...though the previous version was using create as well. Maybe @dawehner can also chime in.
7. I used the more generic variants to avoid too much repetition there. But if it becomes unperformant, we can move to the entity type specific versions. But does it become unperformant?

Cheers!

plach’s picture

I used the more generic variants to avoid too much repetition there. But if it becomes unperformant, we can move to the entity type specific versions. But does it become unperformant?

Well, the repetition is one API method invocation, so I will turn the question. Is it repetition or just correct API usage? ;)

Upchuk’s picture

Well, the repetition is one API method invocation, so I will turn the question. Is it repetition or just correct API usage? ;)

Well, it's not an incorrect usage of the API :) The repetition I was talking about was the call to clear the Views data cache that currently is present in 3 functions and would have to now go into 3 more. I am really not sure of what the policy is here. I had talked to @dawehner about this and he said he prefers the more generic solution.

For me it's ok one way or the other.

D

plach’s picture

Fine for me, personally I'd prefer to call the same API method (cache invalidation) 6 times but doing it only when needed. That's the correct API usage (and not repetition) I was referring to :)

Btw, if we keep the current approach, we should have a separate function called from the three hook implementations to avoid repeating the if logic.

plach’s picture

@upchuck:

What's the current status? Who is supposed to address my review?

Upchuk’s picture

Hey @plach,

Both me and dawehner. But I was away for more than a week so I couldn't do anything..Will pick up your feedback soon. Testing side is fully dawehner though.

Cheers!

plach’s picture

upchuck++ :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,884 pass(es), 10 fail(s), and 0 exception(s). View

upchuck asked me to reroll the patch. Alright, let's give it a try. This does not address comment #45 yet.

Status: Needs review » Needs work

The last submitted patch, 53: 2451657-53.patch, failed testing.

jibran’s picture

This is an api addition so we have to update IS. As it is an API addition we also need a change notice as well.

  1. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,59 @@
    +
    

    Extra space not needed.

  2. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,59 @@
    +          if (strpos($extra, '.langcode') !== FALSE || strpos($extra, '.bundle') !== FALSE ) {
    

    Can we add a comment to explain this?

  3. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -267,96 +269,149 @@ public function buildJoin($select_query, $table, $view_query) {
    +   * @param $arguments
    ...
    +   * @param $condition
    ...
    +   * @param $table
    ...
    +   * @param $select_query
    ...
    +   * @param $left_table
    ...
    +   * @param $extra
    ...
    +   * @param $arguments
    ...
    +   * @param $table
    ...
    +   * @param $select_query
    ...
    +   * @param $left_table
    

    @param type missing.

  4. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -267,96 +269,149 @@ public function buildJoin($select_query, $table, $view_query) {
    +   *
    ...
    +   *
    ...
    +   *
    ...
    +   *
    ...
    +  protected function joinAddExtra(&$arguments, &$condition, $table, SelectInterface $select_query, $left_table = NULL) {
    ...
    +   *
    ...
    +   *
    ...
    +   *
    ...
    +   *
    

    Extra space not needed.

  5. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -267,96 +269,149 @@ public function buildJoin($select_query, $table, $view_query) {
    +    // Figure out the table name. Remember, only use aliases provided
    +    // if at all possible.
    ...
    +      // If we're aware of a table alias for this table, use the table
    +      // alias instead of the table name.
    ...
    +    // Convert a single-valued array of values to the single-value case,
    +    // and transform from IN() notation to = notation
    ...
    +      // With an array of values, we need multiple placeholders and the
    +      // 'IN' operator is implicit.
    ...
    +    // Set 'field' as join table field if available or set 'left field' as
    +    // join table field is not set.
    ...
    +      // Allow the value to be set either with the 'value' element or
    +      // with 'left_field'.
    

    Can we please extend these comments to 80 char limit?

  6. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,59 @@
    + * Implementation for the field_or_language join.
    

    Can we please expand this how the join would actually look like?

Upchuk’s picture

FileSize
3.02 KB
30.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2451657-56.patch. Unable to apply patch. See the log in the details link for more information. View

@plach,

I addressed some of the easier stuff in #45. Regarding the "repetition" point, I left it like this for now and extracted the logic to a function. But do let me know if you insist on making it into separate hook implementations and I'll do it :)

Regarding point 4, Upon a further look, I'm not sure I understand why the base field override fields need to be loaded and processed. The `views_field_default_views_data()` function only seems to get passed field_config entity storage objects and no base_field_override ones. Maybe you can take a look?

@jibran,
Will address your issues as well shortly, didn't have a lot of time now.

Upchuk’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 56: 2451657-56.patch, failed testing.

plach’s picture

Regarding the "repetition" point, I left it like this for now and extracted the logic to a function. But do let me know if you insist on making it into separate hook implementations and I'll do it :)

Yep, sorry, I think it would be better to go with views_entity_field_config_[insert|update|delete] and views_entity_base_field_override_[insert|update|delete]: it will highly reduce the amount of times those hooks are invoked. The separate function to encapsulate the logic looks fine, though.

Regarding point 4, Upon a further look, I'm not sure I understand why the base field override fields need to be loaded and processed.

Because also base multiple fields are stored in dedicated tables. We should not assume an implementation, we should rely on the API to tell us which field definitions we have to deal with. However it seems all the views data code is acting only on configurable fields, so let's ignore this for now :(

  1. +++ b/core/modules/views/views.module
    @@ -468,29 +468,31 @@ function views_add_contextual_links(&$render_element, $location, ViewExecutable
    + * Invalidates field data for 'field_config' and 'base_field_override' entities.
    + * @param \Drupal\Core\Entity\EntityInterface $entity
    

    PHP doc is not standard compliant

  2. +++ b/core/modules/views/views.views.inc
    @@ -359,11 +359,13 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
    +  // If the field is translatable only on certain bundles, there will be a
    +  // join on langcode OR bundle name.
    

    "join" should go in the previous line.

Upchuk’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
30.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2451657-60.patch. Unable to apply patch. See the log in the details link for more information. View

Hey @plach

Quickly addressed your two points in #59:

1. Done! However, removed the separate function because now the logic is really only one liner so no more need.
2. Fixed

Regarding the field_config vs base_field_override issue: indeed, only field_config are taken into account by views here. So leaving as it is for now.

Still remains #55 to address.

@dawehner, think you can see what's going wrong with the tests?

Status: Needs review » Needs work

The last submitted patch, 60: 2451657-60.patch, failed testing.

plach’s picture

Issue tags: +Needs reroll
deepakaryan1988’s picture

Assigned: Unassigned » deepakaryan1988
Status: Needs work » Active
deepakaryan1988’s picture

Assigned: deepakaryan1988 » Unassigned
Status: Active » Needs review
FileSize
2.28 KB
23.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/join/JoinPluginBase.php. View

Re-rolling the patch which is in #60

Status: Needs review » Needs work
Upchuk’s picture

Thanks for the reroll.

+++ /dev/null
@@ -1,59 +0,0 @@
-class FieldOrLanguageJoin extends JoinPluginBase {

What happened to this class?

Gábor Hojtsy’s picture

Issue tags: -sprint

While I think this should ideally be fixed, removing from sprint due to lack of activity in over a month.

deepakaryan1988’s picture

@Upchuk I will take look on this upcoming Friday.
We are having internal code sprint. :)
Sorry for the delay.

Sharique’s picture

Status: Needs work » Needs review
FileSize
23.53 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,257 pass(es), 105 fail(s), and 330 exception(s). View
451 bytes

Fixing syntax error from #64.

Sharique’s picture

FileSize
25.5 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,394 pass(es), 108 fail(s), and 346 exception(s). View
2.25 KB

Adding missing class FieldOrLanguageJoin.

Status: Needs review » Needs work

Upchuk’s picture

Status: Needs work » Needs review
FileSize
30.35 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,637 pass(es), 10 fail(s), and 0 exception(s). View

Rerolling from #60 because other rerolls seem to be missing data. Hopefully this is ok..

Status: Needs review » Needs work

The last submitted patch, 75: 2451657-75.patch, failed testing.

googletorp’s picture

Issue tags: -Needs reroll
lokapujya’s picture

Status: Needs work » Needs review
FileSize
30.2 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,980 pass(es), 8 fail(s), and 0 exception(s). View
732 bytes

This code doesn't do anything. Plus, I think it's causing test failures.

For reference, getFieldLangcode is gone:
https://www.drupal.org/node/1867518#comment-9713165

Status: Needs review » Needs work

The last submitted patch, 78: 2451657-78.patch, failed testing.

lokapujya’s picture

That only fixed two of them. That exposed another issue; the code I removed originally did something (back in the last green patch), and needs to be properly merged with changes in HEAD.

dawehner’s picture

As written on IRC, can't we do something like $entity = $this->getEntityFieldRenderer()->getTranslation()

Upchuk’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,533 pass(es). View

@dawehner, I ran your test locally but it fails (no exception but assertion fails)...not sure what is going on...

Let's try to see if any other test fail without including your test here.

The patch contains the latest from #78 minus the test and test view.

Upchuk’s picture

FileSize
17.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,535 pass(es). View

Damn git..here is the patch :)

lokapujya’s picture

Upchuk, did you try #81? If so, can you post it?

Upchuk’s picture

@lokapujya, no, sorry. We dropped that attempt as discussed on IRC.

lokapujya’s picture

FileSize
30.55 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2451657-86.patch. Unable to apply patch. See the log in the details link for more information. View
1012 bytes

Something like this?

Upchuk’s picture

@dawehner, can you confirm #86?

Upchuk queued 86: 2451657-86.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 86: 2451657-86.patch, failed testing.

Upchuk’s picture

Status: Needs work » Needs review
Issue tags: +Barcelona2015
FileSize
30.55 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2451657-90.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a reroll off #86.

Status: Needs review » Needs work

The last submitted patch, 90: 2451657-90.patch, failed testing.

The last submitted patch, 90: 2451657-90.patch, failed testing.

Upchuk’s picture

Status: Needs work » Needs review
FileSize
30.55 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,908 pass(es), 1 fail(s), and 1 exception(s). View

Sorry, I uploaded the wrong patch. Hope this will apply :)

Status: Needs review » Needs work

The last submitted patch, 93: 2451657-92.patch, failed testing.

The last submitted patch, 93: 2451657-92.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.58 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,635 pass(es), 1 fail(s), and 1 exception(s). View

Rerolled, now working on the failure

dawehner’s picture

FileSize
31.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,645 pass(es). View
2.14 KB

There we go.

The last submitted patch, 96: 2451657-96.patch, failed testing.

The last submitted patch, 96: 2451657-96.patch, failed testing.

dawehner’s picture

Issue summary: View changes

Alright, so the tasks are resolved

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -999,8 +999,12 @@ protected function getTableMapping() {
+    // Retrieve the correct translation object.
+    $entity = $this->getEntity($values);
+    $processed_entity = clone $this->getEntityFieldRenderer()->getEntityTranslation($entity, $values);

A little bit more explanation would be helpful I think. I think everyone should assume at any point that we are doing the right thing :P

plach’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs change record
FileSize
5.53 KB
31.15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,743 pass(es). View

Looks great!

Performed some minor coding standard clean-up. I'm not sure this needs a change record.

dawehner’s picture

Yeah I don't believe either that this is a change record. Its almost like documenting that we have a new field formatter now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 101: 2451657-101.patch, failed testing.

The last submitted patch, 101: 2451657-101.patch, failed testing.

lokapujya’s picture

the order of the results changed, and shows 1 fail, but it's still green?

dawehner’s picture

Well, let's add a sort criteria to ensure the behaviour and not cause any maybe even random failures on the testbot.

alexpott’s picture

Status: Needs work » Needs review
FileSize
634 bytes

I thought this patch might fix the failures I'm seeing when you do this... it does not.

Status: Needs review » Needs work

The last submitted patch, 107: d8.locale-views.test-only.patch, failed testing.

dawehner’s picture

I thought this patch might fix the failures I'm seeing when you do this... it does not.

Yeah that totally doesn't make sense. locale is not about content translation, which is what this issue is about.

Upchuk’s picture

Here we go. Added a sort on language to the view to match the expected array. Hopefully this will fix it :) Did locally..

Upchuk’s picture

Status: Needs work » Needs review

The last submitted patch, 107: d8.locale-views.test-only.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Upchuk

The last submitted patch, 107: d8.locale-views.test-only.patch, failed testing.

Upchuk’s picture

Issue tags: +rc target triage
effulgentsia’s picture

Issue tags: -rc target triage +rc target

Discussed with @alexpott and Views incorrectly returning no results for a fairly common scenario like this one is a serious enough bug to justify fixing during RC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
    @@ -999,8 +999,12 @@ protected function getTableMapping() {
    +    $processed_entity = clone $this->getEntityFieldRenderer()->getEntityTranslation($entity, $values);
    

    Why is this close necessary? I can't see why. If it is it needs documenting.

  2. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,58 @@
    +/**
    + * Implementation for the field_or_language join.
    + *
    + * @ingroup views_join_handlers
    + *
    + * @ViewsJoin("field_or_language_join")
    + */
    

    This needs more documentation as to what it is doing.

  3. This is only a partial review but given the size of the changes we need to confirm we have adequate test coverage and have truly done the minimum change possible so we don't risk introducing critical regressions.
yobottehg’s picture

This patch does not longer apply since 8.0.2. But the bug is there in 8.0.2

lokapujya’s picture

Issue tags: +Needs reroll

Needs reroll according to #118.

AkshayKalose’s picture

Issue tags: -Needs reroll
FileSize
31.37 KB

Rerolled

gvso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 120: 2451657-120.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
lokapujya’s picture

rerolled. diffed the #120 patch and the files are identical.

The last submitted patch, 124: 2451657-124.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 124: 2451657-124.patch, failed testing.

lokapujya’s picture

PHP Fatal error: Cannot use Drupal\Core\Entity\EntityInterface as EntityInterface because the name is already in use in ./core/modules/views/views.module on line 21

lokapujya’s picture

Sharique’s picture

Status: Needs work » Needs review
holist’s picture

Assigned: Unassigned » holist
Status: Needs review » Needs work

Needs reroll after 8.0.3, I will also look into the comments by @alexpott in #117.

holist’s picture

I rerolled the patch, removed the dubious clone @alexpott mentioned (works fine for me, I didn't see the reason either), and improved the comments a bit but probably it needs more.

The patch seems to mostly do it's job, but I noticed one weirdness that definitely is a blocker: I have an integer field that is used in several bundles. I use the field for ordering rows in view. With the patch, the join plugin builds a join clause that OR's this field's bundle matching a different bundle than the one all the nodes in the current view belong to.

To make it more plain: I am ordering rows of nodes of bundle course on field_priority, but the join clause says node__field_priority.bundle = 'management_personnel'. field_priority is used on both course and management_personnel.

The view's SQL before the patch:

SELECT node.nid AS node_nid, node_field_data.langcode AS node_field_data_langcode, node__field_active.field_active_value AS node__field_active_field_active_value, node__field_priority.field_priority_value AS node__field_priority_field_priority_value, node_field_data.nid AS nid
FROM
{node_field_data} node_field_data
INNER JOIN {node} node ON node_field_data.nid = node.nid
LEFT JOIN {node__field_active} node__field_active ON node_field_data.nid = node__field_active.entity_id AND (node__field_active.deleted = '0' AND node__field_active.langcode = node_field_data.langcode)
LEFT JOIN {node__field_priority} node__field_priority ON node_field_data.nid = node__field_priority.entity_id AND (node__field_priority.deleted = '0' AND node__field_priority.langcode = node_field_data.langcode)
WHERE (( (node_field_data.status = '1') AND (node_field_data.langcode IN  ('fi')) AND (node_field_data.type IN  ('course')) ))
ORDER BY node__field_active_field_active_value DESC, node__field_priority_field_priority_value ASC

and after patching:

SELECT node.nid AS node_nid, node_field_data.langcode AS node_field_data_langcode, node__field_active.field_active_value AS node__field_active_field_active_value, node__field_priority.field_priority_value AS node__field_priority_field_priority_value, node_field_data.nid AS nid
FROM
{node_field_data} node_field_data
INNER JOIN {node} node ON node_field_data.nid = node.nid
LEFT JOIN {node__field_active} node__field_active ON node_field_data.nid = node__field_active.entity_id AND node__field_active.deleted = '0'
LEFT JOIN {node__field_priority} node__field_priority ON node_field_data.nid = node__field_priority.entity_id AND node__field_priority.deleted = '0' AND (node__field_priority.langcode = node_field_data.langcode OR node__field_priority.bundle = 'management_personnel')
WHERE (( (node_field_data.status = '1') AND (node_field_data.langcode IN  ('fi')) AND (node_field_data.type IN  ('course')) ))
ORDER BY node__field_active_field_active_value DESC, node__field_priority_field_priority_value ASC

And a small screen shot to illustrate the node__field_priority table: node__field_priority screenshot

Changing to "Needs work" for testbot, but really needs work.

holist’s picture

Assigned: holist » Unassigned
Status: Needs work » Needs review

Actually changing the status now. (And unassigning, can't put more time into this for now.)

lokapujya’s picture

FileSize
1.78 KB

Interdiff for 131, also showing the code affected by reroll.

lokapujya’s picture

I don't even have translation turned on. I added a field to two different bundles. I can reproduce the problem in #131. However, that makes no sense because that means that on one bundle the field is translatable and on the other it is not translatable. The "field OR language" join (introduced in this issue) only gets used when the field is translatable on certain bundles.

In summary, there is a bug ( I think) in Core, in that translatable is different for the 2 bundles. Also, there is an issue with this patch which we are trying to identify.

holist’s picture

Ok now I think I understand what is happening. Our site did actually have the field translatable on one bundle and not on the other, I missed this earlier. If I disable translatability on one bundle, the OR clause changes to (node__field_priority.langcode = node_field_data.langcode OR node__field_priority.bundle IN ( 'management_personnel', 'course' )). And vice versa, if both instances are translatable, no OR's with .bundle.

So works as designed, but does it work as expected? Could we somehow check which bundles we are dealing with here and add only those? If I understand correctly, checking the translatability happens at views_field_default_views_data(), but there it seems we don't have any knowledge of which bundles are being listed in the view. So to me, it seems the answer is no?

So if I got this right, I think this behaviour should be added to the documentation that it causes no further confusion. I can do the patch for that. Considering what @alexpott wrote in #117, we have good progress here but someone still needs to check the test coverage per his request (asking humbly someone who knows what is going on there).

lokapujya’s picture

Issue tags: +Needs tests

Will probably need a test to cover the joins described in #131-#135.

holist’s picture

Status: Needs review » Needs work
holist’s picture

holist’s picture

Status: Needs work » Needs review

Triggering testbot.

xjm’s picture

The core committers and Views maintainers (alexpott, xjm, effulgentsia, tim.plunkett, and dawehner) agreed that this is a major issue. It's not critical because there is no loss of data, but it is a severe bug that will cause incomplete (and therefore essentially broken) view results on many multilingual sites.

xjm’s picture

Issue tags: +Triaged D8 major
Xano’s picture

Version: 8.0.x-dev » 8.1.x-dev

Moving to 8.1.x since that's in beta now, and we must fix it there before we fix it in 8.0.x, or this would be a regression.

lokapujya’s picture

Tried looking at this again. If I create two content types.
1. Create a new integer field on the first one,
2. Reuse the field on the second content type.
3. Export the configuration of both fields

The first has:
translatable: true

The second has:
translatable: false

So, this new join (in this patch) will be used on a view of the only the first content type. See comment #6.
Opened: https://www.drupal.org/node/2689339 Edit: Since closed this issue. The second content type need to be configured to be translatable.

dawehner’s picture

@lokapujya So do you think this issue needs work?

lokapujya’s picture

Status: Needs review » Needs work

For the issue in #131.

the join plugin builds a join clause that OR's this field's bundle matching a different bundle than the one all the nodes in the current view belong to.

and because the new (more complex) join (introduced in this patch) gets used when it's not needed. If the view is only showing one content type then it doesn't need the complex join.

lokapujya’s picture

Closed the issue that I opened in #144. That in itself is not an issue. The 2nd content type just needed to be configured to be translatable. But that doesn't change the fact that this new join still has a problem.

xjm’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: -rc target

@Xano Filing an issue against 8.0.x does not mean it will not be fixed in 8.1.x; just that it is patch-safe as well. OTOH looking at the patch here I agree this is best targeted for a minor version. Therefore, I'm actually moving this to 8.2.x for now.

This was marked as an RC target for 8.0.0. It's possible that it might also be an RC target for 8.1.0 as well based on the impact, but we can evaluate that using the RC target triage process if the issue is actually ready during RC.

Thanks everyone for your continued work here!

holist’s picture

Ok, looked over this issue again. The issue in #131 is about bundle id being checked for if that bundle isn't translatable, because bundles not being looked at in the query are not filtered out. Strictly speaking, it is not good because the query gets more complexity, but without seeing an obvious way to filter the irrelevant bundles out, I lean towards to "it works" approach here. (Considering without the patch, it really does not work.)

Theoretically there could be some performance hit, but I suspect it won't be significant? Maybe profiling if it is a concern?

The latest patch contains a note that this is happening.

Tests though, they should be checked, anyone?

vasi’s picture

I agree that the extra query complexity is ok. It would be much more complex if each field join had to look through all the other query conditions, to figure out which bundles were allowed.

zolt_toth’s picture

The patch in #139 has solved our problem but it cannot be applied for the codebase of 8.1.1. Could someone maintain it?

lokapujya’s picture

Issue tags: +Needs reroll

Needs reroll for 8.2

ndewhurst’s picture

Assigned: Unassigned » ndewhurst

I was also able to reproduce this issue and will look at re-rolling #139...

Ashish.Dalvi’s picture

Hi ndewhurst, lokapujya,

I am able to apply patch on 8.2.x. No Reroll required.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Barcelona2015, -Needs tests, -Needs reroll

Back to needs review, since there are currently no actionable items to work on.

lokapujya’s picture

AND (
    node__field_test.langcode = node_field_data.langcode OR
    node__field_test.bundle IN ('article')
  )

Is this condition any different than not having the condition at all?

holist’s picture

On #157, @lokapujya, does the latter part of that OR clause result in any field of the given bundle(s) match? Then you're right, the condition is not very effective. Maybe it should rather check for LANGUAGE_NONE?

grubshka_v2’s picture

#139 fixed my issue (wrong sort order on untranslated number field)

Thanks !

yobottehg’s picture

Issue tags: +Needs reroll

Does not apply any longer on 8.1.4. Needs a reroll and rebase.

yobottehg’s picture

Assigned: ndewhurst » yobottehg
Status: Needs review » Needs work

i'll try to reroll

yobottehg’s picture

i hope i did everything right

Status: Needs review » Needs work

yobottehg’s picture

new version without the mass replace error

Status: Needs review » Needs work
yobottehg’s picture

one more try

yobottehg’s picture

Status: Needs review » Needs work

Peacog’s picture

I've done a re-roll of the patch from #139 for 8.2.x, since #139 is the last one that passed. I've included a diff rather than an interdiff because a bug in my version of patchutils won't let me create an interdiff of patches that add new files.

Let's see if this one passes.

Status: Needs review » Needs work

Status: Needs review » Needs work
mkernel’s picture

FileSize
1023 bytes

The Patch from #172 worked for us - I just had to fix a minor typo. See attached diff.

lauriii’s picture

Status: Needs work » Needs review
FileSize
31.66 KB

@mkernel: Thank you for the patch. Next time please upload patch which includes all the changes so test bot can test it. Please also read this documentation which explains how to create a patch for Drupal.

I selfishly created the patch myself this time since I believe there is quite a few people using patch from this issue on their sites.

yobottehg’s picture

Status: Needs review » Needs work
diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc
index 1019270..d8bea06 100644
--- a/core/includes/install.core.inc
+++ b/core/includes/install.core.inc
@@ -488,7 +488,7 @@ function install_begin_request($class_loader, &$install_state) {
 
     // Do not install over a configured settings.php.
     if (Database::getConnectionInfo()) {
-      throw new AlreadyInstalledException($container->get('string_translation'));
+      //throw new AlreadyInstalledException($container->get('string_translation'));
     }
   }
 

I think you don't want this in the patch

Xano’s picture

Status: Needs work » Needs review
FileSize
547 bytes
31.12 KB
pminf’s picture

Status: Needs review » Needs work

After appling this patch I get a syntax error, when I filter on a not translatable field (in my case zip code field_service_zip_code):

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') WHERE (( (node__field_service_zip_code.field_service_zip_code_value = '12161')' at line 1: SELECT node_field_data.langcode AS node_field_data_langcode, node_field_data.nid AS nid FROM {node_field_data} node_field_data LEFT JOIN {node__field_service_zip_code} node__field_service_zip_code ON node_field_data.nid = node__field_service_zip_code.entity_id AND (node__field_service_zip_code.deleted = :views_join_condition_0 AND node__field_service_zip_code.langcode = .langcode) WHERE (( (node__field_service_zip_code.field_service_zip_code_value = :db_condition_placeholder_2) )) LIMIT 11 OFFSET 0; Array ( [:db_condition_placeholder_2] => 12161 [:views_join_condition_0] => 0 )

Xano’s picture

@pminf: Please help us reproduce your problem by writing down detailed step-by-step instructions and/or by attaching a failing view configuration file to this issue.

pminf’s picture

Status: Needs work » Needs review

@Xano: This morning I wanted to reproduce it to write a step-by-setp guide but I did not get the error again. I do not know why this happend yesterday but I will keep an eye on it.

Sorry, I had an error while appling the patch. So I updated drupal core to 8.1.7 and applied the patch again with no problem. It's working fine!

swentel’s picture

+++ b/core/modules/views/views.views.inc
@@ -352,6 +353,40 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
+    if ($config->isTranslatable()) {

This had a fatal on one our installations where $config was simply empty .. not sure whether we should account for it or not.

- edit - Ok, so after skipping an empty $config, then clearing cache, and then commenting out the check whether it's empty or not, all is fine again. The thing is, it was impossible to even run update.php or cc with drush before, so quite annoying and almost impossible to recover from this one.

- edit 2 - Actually, after a full cache clear, we're back to fatals:

PHP Fatal error: Call to a member function isTranslatable() on a non-object in /home/project/core/modules/views/views.views.inc on line 368

holist’s picture

Added simple !empty() check to address @swentel's issue in #183.

Status: Needs review » Needs work

The last submitted patch, 184: drupal_2451657_184.patch, failed testing.

lokapujya’s picture

Which config is empty?

Berdir’s picture

+++ b/core/modules/views/views.views.inc
@@ -352,6 +353,42 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
+  // Determine if the field instances are translatable.
+  $bundles_names = $field_storage->getBundles();
+  $translation_join_type = FALSE;
+  $field_configs = [];
+  $translatable_configs = [];
+  $untranslatable_configs = [];
+  $untranslatable_config_bundles = [];
+
+  foreach ($bundles_names as $bundle) {
+    $field_configs[$bundle] = FieldConfig::loadByName($entity_type->id(), $bundle, $field_name);
+  }
+  foreach ($field_configs as $bundle => $config) {
+    if (!empty($config)) {
+      if ($config->isTranslatable()) {
+        $translatable_configs[$bundle] = $config;
+      }
+      else {
+        $untranslatable_configs[$bundle] = $config;
+      }
+    }
+  }

Variable names here could IMHO be better. $config usually refers to simple config, this is a config entity.

Just use $fields and $field. and also $translatable_fields and so on.

The comment is also wrong, there are no field instances in Drupal 8, just field storages and fields.

@swentel: That said, if getBundles() returns something that doesn't exist as a field config, then something is really off. Apparently the field map on that site is out of sync, a field got deleted without updating that, maybe through a low-level config delete. You will have to clean that up manually.

This is not something that should happen. I guess we could use an entity query like this to load them in a way that it only loads those that actually exist:

$ids = \Drupal::entityQuery('field_config')
->condition('entity_type', $field_storage->getTargetEntityTypeId())
->condition('field_name', $field_storage->getName())
->execute();

but that would actually be slower here.

lokapujya’s picture

Issue summary: View changes

Updating Remaining Tasks so some older things don't get forgotten.

holist’s picture

@Berdir, if that shouldn't happen, should we then let the fatal error happen here?

Berdir’s picture

That's always the question. I'm fine with keeping that for now, but while it shouldn't happen, it apparently did. Fatal errors are are bad, silently ignoring something like this too. We could throw an exception, but a non-developer won't be able to solve this himself, so really not sure.

Leave that part as it is for now.

holist’s picture

Thanks @Berdir. If no one else does the variable name change, I'll take that on later today.

I also looked a bit into the discussion with @lokapujya in #157 and #158, doing that part differently would involve changing the whole logic of how the issue has been solved so far. As the current solution works and does not present any apparent problems, I'd rather just go with the current solution and leave open the possibility to improve the query in a follow-up.

swentel’s picture

@Berdir So yeah, for some reason there was a storage field that had no field(s) anymore .. I can't really figure out what happened here since I didn't work on the project internally myself so it's a dark guess. You'd think that $field_storage->getBundles() would return false there, but it doesn't, so as you said, field map was out of sync for some reason and never corrected itself ? (haven't inspected that yet, will do so later)

That said, I'd vote keeping the check in the code, even though it means we have a problem somewhere else, but in this case it's very hard to recover from. The only way is to set a breakpoint and find out which field storage is causing the trouble.

(Ironically I had a somewhat similar problem the same day with a content entity where a term was deleted, but a field was still referencing to it and when trying to display it, it failed horribly)

Berdir’s picture

so as you said, field map was out of sync for some reason and never corrected itself ?

Yeah, the new fieldmap approch has no capability to correct itself. If a change is not reported, then it has no way of finding that out. It's no longer just cached, it's persistently stored.

Agreed, failing with a fatal during cache rebuild is bad, so lets add a check, possibly with a watchdog error that reports the field name and bundle? I think a missing module works in a similar way and it's not easy to clean up either.

swentel’s picture

Agreed, failing with a fatal during cache rebuild is bad, so lets add a check, possibly with a watchdog error that reports the field name and bundle? I think a missing module works in a similar way and it's not easy to clean up either.

Yeah, that was my first thought as well, just wondering whether we have a better place to report this, somewhere more up in the code closer to the field config entities ? Haven't looked closely, but I'm fine here too as a start and maybe a follow up to investigate it more in depth.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

NobodyLikesGreg’s picture

We used #184 in two cases. Once when working with a translated taxonomy field, there it worked. Then we needed it again for a date field and it doesn't work. Could that field still be an exception?

lokapujya’s picture

Issue summary: View changes

Then we needed it again for a date field and it doesn't work. Could that field still be an exception?

Can you elaborate on "it doesn't work"?

Issue Summary Changes: Added the watchdog error to "Remaining tasks". Changed "Improve the query" to "open a followup issue to improve the query."

willwh’s picture

Status: Needs work » Needs review
FileSize
31.14 KB
682 bytes

The last patch failed testing because of a duplicate key in `core/modules/views/tests/modules/views_test_config/test_views/views.view.test_field_config_translation_filter.yml`. I also provided an interdiff Let's hope the test bot likes it, it's passing locally :D

willwh’s picture

Status: Needs review » Needs work

I see Berdir has requested some changes I didn't drop in here, and my patch was bunk too. (nid is not a valid entity_type ;) )

I'll post something up tonight as soon as I can get some time! :)

willwh’s picture

Status: Needs work » Needs review
FileSize
31.17 KB
716 bytes

I'll come back to the rest later tonight :)

willwh’s picture

Interestingly, my patch in #198 has a false positive, insofar, given the actual config in the patch was not quite valid (entity_type: nid).

Anyway, here we go :)

holist’s picture

Thanks @willwh for getting this forward.

Added logging for the empty config entity check.

willwh’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, holist! :)

I've given this a test this morning. Let's hope we can get some more eyeballs on it with an RTBC! :)

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

Interestingly, my patch in #198 has a false positive, insofar, given the actual config in the patch was not quite valid (entity_type: nid).

Interesting! I guess we don't validate views configurations somehow. Do you mind opening a dedicated issue to research that a bit?

F

  1. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,62 @@
    +
    +/**
    + * @file
    + * Contains \Drupal\views\Plugin\views\join\FieldOrLanguageJoin.
    + */
    

    Nitpick: can be dropped

  2. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -274,89 +276,127 @@ public function buildJoin($select_query, $table, $view_query) {
    +   * @param $arguments
    +   *   Array of query arguments.
    +   * @param $condition
    +   *   The condition to be built.
    +   * @param $table
    +   *   The right table.
    +   * @param $select_query
    +   *   The current select query being built.
    +   * @param $left_table
    +   *   The left table.
    ...
    +    * @param $info
    +    *   The extra information
    +    * @param $arguments
    +    *   Array of query arguments.
    +    * @param $table
    +    *   The right table.
    +    * @param $select_query
    +    *   The current select query being built.
    +    * @param $left
    +    *   The left table.
    

    Let's add type information to those parameters.

  3. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -274,89 +276,127 @@ public function buildJoin($select_query, $table, $view_query) {
    +  protected function buildExtra($info, &$arguments, $table, SelectInterface $select_query, $left) {
    

    Is there a reason why &$info is using a reference? I get &$arguments but maybe its just good to document, why we need that.

willwh’s picture

Ok, here we go!

I don't see $info being passed with a reference? Am I blind? :)

Status: Needs review » Needs work

The last submitted patch, 205: drupal_2451657_205.patch, failed testing.

willwh’s picture

Status: Needs work » Needs review

Hmm, tests are passing locally, going to try this again?

dawehner’s picture

I don't see $info being passed with a reference? Am I blind? :)

Nope, but I am :)
I'm pretty sure this was just a random test failure, so let's see what happens. If its green, its RTBC. Great work

willwh’s picture

Hooray :)

So, what did I do wrong, or is it not possible for me to re-queue a patch for testing?

holist’s picture

Status: Needs review » Reviewed & tested by the community

Looks green to me. Thanks @willwh for the final pushes.

holist’s picture

And should this go into 8.2.0 (beta) as well, being a bug fix?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs subsystem maintainer review
  1. There's a lot added conditional logic in this patch - are we sure that we have the requisite test coverage to ensure all cases are tested? Considering the scope of the changes it would be great to have a subsystem maintainer check that out and confirm.
  2. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -274,89 +276,127 @@ public function buildJoin($select_query, $table, $view_query) {
    -/**
    - * @}
    - */
    +
    +
    

    This should be here - it is part of the defgroup docs started at the beginning of the document.

  3. +++ b/core/modules/views/views.module
    @@ -435,23 +434,44 @@ function views_add_contextual_links(&$render_element, $location, $display_id, ar
    - * Implements hook_ENTITY_TYPE_insert() for 'field_config'.
    + * Implements hook_ENTITY_TYPE_insert().
      */
    -function views_field_config_insert(FieldConfigInterface $field) {
    +function views_field_config_insert(EntityInterface $field) {
    ...
    - * Implements hook_ENTITY_TYPE_update() for 'field_config'.
    + * Implements hook_ENTITY_TYPE_update().
      */
    -function views_field_config_update(FieldConfigInterface $field) {
    +function views_field_config_update(EntityInterface $entity) {
    ...
     /**
    - * Implements hook_ENTITY_TYPE_delete() for 'field_config'.
    + * Implements hook_ENTITY_TYPE_delete().
    + */
    +function views_field_config_delete(EntityInterface $entity) {
    +  Views::viewsData()->clear();
    +}
    

    Out of scope changes.

  4. +++ b/core/modules/views/views.views.inc
    @@ -403,11 +463,26 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
    -  // Get the list of bundles the field appears in.
    -  $bundles_names = $field_storage->getBundles();
    

    Seems out of scope too.

sic’s picture

Hi guys,

do I have to apply both patches or is one (which one?) enough?

thanks

jibran’s picture

I'd like to see some Kernel tests like JoinTest::testBasePlugin for FieldOrLanguageJoin.

k4v’s picture

I tested this patch for a different problem:

Views cannot sort on fields that are untranslated for the same reason: The query always contains a join condition node__field_myfield.langcode = node_field_data.langcode.

The effect is that the view is correctly sorted in one language, but not in the other. The patch fixes this for me, thanks :).

A test would be also needed for the sorting problem, I think...

k4v’s picture

Here's a reroll with the changes from #212.

k4v’s picture

k4v’s picture

Assigned: Unassigned » k4v
Issue summary: View changes
jibran’s picture

Status: Needs work » Needs review
k4v’s picture

Status: Needs review » Needs work
k4v’s picture

Assigned: k4v » Unassigned
k4v’s picture

So I worked a bit on the test, trying to add an untranslated weight field to the nodes. But I have no idea how to test the view for different languages...

pminf’s picture

Could you please reroll the last patch for Drupal 8.2?

pminf’s picture

See attached my rerolled version of #217. Beware, it's my first patch.

YesCT’s picture

Status: Needs work » Needs review

needs review to let the bot run on it. (we can then add a test for 8.2.x) [edit: huh. it knew it was 8.2.x? ok.]

Status: Needs review » Needs work

The last submitted patch, 224: views_should_not_2451657_217_for_8_2_x.patch, failed testing.

martin_klima’s picture

I used a workaround until a patch ready for 8.2.x. Ispired by https://api.drupal.org/comment/62648#comment-62648.

function module_name_query_alter(Drupal\Core\Database\Query\AlterableInterface $query) {
  // Define the views name and the field with unwanted langcode relation.
  $views_name = 'views_news';
  $relation_field = 'node__field_news_category';

  if ($query->hasTag($views_name)) {
    // Get tables.
    $tables = &$query->getTables();
    // Get condition.
    $condition = $tables[$relation_field]['condition'];
    // Delete unwanted definition.
    $condition = str_replace("AND $relation_field.langcode = node_field_data.langcode", '', $condition);
    // Replace old condition with fixed one.
    $tables[$relation_field]['condition'] = $condition;
  }
}
lokapujya’s picture

Marting, did you try the patch? Do you think the patch will work for your problem? How does it differ from the code you posted?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

k4v’s picture

k4v’s picture

So here is a version of the sort test. It's still not showing the right thing. When I test manually I can show that the nodes are in the right order in English, but in the wrong order in German.

In this version of the test, the result is in both cases english and the right order. Any idea how I can write the test so it shows the "right" wrong result?

k4v’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 231: test_sort_by_untranslated_field.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views/src/Tests/FieldApiDataSortTest.php
@@ -0,0 +1,164 @@
+    //$config = ContentLanguageSettings::loadByEntityTypeBundle('node', 'page');
+    //$config->setDefaultLangcode('de')
+    //  ->setLanguageAlterable(TRUE)
+    //  ->save();
+
+    /*
+    $node = Node::create([
+      'type' => 'page',
+      'title' => $this->randomMachineName(),
+      'weight' => ['value' => 123],
+      'text' => ['value' => 'moo.'],
+      'langcode' => 'de',
+    ]);
+    $node->save();
+    */

Do you mind removing these orphan experimental code pieces :)

In this version of the test, the result is in both cases english and the right order. Any idea how I can write the test so it shows the "right" wrong result?

Did you maybe forgot something in the test, given its actually failing? Note: For this issue we should IMHO run the tests certainly on all available database engines.

tstoeckler’s picture

FileSize
777 bytes

Just something that at least makes the query in the test query for 'de', the test still fails with this.

tstoeckler’s picture

FileSize
537 bytes

Ok with this I actually get the expected fail because of the incorrect ordering.

k4v’s picture

k4v’s picture

And here's the failing new test.

k4v’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 238: only-sort.test.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

Nice! Passes/fails as expected

k4v’s picture

So what would be the next steps to get this committed? Do we need more tests as requested in #214?

dawehner’s picture

I really like the new methods, as they describe what is done.

+++ b/core/modules/views/src/Tests/FieldApiDataSortTest.php
@@ -0,0 +1,138 @@
+class FieldApiDataSortTest extends FieldTestBase {

We could fix #214 by not extending from FieldTestBase but rather writing a kernel test. Therefor extend from \Drupal\Tests\views\Kernel\ViewsKernelTestBase Maybe check out some of the other classes extending that one. It should give you a good hint.

SaytO’s picture

Apply this solution: #227.
For now solve my problem, TY @martin_klima.

k4v’s picture

Status: Needs review » Needs work
Peacog’s picture

Status: Needs work » Needs review
FileSize
4.82 KB
36.98 KB

This needs a re-roll following on from the decision to use short array syntax in core #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase. Here's a patch.

I couldn't create an interdiff because it fails for some reason, so I've addad an ordinary diff instead.

Peacog’s picture

I missed some arrays using long array() syntax in FieldApiDataSortTest.php. Here's an updated patch.

dawehner’s picture

Here is just some really quick feedback.

+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -1042,10 +1042,13 @@ protected function getTableMapping() {
   public function getValue(ResultRow $values, $field = NULL) {
     $entity = $this->getEntity($values);
+    // Retrieve the translated object.
+    $translated_entity = $this->getEntityFieldRenderer()->getEntityTranslation($entity, $values);
+
     // Some bundles might not have a specific field, in which case the entity
     // (potentially a fake one) doesn't have it either.
     /** @var \Drupal\Core\Field\FieldItemListInterface $field_item_list */
-    $field_item_list = isset($entity->{$this->definition['field_name']}) ? $entity->{$this->definition['field_name']} : NULL;
+    $field_item_list = isset($translated_entity->{$this->definition['field_name']}) ? $translated_entity->{$this->definition['field_name']} : NULL;
 

Is it just me that this seems to be an unrelated issue actually? I guess you used that to write the test coverage, right? What could be nice is to have explicit tests which deal which tests just that, without the language joining.

rodrigoaguilera’s picture

My team testing the latest patch on a drupal 8.3.0-rc2 installation and we are experiencing issues when importing configuration with "drush config-import"

Import the listed configuration changes? (y/n): y
RuntimeException: Unable to determine class for field type '' found in the 'field.field.' configuration in Drupal\Core\Field\FieldConfigStorageBase->mapFromStorageRecords() (line 28 of /var/www/web/co
re/lib/Drupal/Core/Field/FieldConfigStorageBase.php).

I'm feeling this is an untested part of core.

Maybe this is related to the fact that the default language of the site is non-english and all our configuration is in that language.

I don't understand the reason why this fails now but I hope It can give you some information about the risk of this change.

rodrigoaguilera’s picture

Digging more I found that it has to do with having config field translations in /config/sync/language/en/field.field.somentity.fieldname.yml

Making the static cache in the config factory to be corrupt(by only having the translated parts) and leading to the error I pasted.

So this is just triggering it with the following line from the patch

    $fields[$bundle] = FieldConfig::loadByName($entity_type->id(), $bundle, $field_name);

I'll investigate more but probably it won't belong here and it can be triggered by different means.

tstoeckler’s picture

Issue tags: +DevDaysSeville

Talked to @rodrigoaguilera in Seville, and if I understood correctly #250 was caused by a separate issue and was able to be reproduced in HEAD. Would be nice for him or someone from his team to confirm, though.

rodrigoaguilera’s picture

Sorry for the late answer.

I think the issue that I found can't be blamed on this patch although this patch triggers it (there is other ways to trigger it).
I still have to gather some files to report the issue properly. I'll post it here but the work on this shouldn't be halted that's why I didn't change status.

At the moment we really need this patch so we deleted all English configuration since we don't really use it and we have English enabled just for the interface translations. This may be useful for someone as a workaround.

k4v’s picture

To address #248 I created a followup: https://www.drupal.org/node/2866067. EntityField::getValue should return the right translation.

k4v’s picture

As requested in #243 I replaced FieldApiDataSortTest with a KernelTest, now titled SortTranslationTest.

Status: Needs review » Needs work

The last submitted patch, 254: views_should_not-2451657-254.patch, failed testing.

k4v’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 254: views_should_not-2451657-254.patch, failed testing.

tacituseu’s picture

'Missing @group annotation in Drupal\Tests\views\Kernel\Handler\SortTranslationTest'

Berdir’s picture

Fixed that and some more comment clean-ups.

heldercor’s picture

With latest patch from Berdir, in Drupal 8.3, I'm getting this error:

Warning: Illegal offset type in isset or empty in views_query_views_alter() (line 683 of core/modules/views/views.module).
views_query_views_alter(Object, NULL, NULL) (Line: 501)
Drupal\Core\Extension\ModuleHandler->alter('query', Object) (Line: 478)
Drupal\Core\Database\Query\Select->preExecute() (Line: 191)
Drupal\views\Plugin\views\cache\CachePluginBase->generateResultsKey() (Line: 132)
Drupal\views\Plugin\views\cache\CachePluginBase->cacheGet('results') (Line: 1405)
Drupal\views\ViewExecutable->execute() (Line: 807)
views_get_view_result('banners', 'embed', '2') (Line: 174)

The lines in question:

function views_query_views_alter(AlterableInterface $query) {
  $substitutions = $query->getMetaData('views_substitutions');
  $tables = &$query->getTables();
  $where = &$query->conditions();

  // Replaces substitutions in tables.
  foreach ($tables as $table_name => $table_metadata) {
    foreach ($table_metadata['arguments'] as $replacement_key => $value) {
      if (isset($substitutions[$value])) { # line 683
        $tables[$table_name]['arguments'][$replacement_key] = $substitutions[$value];
      }
    }
  }

  // Replaces substitutions in filter criteria.
  _views_query_tag_alter_condition($query, $where, $substitutions);
}

The problem is that in isset($substitutions[$value]), $value is an array (see views_join_condition_2):

$condition = 'bean_field_data.bid = bean__field_image.entity_id AND bean__field_image.deleted = :views_join_condition_0 AND (bean__field_image.langcode = bean_field_data.langcode OR bean__field_image.bundle IN ( :views_join_condition_2[] ))';
$arguments = [
  ':views_join_condition_0' => 0,
  ':views_join_condition_2' => [
    'airline',
    'travel_deal',
  ],
];

The view has a filter for not empty (field_image).

This is without the patch:

$condition = 'bean_field_data.bid = bean__field_image.entity_id AND (bean__field_image.deleted = :views_join_condition_0 AND bean__field_image.langcode = bean_field_data.langcode)';
$arguments = [
  ':views_join_condition_0' => 0,
];
tstoeckler’s picture

This adds a kernel test for the new join plugin similar to JoinTest::testBasePlugin per #214. I actually copied some parts of that method over to the new test because the sole entry point for these join plugins is ::buildJoin() but we are only overriding a specific sub-part of that, so I think it makes sense to make sure that we don't accidentally break the "normal" joining.

So this completes the test coverage as far as I can see, so removing the "Needs tests" tag. Also @dawehner has approved on the general direction of this patch above as far as I can tell, so removing the "Needs subsystem maintainer review" tag as well.

Anyone care to RTBC? ;-)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice test coverage in #262!

  1. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,57 @@
    + * they will be grouped and joined with OR. All bundles the field
    + * appears on as untranslatable are included in $this->extra.
    

    The german in me would like to add some commas in the last sentence. Not sure which specific instance of the english language we follow, but one comma after "bundles" and another one after "untranslatable" makes this sentence, at least for me, more readable.

  2. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    diff --git a/core/modules/views/src/Plugin/views/join/JoinPluginBase.php b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    index 0aa6aa3..bf6c54a 100644
    
    index 0aa6aa3..bf6c54a 100644
    --- a/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    
    --- a/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    
    +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -2,6 +2,7 @@
    
    @@ -2,6 +2,7 @@
     
    

    +1 again for the general refactoring here. This makes the code easier to understand.

  3. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -274,87 +276,124 @@ public function buildJoin($select_query, $table, $view_query) {
    +   * Builds a single extra condition.
    +   *
    +   * @param array $info
    +   *   The extra information
    +   * @param array $arguments
    +   *   Array of query arguments.
    +   * @param array $table
    +   *   The right table.
    +   * @param \Drupal\Core\Database\Query\SelectInterface $select_query
    +   *   The current select query being built.
    +   * @param array $left
    +   *   The left table.
    +   *
    +   * @return string
    +   *   The extra condition
    +   */
    +  protected function buildExtra($info, &$arguments, $table, SelectInterface $select_query, $left) {
    +    // Do not require 'value' to be set; allow for field syntax instead.
    

    I'm adding this point given some discussions in #2877593: Improve \Drupal\hal\LinkManager\RelationLinkManager::getRelations() documentation.
    A review of the documentation of this method:

    1. We clearly indicate the purpose of this function
    2. We document each individual variable
    3. We don't make it clear what is inside $info. I would recommend to NOT provide the information but refer to the general documentation on the class itself. If we repeating this information we add cognitive overload, which causes more harm here.
+++ b/core/modules/views/tests/src/Kernel/Plugin/FieldOrLanguageJoinTest.php
@@ -0,0 +1,224 @@
+ * This extends JoinTest to make sure that the specialized join plugin does not
+ * break any functionality provided by the generic one.

+1 for documenting test purposes!

k4v’s picture

woot!

jibran’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
Related issues: -#2451657: Views should not condition joins on the langcode of fields that are not translatable

Nice work @tstoeckler. Sorry for being a buzz kill but we need some docs here. I also think we need a change notice for this as well.

+++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
@@ -0,0 +1,57 @@
+ * If the extra conditions contain either .langcode or .bundle,
+ * they will be grouped and joined with OR. All bundles the field
+ * appears on as untranslatable are included in $this->extra.

We should add a configuration and SQL example to this doc block for better understanding just like JoinPluginBase. TBH, I didn't understand the problem till I read the newly added KTB so I think docs are essential here. It can be something like this:

    $configuration = [
      'table' => 'node__field_tags',
      'left_table' => 'node',
      'left_field' => 'nid',
      'field' => 'entity_id',
      'extra' => [
        [
          'field' => 'deleted',
          'value' => 0,
          'numeric' => TRUE,
        ],
        [
          'left_field' => 'langcode',
          'field' => 'langcode',
        ],
        [
         'value' => ['page'],
         'field' => 'bundle',
        ]
      ],
    ];

SQL 'AND (node__field_tags3.bundle = :views_join_condition_5 OR node__field_tags3.langcode = .langcode)'

tstoeckler’s picture

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
47.24 KB

This should fix the docs per #263 and #265. I hope this makes it more clear for everyone. I also took the liberty of a minor array re-ordering for consistency with the other conditions.

Re #265: Can you clarify why this needs a change notice? I'm not sure what that should contain, to be honest.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, for adding the docs. We are changing the default behavior here so I thought that's why we need a change notice here. I'll the committer decide that. Do we need an update/post-update hook to clear the cache for existing views?

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -274,87 +276,124 @@ public function buildJoin($select_query, $table, $view_query) {
+   *   The extra information. See JoinPluginBase::$extra for details.   ¶

Redundant blank space. It can be cleared out on the commit.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Sorry this took so long to review, I found a couple of things.

On the update, we should add an empty post update if this is going to get committed to 8.3.x, but since 8.4.x will have other updates, one isn't needed for that.

  1. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,103 @@
    +
    +      if ($extras) {
    +        // Remove and store the langcode OR bundle join condition extra.
    +        $language_bundle_conditions = [];
    +        foreach ($extras as $key => &$extra) {
    +          if (strpos($extra, '.langcode') !== FALSE || strpos($extra, '.bundle') !== FALSE) {
    +            $language_bundle_conditions[] = $extra;
    +            unset($extras[$key]);
    +          }
    +        }
    

    Why &$extra in the foreach? I can't see the reference being used anywhere. If there's a reason for it, it could use a comment because it's not obvious.

  2. +++ b/core/modules/views/src/Plugin/views/join/FieldOrLanguageJoin.php
    @@ -0,0 +1,103 @@
    +        }
    +
    +        if (count($extras) == 1) {
    +          $condition .= ' AND ' . array_shift($extras);
    +        }
    +        else {
    +          $condition .= ' AND (' . implode(' ' . $this->extraOperator . ' ', $extras) . ')';
    +        }
    

    Sine we unset $extras[$key] above sometimes, isn't it theoretically possible that count($extras) == 0 by the time we get here? If so should it not be

    if (count($extras) > 1) {
    }

    else if ($extras) {
    }
    else {
    // Not sure??
    }

  3. Also if it's theoretically possible, is that only due to a bug or is it a valid condition? If it's a valid condition looks like we're missing test coverage.

  4. +++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
    @@ -274,87 +276,124 @@ public function buildJoin($select_query, $table, $view_query) {
    +      if ($extras) {
    

    Why would $extras be empty when we've already checked is(array($this->extra)) above?

  5. +++ b/core/modules/views/src/Tests/FieldApiDataTest.php
    @@ -137,4 +262,119 @@ protected function getViewsData() {
    +      // Why is this one returned?
    

    Is this a @todo/bug?

  6. +++ b/core/modules/views/tests/src/Kernel/Plugin/FieldOrLanguageJoinTest.php
    @@ -0,0 +1,225 @@
    +
    

    Extra blank line.

geertvd’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
47.22 KB
  1. Seemed unnecessary to me too, removed it.
  2. $extras can't be empty in this situation, it will at least contain a ['field' => 'deleted', 'value' => 0, 'numeric' => TRUE] record

    I rewrote that part a bit so it doesn't fatal if someone manages to get there without it anyway. I'm not sure if we should add test coverage for that case.

  3. It could be an empty array I guess, I added an early return instead to make it a bit more readable
  4. This does seem like a @todo which was added in #31
    To answer the question
    Why is this one returned?

    That field is untranslatable so it has a fallback to the original value which is "'field name 3: es'", not sure if that's correct so I left the comment in for now.

  5. Removed
um’s picture

Thanks for the patch.
#270 worked perfectly for me.

tstoeckler’s picture

This adds explicit test coverage per #269.2. Also extracted a helper function in the test to build the join information. This makes the interdiff a bit harder to read, but the resulting test looks a bit cleaner, IMO.

Also not sure why this is tagged "Needs change record", does anyone know?

tstoeckler’s picture

FileSize
46.72 KB

This time with an actual patch, not just a diffstat. Sorry.

The last submitted patch, 272: 2451657-272.patch, failed testing. View results

pritish.kumar’s picture

Fixed the coding standard messages. Interdiff was not generated so couldn't upload.

It gave this error

The next patch would create the file /tmp/interdiff-1.arq4Hs,
which already exists!

lokapujya’s picture

@pritish.kumar , Can you generate an interdiff between #270 and #275?

tstoeckler’s picture

FileSize
1.48 KB

Here is the interdiff.

@pritish.kumar apparently you are using some tool to generate the interdiff which failed you in this instance. What I did was just apply the previous patch with --index then reverse apply it and then apply the new patch. That doesn't always work, but most of the time it does, so hope that helps you for the next time ;-). Concretely, this is what I did in this instance:

$ curl https://www.drupal.org/files/issues/2451657-273.patch | git apply --index
$ curl https://www.drupal.org/files/issues/2451657-273.patch | git apply -R
$ curl https://www.drupal.org/files/issues/2451657-275.patch | git apply

Hopefully someone can RTBC now ;-)