Support block and rest displays in the display deriver

Hm, interesting. I guess if we do this, we should either rename the display plugin to just "Views" (not "Views Page"), or create two new plugins for blocks and REST displays. (I guess I'd prefer the second variant – and with some small change, they could still all use the same deriver.)
Also, while supporting REST displays makes perfect sense, I'd say blocks are a bit of a special case for a "search display" – specifically, because they have no fixed path. We even have a getPath() method on the interface (though I don't know why that's not getUrl()), currently with no option to return NULL – what should that be for blocks?
I guess it can make sense, on a site, to define a search in a block, and then of course you might still want facets (or other stuff, e.g., autocompletion) for that search – but then we probably have to re-think display plugins a bit. At the very least, that one method. Which is really a pity, with the module now being in Beta, but I guess that can't be helped.

We resolved this in facets like this, I just noticed that we don't do anything for rest displays.

OK, I guess that will work fine for facets functionality in most cases. And really nothing better we can do there, I'd say (except spy into the block settings to see if it's enabled only for a certain path – but that's hacky, unreliable and just overkill, I'd say).
However, for search display plugins, I think just returning the current path there is pretty misleading. If you want to, e.g., show a list of all known search pages, it would have a link to that list for block displays. Being able to just return NULL seems to make more sense to me in that case. Just have to deal with that case in your Search API facet source plugin, I guess.

• Change the interface to allow returning NULL as well as a string for the getPath() method
• Create an issue to deprecate getPath and use getUrl instead

getPath() doesn't return a string but a URL. That's why I want to rename it. But yeah, I guess we should deprecate it instead and remove it only in RC phase.

Otherwise, yeah, seems like this should do it. Thanks!
Do you already have a solution for isRenderedInCurrentRequest() for blocks?

Thanks a lot!
Here are a few style changes plus nit-picking, but generally this already looks quite good.

I'm wondering, though, whether we can keep the static caching as-is in the deriver, with $this->derivatives? Seems like that would cache the derivatives for one of the three display types and then return them for the other two, too. Did you test whether this really works like this? Otherwise, we'll probably have to go back (cf. #2758321: Clean up deriver code) to keying the cached definitions by base plugin ID, too.

If you have some time, could you maybe also add tests for this to the Views test? I guess you'd just need to add a block and a REST display to the view and check they have corresponding search display plugins, too.

1. +++ b/src/Plugin/search_api/display/ViewsDisplayDeriver.php
   @@ -0,0 +1,88 @@
   +      foreach ($all_views as $view) {
   

   foreach{ if { foreach { if { while {

   This is pretty complex to read. Can't we simplify this a bit so this becomes easier to read and easier to maintain?

2. +++ b/src/Plugin/search_api/display/ViewsDisplayDeriver.php
   @@ -0,0 +1,88 @@
   +              // Make sure the machine name is unique. (Will almost always be
   

   This is pretty scary to read :) So what if this is suddenly the case? Should we add a testcase for this?

Tagging RTBC. Because this is one of the last blockers to get FacetAPI back up and running I vote to get this committed unless someone shouts really loud with a clear reason why not to do this.

Committed. Thanks everyone!

Thanks for fixing this, and sorry I wasn't there to help.
Just have a few minor code style changes/corrections to propose for this.

Good to hear, thanks for reviewing!
Committed.