REST views: DataEntityRow allows itself to be added to non-entity views

While we're at it, we might want to make DataEntityRow and EntityRow both inherit from a TranslatedRowPluginBase. That would save a bunch of boilerplate (eg: for EntityTranslationRenderTrait), and they could probably use the same deriver too.

If we want this, then this should be blocked on #2664880: DataEntityRow doesn't respect translations (which I just RTBC'd, yay!).

But I think this can continue regardless in the mean time, because the bulk of the work can still be done.

Thanks for the great bug report! Updated the issue summary's proposed resolution with some more details.

To be honest I disagree with the solution in the issue summary as it would require a BC breaking change. It would certainly require an update of existing views, which we better avoid if possible.

An alternative approach is to use hook_views_plugins_row_alter() and add all entity base tables to data_entity][base.

Ohhh, I like that idea! I was also concerned about the BC break, and first wrote a comment explaining how I didn't see how that could work, but then I saw it, and couldn't see a better way.

So, I guess, #5 is the recommended solution because no BC break, but for D9, we'd want to do it in the way described in the IS?

So, I guess, #5 is the recommended solution because no BC break, but for D9, we'd want to do it in the way described in the IS?

Yeah I think so. It provides a better UX (as it has a better label) and it provides more semantic information in the configuration. It also allows specialized plugins per entity type, even we should avoid that.

Alright, cool. Updated IS accordingly.

Forgot to update the API changes and Data model changes sections.

Thank you wim!

Here is a quick try.

Manually tested.

Before

Perform the STR in the IS: create a new view: display log entries, check the checkbox for REST export.

Observe that it is showing entities, which makes no sense

Confirmed by looking at the config:

      row:
        type: data_entity
        options: {  }

After

Same as above

Observe … that it is unfortunately the same. So #11 did not fix it.

Confirmed by looking at the config:

      row:
        type: data_entity
        options: {  }

Finally, this probably needs an integration test? That would:

1. Enable Views + REST
2. Go to /admin/structure/views/add
3. Do the same as the STR
4. View::load(), inspect the rest_export_1 display's [display_options][row][type]

Thank you for the manual test

Oh I see, so this is about the wizard part as well.

Added tests and made sure the default row type was not set to data_entity for non-entities.

This one should be better

Nice test!

Needs a reroll.

Thank you @geertvd!

+++ b/core/modules/rest/rest.module
@@ -27,3 +28,10 @@ function rest_help($route_name, RouteMatchInterface $route_match) {
+function rest_views_plugins_row_alter(array &$plugins) {
+  (new RestViewsPluginsRowAlter(\Drupal\views\Views::viewsData()))->viewsPluginsRowAlter($plugins);
+}

Instead of three lines of logic in a hook implementation, we have one line, then 15 lines of boiler plate in the class, so we can call a method from the hook implementation that contains the three lines. Really?

Also, can we not do the original solution in 8.x? We could add new plugins, deprecate the old ones, hide them in the UI, keep them around for bc for exported Views, and potentially even do the upgrade path for non-exported views. And then even later add deprecation notices for the old plugins to inform people they need to update their Views in preparation for 9.x

Interesting. Assigning to @dawehner, this is more Views than it is REST. I think this makes it no longer novice.

Two clarifications on #22:

1. If this was complex and/or needed unit testing, then I could see the case for taking the logic out of the hook implementation into a class, neither is the case here though.

2. We don't necessarily have to do that fix here, but it seems worth exploring whether it's possible since it could apply to more things later on, and worth trying in a follow-up.

clarifications on #22:
1. If this was complex and/or needed unit testing, then I could see the case for taking the logic out of the hook implementation into a class, neither is the case here though.

Well yeah I'm following this pattern on custom module development, as it enables you to actually do TDD, which well, at the end of the day, doesn't require you to keep the tests around.

2. We don't necessarily have to do that fix here, but it seems worth exploring whether it's possible since it could apply to more things later on, and worth trying in a follow-up.

Well we could try that fix together with an update path. The more you talk about it, the more I'm convinced of the other solution not really being a BC break.

+++ b/core/modules/rest/src/Plugin/Deriver/ViewsEntityRow.php
@@ -0,0 +1,112 @@
+    $this->derivatives[''] = [
+      'provider' => 'rest',
+      'no_ui' => TRUE,
+    ] + $base_plugin_definition;

What happens if you go to the UI, don't the radio than not has this value and you get a form validation error?

Also, I wonder if this is the root cause of #2858814: REST views: fatal error when using row plugin and Views is aware of non-Entity-based tables.

I think #2858814: REST views: fatal error when using row plugin and Views is aware of non-Entity-based tables may be a duplicate of this.

Apparently I already said that in #32.

@vasi's patch in #26 no longer applied to 8.6.x. A reroll with no further modifications is attached - comments in #28 still not addressed.

Just adaptation to the patch works on drupal 9 update

Marked #2858814: REST views: fatal error when using row plugin and Views is aware of non-Entity-based tables as a dupe and transferred credit

Can we get an issue summary update of what's left here - looks like #28?