Views should not condition joins on the langcode of fields that are not translatable

/

... its pretty major, honestly

Major, at least. Testing this right now.

@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')
    )
  

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

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

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

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.

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.

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.

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

+1

@Upchuk: still working on this one?

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.

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.

+++ 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.

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

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

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. :)

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

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.

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? ;)

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.

@upchuck:

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

upchuck++ :)

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

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?

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.

Re-rolling the patch which is in #60

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

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

Fixing syntax error from #64.

Adding missing class FieldOrLanguageJoin.

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

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.

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

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

Something like this?

Rerolled, now working on the failure

There we go.

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

Looks great!

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

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

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

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

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

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.

Thank you @Upchuk

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.

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.

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

Needs reroll according to #118.

Rerolled

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

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

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

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:

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

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

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

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.

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).

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

New patch with improved documentation.

Oops, wrong file, let's try again.

Triggering testbot.

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.

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.

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.

@lokapujya So do you think this issue 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.

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.

@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!

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?

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.

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

Needs reroll for 8.2

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

Hi ndewhurst, lokapujya,

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

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

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?

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?

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

Thanks !

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

i'll try to reroll

i hope i did everything right

new version without the mass replace error

one more try

Forgot one Test

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.

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

@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.

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

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 )

@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.

@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!

+++ 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

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

Which config is empty?

+++ 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.

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

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

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.

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.

@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)

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.

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.

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?

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."

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

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! :)

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