Problem/Motivation

From #2543726: Make $term->parent behave like any other entity reference field:

It seems to be a generic problem with views data generated for multi-value based fields. The code is assuming that if it only have one property/column it should just use the field name, which in this case was 'parent' but this field is multi-value, so it has a dedicated table, which means the actual schema column name should be used. I think it's correct to always use this as regular base fields will have the same result as the actual $field_name we were using before. So we basically just remove the $multiple logic. So it kind of worked by chance before, because field names and storage names were always the same. As a side note, configurable fields are not affected as they are handled slightly differently in views_views_data().

Example breakage from the above issue:

- Base field is create with name 'parent', and it's a multi-value field - this means it gets it's own storage table, 'taxonomy_term__parent'
- This table does not have the same format as base fields in the base table, so 'parent' would not be named in a single column 'parent', the actual value field is 'parent_target_id' - similar to any other configurable field
- When the views data is generated, it assumes the field name due to this code:

    $multiple = (count($field_column_mapping) > 1);
    $first = TRUE;
    foreach ($field_column_mapping as $field_column_name => $schema_field_name) {
      $views_field_name = ($multiple) ? $field_name . '__' . $field_column_name : $field_name;
      $table_data[$views_field_name] = $this->mapSingleFieldViewsData($table, $field_name, $field_definition_type, $field_column_name, $field_schema['columns'][$field_column_name]['type'], $first, $field_definition);

      $table_data[$views_field_name]['entity field'] = $field_name;
      $first = FALSE;
    }

So the wrong assumption is that any field that only has one column in the mapping must have the same column name as the field name itself! Which is not a correct assumption. This is only TRUE (and because the storage implementation happens to use the same names) for base fields that share a table.

Another side affect of this, the Views UI contains two handler entries for each of these fields, one working implementation and one broken one, for things like user roles. Only because UserViewsData is explicitly creating the 'roles_target_id' item itself, to basically get around this bug. Some of this data is just wrong, it doesn't work, and it should be be there.

Proposed resolution

Use the schema field name, as it already generates the correct field name. Still use the field name for the 'entity field' value. The tests need to be fixed as they expect the wrong values in some assertions.

Remaining tasks

User interface changes

Some duplicate handlers will be removed in the Views UI listing

API changes

Data model changes

Comments

damiankloip created an issue. See original summary.

damiankloip’s picture

damiankloip’s picture

Issue summary: View changes

The last submitted patch, 2: 2846614-1-correct-tests-FAIL.patch, failed testing.

tstoeckler’s picture

tstoeckler’s picture

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.

tstoeckler’s picture

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.

Wim Leers’s picture

Status: Needs review » Needs work

The explanation in the IS makes total sense.

NW for #6 and #7.

damiankloip’s picture

Thanks tstoeckler, and sorry for not seeing your original issue. Daniel pointed me to that afterwards, but by then you'd already commented here :)

Yes, changing the name to the 'real' column value is a good idea, ++ to that.

So yes, user__roles is really the only case we have in core right now, I think. It will mean the 'user__roles' > 'roles' item will not exist anymore, and the 'user__roles' > 'roles_target_id' will be created correctly, we would just need to modify it to add our own handler IDs instead of creating it from scratch like it currently happening. That's actually a clear sign that our integration is currently broken too.

So maybe we need to write an update handler to fix this in user module? but if people are using that version, it's probably not working correctly anyway? or, we just copy the new data over to 'roles' too, so it's still there.

damiankloip’s picture

Here is the cleanup for #6 and some other stuff we can remove now we have user__roles table data being generated correctly. Just need to address the loss of the 'roles' item. I'm thinking an update handler is the best way to go here..

damiankloip’s picture

Status: Needs work » Needs review
FileSize
8.26 KB
5.75 KB

Ugh, forgot to attached the patch!

Wim Leers’s picture

Issue tags: +blocker

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.

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs upgrade path, +Needs upgrade path tests

It is really great to see #2543726: Make $term->parent behave like any other entity reference field 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.

damiankloip’s picture

1. Why do we need to update that todo comment? That's not anything to do with this fix.

2. Yes we need an update handler I think. Not sure about update path tests though. That seems overkill to me, for essentially a broken handler. Also how do you test two versions of generated views data that's only cached? I guess the best we could do is have a view with the old data and test it gets updated maybe.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path
FileSize
11.57 KB
3.32 KB

