Incorrect field name is used in views integration for multi-value base fields

I had opened #2838040: Multi-value multi-column base fields are queried with incorrect column names in Views for that before. Marking that one as duplicated, though, as I didn't have a patch there.

Patch looks great, makes totally sense. One nitpick:

+++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
@@ -527,8 +527,12 @@ public function testBaseTableFields() {
+    // The string field name should be used as the 'entity field' but the actual
+    // field should reflect what the column mapping is using for multi-value
+    // base fields NOT just the field name. The actual column name returned from
+    // mappings in the test mocks is 'value'.
+    $this->assertStringField($data['entity_test__string']['value']);

@@ -741,7 +745,7 @@ public function testRevisionTableFields() {
         ['string', ['value' => 'value']],

Can we change that to return string_value instead, so it matches the real world scenario? This way it is very confusing, IMO.

That's not introduced here, but I still it would be in scope to fix that as part of this issue since we have to change the tests anyway.

Also we should look at how this affects the generated views data of the user roles field as that is a multi-value base field, as well. UserViewsData does contain some overrides for that, but not sure if in the aggregate anything is changed/broken by this.

The explanation in the IS makes total sense.

NW for #6 and #7.

I'd love to RTBC, but can't really do that :)

In any case, this is blocking #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration.

It is really great to see #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration moving forward.

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -371,13 +371,10 @@ protected function mapFieldDefinition($table, $field_name, FieldDefinitionInterf
        // @todo Introduce concept of the "main" column for a field, rather than
        //   assuming the first one is the main column. See also what the
        //   mapSingleFieldViewsData() method does with $first.
   

   This should be updated as well.

2. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_multivalue_basefield.yml
   @@ -30,7 +30,7 @@ display:
   -          field: name
   +          field: name_value
   

   This means we need an upgrade path and upgrade path tests.

Thanks for the update path.

+++ b/core/modules/views/views.install
@@ -412,6 +412,82 @@ function views_update_8201() {
+  // Find all multi-value base fields for content entities.

This should be done in post-update hook and we should store the basefields in KV store.
After that update hook should run and update the views config.

@jibran
I'm confused about your suggestion. Post_update should be used in case you have a working environment, everything is up to date. I'm not sure how you imagine an environment is working, when views are pointing to invalid views data entries. These views are not runnable at that point.

Given that I think using hook_update_N is the sane choice to go here.

Here is the fix for #13.1.

@dawehner let's break it into two update_N hooks so that contrib can also do their thing. Thanks @claudiu.cristea for the fix.

@dawehner let's break it into two update_N hooks so that contrib can also do their thing. Thanks @claudiu.cristea for the fix.

Do you have a concrete usecase? Why can contrib not just run after the other update_N? You know a concrete usecase would be really helpful.

Re #20:

@damiankloip, OK. Opened #2847034: Use getMainPropertyName() instead of first column when building entity views data for that. We'll see which gets in first and adapt the other.

Still needs tests for upgrade path.

1. +++ b/core/modules/views/views.install
   @@ -412,6 +412,82 @@ function views_update_8201() {
   +    }
   +  }
   +
   +  $config_factory = \Drupal::configFactory();
   

   Let's give a chance to exit early here if $table_update_info is empty.

2. +++ b/core/modules/views/views.install
   @@ -412,6 +412,82 @@ function views_update_8201() {
   +
   +}
   +
    /**
     * @} End of "addtogroup updates-8.2.0".
     */
   

   We need to open a new group for 8.3.x updates and include the new function there.

RE #21: If you think it is not worth it then just leave it. :)
NW as per #24.

• +++ b/core/modules/views/src/Tests/Update/EntityViewsMultiValueBaseFieldDataUpdateTest.php
  @@ -0,0 +1,50 @@
  +    \Drupal::service('module_installer')->install(['entity_test', 'views_test_config']);
  

  These should be enabled in additional dump script.

• +++ b/core/modules/views/src/Tests/Update/EntityViewsMultiValueBaseFieldDataUpdateTest.php
  @@ -0,0 +1,50 @@
  +    ViewTestData::createTestViews(get_class($this), ['views_test_config']);
  

  This should also be moved to active storage in extra dump script.

+++ b/core/modules/system/tests/fixtures/update/drupal-8.views-entity-views-data-2846614.php
@@ -0,0 +1,32 @@
+$views_configs[] = Yaml::decode(file_get_contents(__DIR__ . '/drupal-8.views-entity-views-data-2846614.yml'));
+
+foreach ($views_configs as $views_config) {
+  $connection->insert('config')
+    ->fields(array(
+      'collection',
+      'name',
+      'data',
+    ))
+    ->values(array(
+      'collection' => '',
+      'name' => 'views.view.' . $views_config['id'],
+      'data' => serialize($views_config),
+    ))
+    ->execute();

This looks exactly like I woul dhave done it.

Yay!

+++ b/core/modules/system/tests/fixtures/update/drupal-8.views-entity-views-data-2846614.php
@@ -0,0 +1,32 @@
+    ->fields(array(
...
+    ))
+    ->values(array(
...
+    ))

This is new code so let's use square bracket array syntax.

I'm still trying to fix the update path 100%.

Thanks, @dawehner!

This was a bit of a fight.

• We need to update the plugin_id as well
• We need to update the string configured in the roles filter to an array
• We need to remote orphan keys, for example as we convert a field from a fieldapi handler to a non fieldapi handler. This requires some typed data magic.

Wow, that looks like quite some fun. My brain ran away when I opened the patch file, so only a cursory review for now.

1. +++ b/core/modules/views/views.install
   @@ -460,14 +460,22 @@ function views_update_8300() {
   +  $views_data = \Drupal\views\Views::viewsData();
   

   use a proper use ;-)

2. +++ b/core/modules/views/views.install
   @@ -460,14 +460,22 @@ function views_update_8300() {
   +  $handler_types_singular = array_map(function ($handler_type) {
   +    return substr($handler_type, 0, -1);
   +  }, array_combine($handler_types_plural, $handler_types_plural));
   
   @@ -479,14 +487,34 @@ function views_update_8300() {
   +              $view->set($path_plugin_id, $views_data->get($table)[$table_update_info[$table][$original_field_name]][$handler_types_singular[$handler_type]]['id']);
   

   That's fun! :-)

   Since you're only using the singular version in a single place, you could also just do the substr() in the loop instead of up-front, but that's just me.

3. +++ b/core/modules/views/views.install
   @@ -460,14 +460,22 @@ function views_update_8300() {
   +    /** @var \Drupal\Core\Config\Schema\TypedConfigInterface $typed_view */
   
   @@ -495,10 +523,37 @@ function views_update_8300() {
   +    $typed_view = $typed_config_manager->get($id);
   

   The inline typehint got lost a bit, I guess.

4. +++ b/core/modules/views/views.install
   @@ -495,10 +523,37 @@ function views_update_8300() {
    
   +
    }
   

   Unneeded

Since you're only using the singular version in a single place, you could also just do the substr() in the loop instead of up-front, but that's just me.

Good point. I think using singular by default is a better solution;

The inline typehint got lost a bit, I guess.

Indeed. Lost in code.

We might need to be more specific...

Fair point ...

Nice that we have consistent name spacing form user module plugins :P

Life isn't fair

I think we might need to check the config schema here to determine whether this should indeed be an array or not. E.g. if the new handler is also a standard filter, a string would be correct.

Any idea how we could do that?

Oh, do we need to do separately so the config is reloaded? Maybe a quick addition to that comment.

Yeah

So something like this?

One word - legendary.

Hehe, well as you see it doesn't work.

... I rewrote the logic to just load each view once. I don't understand why, but you know it works.

Cool! Looks nice.

+++ b/core/modules/views/views.install
@@ -415,3 +417,147 @@ function views_update_8201() {
+  // keys so he configuration applies to the config schema.

Typo: he > the. Can be fixed when committing the patch.

Is this bug going to be fixed in The Drupal 8.3 series, or do we have to wait for 8.4?

Patch 51 has been tested on 8.2.4 and it works. Another RTBC.

Is it possible that this patch breaks something with multi-value fields? I created a view between a base table and another Entity that has unlimited cardinality. This Entity has a multi-value field also with unlimited cardinality. The view's results display fine as an unsorted list after applying the patch, but if I try to push the results into a custom views formatter, I have to do some sneaky stuff the get the correct value:

function template_preprocess_views_view_flot_views_time(&$variables) {
  $view = $variables['view'];
  $field_name = 'foo_bar';
  foreach ($view->result as $row) {
    $field_value = $view->field[$field_name]->getValue($row);
    if ($view->field[$field_name]->multiple){
      $field_value = $view->field[$field_name]->getItems($row)[0]['rendered']['#markup'];
    }
  }
}

My field is named 'foo', and the subfield is 'bar'.

in the example above, where $field_name is 'foo_bar', the field is named 'foo', and it contains a few columns, one of which is 'bar'.

This has now been blocking for precisely the entire month of February: #2543726-96: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration.

Well getting 8.3.x out is much more important than a bugfix, which will be fixed after 8.3.x is out. Distracting committers is IMHO always a bad idea.

Of course. The core committers have been doing a fabulous job :)

But that doesn't mean Drupal would not benefit from more core committers.

I kinda believe that this is not the biggest problem we have :)

Re-rolled.

#66 matches the previous patch (in #51). Just updated for short array syntax.

Something with the reroll in #70 went wrong, started again from #66.

1. Sorry for taking ages to review this
2. Don't we need to do something like #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table to fix views stored in module's config folders? That patch added \Drupal\views\Entity\View::fixTableNames() and calls in during preSave().

What is the status of this? It's holding up a few other issues.

This needs an answer to #74.2, possibly a reroll.

Well the answer to #74.2 is 'Yes, we do need something like that' :).
See #2870724: Define and document best practices for configuration entity updates/bc layers for the route we take at the moment for implementing #74.2.

The main problem I see here is that the assumption of using hook_ENTITY_TYPE_presave mentioned here https://www.drupal.org/node/2851293#comment-12043751

Using hook_ENTITY_TYPE_presave() looks heavy-handed, but I don't think it's that bad really:

- generally it's a couple of quick if checks

Is not valid here.

The hook update provided by this patch first makes a list of all the tables relevant to specific entities and then iterates through all views/handlers to find matches. I'm seeing a big performance impact if we try to do the same that we did in #2851293: dblog is using the wrong views field, filter and relationships definitions

Do anyone knows a better aproach for this case?

We pretty much have an unmaintainable system here.

:(

Are the last few comments really saying this is basically impossible to fix?

Question. Could we make this a hook_requirement task and regularly check for new views/entity types created with cron? So we can discover broken views without affect the performance of the system each time a view is saved.

Maybe we can have a list of recently updated views with a timestamp and check the new ones based on that information.

This is a major bug. I'm voting for committing it as is and announce the BC break.

I had a look at the patch and foreach ($entity_type_manager->getDefinitions() as $entity_type_id => $entity_type) { part of the patch can be replaced by collecting all the entity types of a saving view and rest of the code would be the same but to call that in preSave would have large performance impact.

I really don't like solution purposed in #83 but if that's the only choice we have to maintain BC then I think we can opt it. As @damiankloip purposed the solution I think we need a +1 from @dawehner or @Lendude. Also before implementing anything, I think an approval from framework manager is also required.

Changing it to NR and adding tags.

Per #2543726-111: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration, can we please move forward here? This has been blocking that issue since January at minimum now…

Actually, per #84 and #85, this actually should be RTBC again it seems.

Patch no longer applies, conflicts with #2868841: \Drupal\user\Plugin\views\filter\Roles::calculateDependencies breaks when using the empty/not empty operators .

This one looks a bit more difficult to resolve, so I'm leaving it to those who know \Drupal\user\Plugin\views\filter\Roles::calculateDependencies() better to resolve it.

+++ b/core/modules/user/src/Plugin/views/filter/Roles.php
@@ -74,10 +74,19 @@ public function operators() {
    */
   public function calculateDependencies() {
     $dependencies = [];
+
     if (in_array($this->operator, ['empty', 'not empty'])) {
       return $dependencies;
     }
-    foreach ($this->value as $role_id) {
+
+    // Due to a bug $this->value might be a string, see
+    // https://www.drupal.org/node/2846614. In the empty case stop early.
+    // Otherwise we cast it to an array later.
+    if (is_string($this->value) && $this->value === '') {
+      return [];
+    }
+
+    foreach ((array) $this->value as $role_id) {

These are the changes now.

+++ b/core/modules/user/src/Plugin/views/filter/Roles.php
@@ -74,7 +74,15 @@ public function operators() {
-    foreach ($this->value as $role_id) {
+
+    // Due to a bug $this->value might be a string, see
+    // https://www.drupal.org/node/2846614. In the empty case stop early.
+    // Otherwise we cast it to an array later.
+    if (is_string($this->value) && $this->value === '') {
+      return [];
+    }
+
+    foreach ((array) $this->value as $role_id) {

Versus this before.

… that makes sense :)

I think it's quite an edge case that a module would be providing default configuration in a view that doesn't work? The only case that kind of works ok I think is the field handler(s).

I don't think Views with field handlers is exactly an edge case, is it? So in other words, we have this in the patch:

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_multivalue_basefield.yml
@@ -30,7 +30,7 @@ display:
-          field: name
+          field: name_value

This is for a module-provided View that works perfectly fine in HEAD (AFAIK), and has a EntityViewsWithMultivalueBasefieldTest test to prove that. But if we commit this patch without the above hunk, then that View (and test) would become broken. Which means if we commit this patch as-is, then it is a real BC break that would impact sites using contrib modules with similar kinds of exported Views that the contrib authors have not yet re-exported.

To be honest, I'm not 100% convinced that we need to babysit non-updated contrib. Default config is part of code, and we allow for some BC breaks in code (just not in the API) between minor releases. However, https://www.drupal.org/core/d8-bc-policy says that "References to plugin IDs and settings in default configuration can be relied upon however.", and if we're serious about that, then I guess we do need to preserve BC in this case (since this part of the configuration is a plugin setting, I think). There's also some background reading in #2870724: Define and document best practices for configuration entity updates/bc layers and #2248983-130: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table.

Tagging for release manager review as well as framework manager review, because I definitely see the tension here between keeping our promise as it's currently written in https://www.drupal.org/core/d8-bc-policy and the concerns expressed in #84 about the performance impact on every View save of doing so.

I'm also curious if it's possible to only do the update logic on the first save from default config to active config rather than on every save within active config. Not sure if that's already been discussed on any of the other issues where this has come up.

Making sure I understand the bc implications correctly:

1. Field handlers affected by this probably work, the reason they work is because they don't reference column names anyway usually (values are loaded from the entity). So while a view using them might work and be stale, it will continue to work even though it's stale.

2. sort/filter/arguments are currently broken (you could set a view up with them but it won't work), and they get fixed by the patch. Nothing that's not already broken will get broken.

If that understanding is correct, then it's a shame about stale default configuration, but seems like an OK trade-off to fix the major bug.

The update hook here does look too heavy to do in presave, the only way I can think to make it acceptable would be to do something like add a _2846614_updated key to views config entities, so it only runs once per view (and every time for default configuration). If this is going to break default config then that might be something to consider, but if it's not then adding that complexity feels worse than some potentially stale (but not broken) exported views.

Field handlers affected by this probably work, the reason they work is because they don't reference column names anyway usually (values are loaded from the entity). So while a view using them might work and be stale, it will continue to work even though it's stale.

I don't think that's true. I don't see how the config in field handlers is stale. It seems to me to be actually used config. To demonstrate that, here's the same patch as #90, but with the change to views.view.test_entity_multivalue_basefield.yml reverted. Predictably, it breaks EntityViewsWithMultivalueBasefieldTest. AFAICT, this means that if we commit #90 as-is, then every contrib module's default Views with perfectly functional field displays of multivalued base fields will start breaking.

Another concern I have with the approach in this issue. Specifically, the IS's proposed resolution of:

Use the schema field name, as it already generates the correct field name.

This will then tie default config to storage assumptions. For example, suppose module A ships a default View exported from a site where a given base field is single valued. But now, suppose module B implements hook_entity_base_field_info_alter() to change the cardinality of that base field. With #90, doing this means that module A's default View is now broken for a site with module B enabled.

I don't know if this is already a pre-existing condition elsewhere in Views config structure.

It shouldn't be this hard to fix something which is completely broken.

I completely sympathize with your frustration. So I thought more about what I wrote in #92: are we being too strict in our BC policy with respect to default config, specifically the part about "References to plugin IDs and settings in default configuration can be relied upon however.".

However, no, I think we're right to be that strict about that. Because everything else that we consider @internal is stuff that it should be possible to write a contrib module without using. But the canonical way for a module to provide default config is via default config, so I think we must consider plugin IDs and settings referenced by default config to be public API.

we could also omit fields from this patch so they can keep the wrong column names

That would satisfy the BC requirement. And also #96. However, I still wonder why we want to make arguments/filters/sorts reference the db column name. Instead of changing the config, could we instead make the code expect the entity field name, and figure out the column name at runtime? If that's an expensive determination, can we cache it?

Thanks, @damiankloip!

Back to RTBC for feedback from @effulgentsia.

While we are waiting for @effulgentsia we can remove the subsystem and release manager tags.

@damiankloip is actively involved in it so removing 'subsystem maintainer review' tag. @catch would you like to remove the 'release manager review' tag?

Yes I'm happy with this patch so I think we just need effulgentsia to come back on it at this point.

Thanks! catch. Let's assign it to him.

Thanks catch!

Well, meanwhile, this also needs a change record if there is a BC break.

Since @effulgentsia hasn't had a chance to provide feedback in a couple months, and since @catch said he was comfortable with it, we can unassign from @effulgentsia. Marking NW for the CR though.

Also, can we reattach the correct patch and a test-only patch while we're at it? Something is weird with it in the IS.

One small thing to fix while I'm here:

+++ b/core/modules/user/src/Plugin/views/filter/Roles.php
@@ -74,10 +74,19 @@ public function operators() {
+    // Due to a bug $this->value might be a string, see
+    // https://www.drupal.org/node/2846614. In the empty case stop early.
+    // Otherwise we cast it to an array later.

We should almost never refer to the issue in a code comment; instead, the code comment should sufficiently and succinctly explain the bug.

Removing the tags now. CN looks good to me. Back to RTBC.
I found dynamic_entity_reference_entity_test_views_data added in #2548395-35: Provide views relationships for DER base fields providing field data for DER basefields I added that before #2477899: Multiple valued Base fields won't work in Views and never got around to removing that. I had the correct mapping all along and hopefully, I can remove that hook once this is committed.

Just minor nit. It can fixed on commit.

+++ b/core/modules/views/src/Tests/Update/EntityViewsMultiValueBaseFieldDataUpdateTest.php
@@ -0,0 +1,56 @@
+ * @see views_update_8300()

s/8300/8500

Agreed with re-RTBC'ing.

Needs a re-roll after #2901739: Fix 'Squiz.Arrays.ArrayDeclaration' coding standard.
Edited: confused myself with #112.

This patch was RTBC for two months and one day, and now it needs to be rerolled that? 😢

Git was able to rebase this without any interaction so going straight back to RTBC. Bot will set me straight, in case anything went wrong.

effulgentsia had some remaining concerns that this could break previous working views, but those concerns aren't expressed on this issue and I think the rest of us are pretty convinced that this shouldn't happen. Also that theoretical possibility shouldn't completely override fixing definite currently broken views/making working views possible.

So I'm planning to commit this to 8.5.x this week, but not to 8.4.x to give time to flush out any such issues.

Tagging for release notes for the same reason.

Committed 3b684e5 and pushed to 8.5.x. Thanks!

Comment/nit from #113 wasn't fixed.

Aargh well spotted. Made an additional commit to fix that.

Woot, this finally unblocked #2543726 — see #2543726-150: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration!

As an aside, Patch #95 works on my 8.4.2 installation