In #2859139: Automatically disable caching of views facet sources (regression) we noticed that there's code duplication in the module, we do something like this a couple of times. And we'll probably do this more if we want to handle views-specific stuff.

Because of this, it's may be a good idea to add a getViewsDisplay method to the \Drupal\facets\FacetSource\SearchApiFacetSourceInterface class.
Because we use this class also for integration with the search_api_pages modules we have to find a way to do this that allows us to return FALSE or something similar from that method.

      $facet_source_display_id = $facet_source->getPluginDefinition()['display_id'];
      $search_api_display = \Drupal::service('plugin.manager.search_api.display')
        ->createInstance($facet_source_display_id);
      $search_api_display_definition = $search_api_display->getPluginDefinition();

      if (!empty($search_api_display_definition['view_id'])) {
        $view_id = $search_api_display_definition['view_id'];
        $view_display = $search_api_display_definition['view_display'];

        $view = Views::getView($view_id);
        $view->setDisplay($view_display);
     }

Todo:
- Discuss if we want this.
- Implement in the interface
- Implement the code
- Overwrite the current usages of this code to the new code
- Write an additional test.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

Issue tags: +beta blocker

Doing this will break the API, so we should do this before we tag the first beta.

borisson_’s picture

Status: Active » Needs review
FileSize
8.14 KB

Status: Needs review » Needs work

The last submitted patch, 3: api_break_add-2860393-3.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
640 bytes
8.14 KB

This is probably the 200th time I had this problem.

acbramley’s picture

Nit:

+++ b/src/Plugin/facets/facet_source/SearchApiDisplay.php
@@ -320,4 +330,25 @@ public function getDisplay() {
+    $view = Views::getView($view_id);

I believe you could use ViewsDisplayBase::getView here instead so

$this->getDisplay()->getView()->getExecutable()

Other than that it looks good, I can't help but think this may be better suited on the ViewsDisplayBase class though.

borisson_’s picture

Status: Needs review » Needs work

You're right, on both points. Back to needs work, I'll fix those tonight.

borisson_’s picture

Status: Needs work » Needs review
FileSize
645 bytes
7.89 KB

Actually, moving this to the ViewsDisplayBase class is not possible, as that would be a bc break for search api, that's why I'd prefer to keep this here.

I fixed the other remark.

Status: Needs review » Needs work

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

borisson_’s picture

Status: Needs work » Needs review
FileSize
7.93 KB

Reverted 8, can't use a protected method, making it public is an api break for search api.

borisson_’s picture

FileSize
2.73 KB
8.69 KB

Patch introduced coding standards issues. Thanks testbot!

acbramley’s picture

Status: Needs review » Reviewed & tested by the community

Actually, moving this to the ViewsDisplayBase class is not possible, as that would be a bc break for search api, that's why I'd prefer to keep this here.

Makes sense.

Reverted 8, can't use a protected method, making it public is an api break for search api.

Damn, that's a shame! Sorry I should've noticed that!

This is all good from my point of view then.

borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed, thanks for the review!

  • borisson_ committed 3cdc87e on 8.x-1.x
    Issue #2860393 by borisson_, acbramley: API break: Add getViewsDisplay...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.