Here is the update handler.

jibran’s picture

Status: Needs review » Needs work

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.

dawehner’s picture

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
4.19 KB
14.21 KB

Here is the fix for #13.1.

jibran’s picture

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

damiankloip’s picture

No, IMO #13.1 is out of scope of this issue. This is not what it's trying to fix - otherwise I would have fixed it :)

dawehner’s picture

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

claudiu.cristea’s picture

damiankloip’s picture

claudiu.cristea’s picture

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.

jibran’s picture

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

damiankloip’s picture

Going to work on an upgrade path test with a view being updated.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
14.33 KB
4.19 KB

Here are some of those changes and a start on update tests. They currently don't work. Lots of schema validation issues when the views are saved I think... Amongst other things.

Interdiff is from #15.

jibran’s picture

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

damiankloip’s picture

Yeah, there is still a WIP currently :/

Status: Needs review » Needs work

The last submitted patch, 27: 2846614-27.patch, failed testing.

damiankloip’s picture

Here is an updated update path test with the standard d8 db dump but a new views data file to add the view the update handler should fix.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
+++ 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.

Wim Leers’s picture

Yay!

Status: Reviewed & tested by the community » Needs work

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

claudiu.cristea’s picture

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

damiankloip’s picture

OK, thanks. That will be in the next patch.

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.

Wim Leers’s picture

Version: 8.4.x-dev » 8.3.x-dev
dawehner’s picture

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

Wim Leers’s picture

Thanks, @dawehner!

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.88 KB
6.83 KB

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.
tstoeckler’s picture

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

xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev
damiankloip’s picture

