Pass view name and display id to alterFormField() method in ViewsReferenceSetting plugin

I think passing the view name and display ID as plugin config is very creative. It would mean we can fix #3004636: Add View filter plugin to improve experience for content editor and #3081979: Add $element and $form_state to interface method alterFormField without breaking BC. I like it, nice job!

Here is a quick review. I think we should probably pass the view name and display ID to createInstance in viewsreference_views_pre_view() as well.

1. +++ b/src/Plugin/Field/FieldWidget/ViewsReferenceTrait.php
   @@ -88,13 +88,14 @@ trait ViewsReferenceTrait {
   +    $display_id = isset($field_value['display_id']) ? $field_value['display_id'] : '';
   

   Since PHP 7 we can finally do this:
   $display_id = $field_value['display_id'] ?? '';

   Let's also default to NULL instead of an empty string.

2. +++ b/src/Plugin/Field/FieldWidget/ViewsReferenceTrait.php
   @@ -148,7 +159,10 @@ trait ViewsReferenceTrait {
   +          'view_name' => isset($view_name) ? $view_name : NULL,
   +          'display_id' => isset($display_id) ? $display_id : NULL,
   

   We might want to make sure those vars always exist.

The config solution is nice, but each plugin requiring the view in its alterFormField() will be forced to load the view in their own ways, which is a waste of memory as the same view will be loaded in multiple objects.

We could provide a base method - or a trait - doing something like:

protected function loadView() {
static $view;
if (!isset($view) && $view = Views::getView($field_value['target_id'])) {
$view->setDisplay($field_value['display_id']);
$view->initHandlers();
}

return $view;
}

Which at least reuses the same $view object, and there is a helper which avoid the far-west of everyone loading the view on own ways. Thoughts?

Thanks for the response gambry. I see your point about the inefficiencies of having to load the view multiple times, however, the entity storage already has static caching to make this more efficient. Besides that, this would only impact specific admin forms further mitigating the issue. I'm not sure how common it is to embed a lot of entity references on a single entity, but I can imagine it is mostly one and a couple at most.

So even though I agree that we might be able to improve / rethink this in a future version, I would really prefer that we do not break existing sites with custom plugins for now.

One thing we could do, is provide a base class implementing ViewsReferenceSettingInterface with a protected helper method to fetch the view from config. That way, custom plugins could extend that if they want to. We might also want to have the existing plugins extend that to lead by example.

Ok tested the above patch, and used it combination with the viewsreference_filter module.

Works as expected: Selecting the view display initiates an additional ajax call to load the form settings. Values for view_name and display_id are passed now to the viewsreferencesetting plugin as configuration:

    $view = $this->viewsUtility->loadView($this->configuration['view_name'],
      $this->configuration['display_id']);

Be great to get this merged so viewsreference_filter does not have a dependancy on this patch and can have a stable release.

@gambry #3 I've added a service to the above module that handles the loading of view, as suggested in your comment.

Here is an updated patch with my own suggestions from #2 implemented. Also simplified the patch a bit by removing unnecessary parts. It would be great to have some confirmation this doesn't break anything.

If someone could RTBC I will commit it.

Patched applied cleanly and code seems to work :)

Fixed! Will roll a new release today.

I see the claim here that "It also adds ajax refresh to the options field triggered by a change on the display_id field" but that does not seem to be the case -- can anyone else confirm? The lack of this ajax call seems to be causing #3157245: Contextual Filter is not showing on the Editor Page

Patch for the display ID problem posted on #3166490: Trigger ajax refresh when display ID is changed.