One word - legendary. This is pretty much what we need I think.

  1. +++ b/core/modules/user/src/Plugin/views/filter/Roles.php
    @@ -74,7 +74,14 @@ function operators() {
    +    // Due to a bug $this->value might be a string. In the empty case stop
    

    We might need to be more specific...

  2. +++ b/core/modules/views/src/Tests/Update/EntityViewsMultiValueBaseFieldDataUpdateTest.php
    @@ -0,0 +1,56 @@
    +      $this->assertEqual($type === 'arguments' ? 'user__roles_rid' : 'user_roles', $handler_config['plugin_id']);
    

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

  3. +++ b/core/modules/views/views.install
    @@ -415,3 +415,147 @@ function views_update_8201() {
    +              if ($handler_type === 'filters' && is_string($data['value'])) {
    

    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.

  4. +++ b/core/modules/views/views.install
    @@ -415,3 +415,147 @@ function views_update_8201() {
    +  // Beside of updating the field and plugin ID we also need to truncate orphan
    +  // keys so he configuration applies to the config schema.
    +  foreach ($required_cleanup_handlers as list($id, $path_to_handler)) {
    

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

damiankloip’s picture

Issue summary: View changes
dawehner’s picture

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

damiankloip’s picture

Any idea how we could do that?

I think as we already have the loaded schema object, we should be able to just check the type of the value property from there?

dawehner’s picture

So something like this?

Status: Needs review » Needs work

The last submitted patch, 48: 2846614-48.patch, failed testing.

Wim Leers’s picture

One word - legendary.

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.09 KB
3.63 KB

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.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

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.

damiankloip’s picture

+++ b/core/modules/views/views.install
@@ -517,40 +517,42 @@ function views_update_8300() {
+      if (strpos($path_to_handler, 'filters') !== FALSE && $typed_config->get('value') instanceof ArrayElement && is_string($config['value'])) {

Yes ++ This is a change that I had locally to fix a small issue before I realised you were fixing the patch up.

The changes you've made for the update schema stuff makes sense to me now, and works nicely!

RTBC +1

Beakerboy’s picture

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

Beakerboy’s picture

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

Beakerboy’s picture

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

damiankloip’s picture

Do you have sample configuration you can attach? Also, are your custom fields attached to the new/correct columns (field_name > field_name_column_name), not the old ones (field_name > field_name)?

Beakerboy’s picture

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

damiankloip’s picture

Right, I saw that, but that doesn't give me any additional info about your custom handler :)

Wim Leers’s picture

This has now been blocking for precisely the entire month of February: #2543726-96: Make $term->parent behave like any other entity reference field.

dawehner’s picture

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.

Wim Leers’s picture

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

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

dawehner’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: 2846614-51.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
25.04 KB

Re-rolled.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 66: 2846614-66.patch, failed testing.

gaurav.kapoor’s picture

Assigned: Unassigned » gaurav.kapoor
gaurav.kapoor’s picture

Assigned: gaurav.kapoor » Unassigned
Status: Needs work » Needs review
FileSize
25.11 KB

Status: Needs review » Needs work

The last submitted patch, 70: 2846614-70.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
25.01 KB

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

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Looks good - back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
  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().
dubcanada’s picture

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

Wim Leers’s picture

Status: Needs review » Needs work

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

Lendude’s picture

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.

dagmar’s picture

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?

damiankloip’s picture

I agree there is going to be a lot of logic needed if this does go into a presave hook. There is not much in the way of alternatives afaik. As I mentioned before, this is also an awful lot of work for a group of essentially broken handlers :( We pretty much have an unmaintainable system here.

Wim Leers’s picture

We pretty much have an unmaintainable system here.

:(

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

damiankloip’s picture

I think we certainly have to fix this, the current views data is just wrong. Which means by default all multi value fields are broken. It's just frustrating with all the hoops we need to jump through for BC for existing broken handlers. That if used, people would mostly have broken views anyway... 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 wonder if we could copy views data to the old location for field handlers only? That could be a work around here.

dagmar’s picture

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.

damiankloip’s picture

Potentially.. still seems like a disproportionate amount of work for fixing broken views handlers though :)

What do you think about copying the views data back to the old key for fields only? The rest are almost certainly broken anyway. If people are making alterations in their code, we have no real way to fix that, unless we duplicate all data to the old location too, which I kind of don't like.

jibran’s picture

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.

damiankloip’s picture

Yes, I would rather just have a break, as it's for stuff mostly..broken :) The copying of views data is kind of a last resort for me. Well, actually, I would prefer that over something that will be running on every view save operation.

Wim Leers’s picture

Per #2543726-111: Make $term->parent behave like any other entity reference field, can we please move forward here? This has been blocking that issue since January at minimum now…

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 72: 2846614-72.patch, failed testing.

Wim Leers’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
25.16 KB
985 bytes

I think this should do it.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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 :)

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs release manager review

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.

damiankloip’s picture

Field handlers weren't included in the 'edge case'. It is sorts/filters/arguments (that would alter queries) that are the ones that'll be broken. Most field handlers use the field field handler which doesn't care what actual column it is really, as it just renders it from the entity.

As mentioned somewhere above, we could also omit fields from this patch so they can keep the wrong column names. I don't mind. It shouldn't be this hard to fix something which is completely broken.

catch’s picture

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.

effulgentsia’s picture

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.

effulgentsia’s picture

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.

effulgentsia’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 95: 2846614-95.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damiankloip’s picture

This will then tie default config to storage assumptions.

How do you account for both instances in that case? Take a look at UserViewsData, $data['user__roles']['roles_target_id'] and $data['user__roles'] and everywhere else - this is adding 'schema' knowledge to the views data, which is why that code works. It is really only needed due to this bug, so there are currently duplicate entries for user data, for the roles field - one broken and one working (added to work around this bug originally I guess). So there will likely be many many views using that configuration. There are also tons of other places that actual table and column names are used and needed.

... With #90, doing this means that module A's default View is now broken for a site with module B enabled

I don't think so, this would already break AFAICT. The code in EntityViewsData also tries to work out the table names based on the field table mappings. So your table would already change if you did that anyway. Your views field would move to a different table already. The same code also mimics the table mapping column name based on whether it's a multi value field or not too!

$views_field_name = ($multiple) ? $field_name . '__' . $field_column_name : $field_name;

So I guess I don't really get what you're getting at with that angle, sorry :/

Another potential option (which me and dawehner weren't really keen on originally) is to add a 'real field' key to every multi value field accross all views data. That would keep the current, incorrect, inconsistent column keys.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Thanks, @damiankloip!

Back to RTBC for feedback from @effulgentsia.

jibran’s picture

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

jibran’s picture

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

catch’s picture

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

jibran’s picture

Assigned: Unassigned » effulgentsia

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

Wim Leers’s picture

Thanks catch!