Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Leksat created an issue. See original summary.

Leksat’s picture

Version: 8.x-1.x-dev » 8.x-1.0-alpha1
Status: Active » Needs review
FileSize
17.79 KB

Here is a patch that extends the existing classes for that. Made on the base of 8.x-1.0-alpha.

Status: Needs review » Needs work

The last submitted patch, 2: 2678486-2.patch, failed testing.

The last submitted patch, 2: 2678486-2.patch, failed testing.

Leksat’s picture

FileSize
8.14 KB

Here is the same patch, but it's more readable since file renames are handled.

Leksat’s picture

Most probably we'll move to the dev version soon, so I will create a patch for 8.x-1.x shortly after that.

borisson_’s picture

I had already spent some time on a patch that's similar to this one in an older issue, but we closed that because there were more things going on in that issue.

#2598902: Add 'show only on a page that contains the facet source' setting and a second facetsource plugin to include view blocks

https://www.drupal.org/node/2598902#comment-10639420 was my latest patch, intead of merging both facet sources, what you've done here, I chose to add an extra facet source. I think the approach in this patch makes more sense as it leads to less duplicate code.

One thing I'm afraid about is the getPath and isRenderedInCurrentRequest methods. Not sure how those are going to work for blocks. We would love to get support for blocks, but this is not very easy, considering the fact that blocks can be rendered in isolation.

I'm looking forward to seeing a patch that applies to see how many of our current tests are going to break because of this.

borisson_’s picture

Version: 8.x-1.0-alpha1 » 8.x-1.x-dev
Status: Needs work » Needs review
FileSize
7.51 KB

I rerolled the patch so we can see what the bot thinks.

Status: Needs review » Needs work

The last submitted patch, 8: views_block_display-2678486-8.patch, failed testing.

The last submitted patch, 8: views_block_display-2678486-8.patch, failed testing.

Leksat’s picture

@borisson_, thanks for your reply and reroll!

Initially I also started with splitting up SearchApiViewsPage to SearchApiViews (as a base), SearchApiViewsPage, and SearchApiViewsBlock. But I met some issues. The biggest one is: there is an identifier that's used for two different terms:
- search id
- facet source id (I guess, facet plugin id is the same?)

For example, we have "search_api_view:my_view:my_display" id.
- as a search id it is built as"search_api_view:" . $view_name . ":" . $display_name
- as a facet source id: $facet_plugin_type . ":" . $view_name . ":" . $display_name

So, when I added plugin type "search_api_view_block", the source id became "search_api_view_block:my_view:my_display", and nothing working.

UPD: just checked your patch and found that you are aware of this :)

Leksat’s picture

Status: Needs work » Needs review
FileSize
8.1 KB

Just found that my rollout is a bit different. It also updates \Drupal\facets\Plugin\facets\url_processor\QueryString::buildUrls().

Leksat’s picture

Green light! :D

borisson_’s picture

Issue tags: +Needs tests

Awesome; looks like I made a mistake rebasing the patch. This looks great. All we need to do is add a view that has a block display to the tests and see if the integration tests also work with that block. I'll have a stab at that today.

borisson_’s picture

FileSize
3.35 KB
21.68 KB

So, this adds a basic integration test for the functionality. The rendered in current request functionality is not tested, because it's not expected to work yet.

Not sure if we should just mention this in the readme / code and be done with it or if we should do something else with that. I'll ping @nick_vh, he might remember more from that discussion.

Status: Needs review » Needs work

The last submitted patch, 15: views_block_display-2678486-15.patch, failed testing.

The last submitted patch, 15: views_block_display-2678486-15.patch, failed testing.

Nick_vh’s picture

Overall I agree with the approach, but it does lack some information as to why we initially only supported pages. It's also good that this is limited to blocks but it should make that more clear in the description but also in the UI of facets. I am sure if there is another view display that is just like a page but has a different name, people will get confused because they are not seeing that facet source in the list.

  1. +++ b/src/Plugin/facets/facet_source/SearchApiViews.php
    @@ -0,0 +1,167 @@
    + * Represents a facet source which represents the search api views.
    

    Let's be very exact in our description what this does. It represents a facet source which has support for search_api_views pages and blocks.

  2. +++ b/src/Plugin/facets/facet_source/SearchApiViewsDeriver.php
    @@ -0,0 +1,64 @@
    +            if (in_array($display_info['display_plugin'], ['page', 'block'])) {
    

    Also where we see the if, it should give a much better answer as to why we are only supporting those two. Why not other view modes. Do we ever want to make this configurable so that people can change this at their own risk, say integration modules that add support for other view displays?

  3. +++ b/src/Plugin/facets/facet_source/SearchApiViews.php
    @@ -0,0 +1,167 @@
    +        return TRUE;
    

    This should be FALSE, if we are uncertain let's make sure we do not fool anyone and do not give it the benefit of the doubt. Also, it would be good to add an explanation why it is not possible to know if a block is embedded on a page.

  4. +++ b/src/Plugin/facets/facet_source/SearchApiViewsDeriver.php
    @@ -0,0 +1,64 @@
    + * Derives a facet source plugin definition for every search api view.
    

    Same here, add description that this only derives a facet source for search_api_views pages and blocks.

  5. +++ b/src/Plugin/facets/facet_source/SearchApiViewsDeriver.php
    @@ -0,0 +1,64 @@
    +              $machine_name = $view->id() . PluginBase::DERIVATIVE_SEPARATOR . $name;
    

    Is this really the "name" or is it the $display_id / $id?

  6. +++ b/src/Plugin/facets/facet_source/SearchApiViewsDeriver.php
    @@ -0,0 +1,64 @@
    +                'label' => $this->t('Search api view: %view_name, display: %display_title', ['%view_name' => $view->label(), '%display_title' => $display_info['display_title']]),
    

    Should we also add in the label whether it came from a block or a page? Eg add in the $display_info['display_plugin']?

  7. +++ b/src/Plugin/facets/facet_source/SearchApiViewsDeriver.php
    @@ -0,0 +1,64 @@
    +                'Search api view: %view, display: %display',
    

    Silly other remark, do we refer to "Search api" or "Search API" or "Search Api". Would be good to be consistent with what search_api calls itself.

  8. +++ b/src/Plugin/facets/url_processor/QueryString.php
    @@ -68,7 +68,7 @@ class QueryString extends UrlProcessorPluginBase {
    +      $request = Request::create($facet->getFacetSource()->getPath());
    

    is this going to create issues? There's probably a reason why the slash was added?

borisson_’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.05 KB
22.75 KB

Fixed the test fail by making sure to use the correct facet source for testing. Also added extra comments in the isRenderedInCurrentRequest method. to fix #18.3

I'll look at the other remarks now. Thanks for the awesome review.

Status: Needs review » Needs work

The last submitted patch, 19: views_block_display-2678486-18.patch, failed testing.

The last submitted patch, 19: views_block_display-2678486-18.patch, failed testing.

Leksat’s picture

Actually, I think that we can support all view display types:
- instead checking $display_type == 'page', we can check \Drupal\views\Plugin\views\display\DisplayPluginInterface::hasPath()
- isRenderedInCurrentRequest() also can use display's hasPath() + getPath() to determine the visibility

One side effect could be that /admin/config/search/facets page will become huge. But we can hide sources having zero facets, right?

borisson_’s picture

Status: Needs work » Needs review
FileSize
3.16 KB
23.46 KB

#18

  1. Fixed.
  2. If there are other view modes to support, that can happen trough a patch/issue here or a copy of the complete source with custom logic, let's not make sources harder to understand than they already are.
  3. I agree, fixed in #19
  4. Fixed.
  5. Fixed.
  6. Sure, fixed.
  7. Pretty sure that's inconsistent everywhere, created a followup issue #2679345: Be consistent in code about Search api casing
  8. Not sure, as long as the tests pass I don't think there should be any trouble. But yeah, let's be very careful for that.

Tests will still fail, I'll have another look at that now.

Status: Needs review » Needs work

The last submitted patch, 23: views_block_display-2678486-20.patch, failed testing.

The last submitted patch, 23: views_block_display-2678486-20.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
874 bytes
24.31 KB

Test should be green now.

borisson_’s picture

Actually, I think that we can support all view display types:
- instead checking $display_type == 'page', we can check \Drupal\views\Plugin\views\display\DisplayPluginInterface::hasPath()
- isRenderedInCurrentRequest() also can use display's hasPath() + getPath() to determine the visibility

This is very interesting, but I think I'd prefer to be conservative here, if another module adds a new display type, that doesn't mean we can automatically support that. (e.g. data export)

Status: Needs review » Needs work

The last submitted patch, 26: views_block_display-2678486-26.patch, failed testing.

The last submitted patch, 26: views_block_display-2678486-26.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
505 bytes
24.33 KB
5.83 KB

And another test broke, this fixes that one. I also added an interdiff to #15.

Leksat’s picture

About slashes in URLs.
As far as I can see, Drupal 8 uses leading slash in URLs in all methods. Probably, views is the last module where this was not changed.

Nick_vh’s picture

So that means that change needs to be reverted in the patch or?

Leksat’s picture

I'd prefer if facets behaves the same as most of D8 core, so uses leading slashes in URLs. That's why I changed \Drupal\facets\Plugin\facets\facet_source\SearchApiViews::getPath() to return an already slash-prefixed path.

Leksat’s picture

FYI, created a core issue #2680209: [meta] Advanced search/view with advanced layout (draft).

A use-case: a page_manager page contains a search_api_view view block, a fulltext search field (in exposed form block), facet blocks.

borisson_’s picture

FileSize
24.63 KB

This needed a reroll.

Status: Needs review » Needs work

The last submitted patch, 35: views_block_display-2678486-35.patch, failed testing.

The last submitted patch, 35: views_block_display-2678486-35.patch, failed testing.

borisson_’s picture

FileSize
1.41 KB
26.04 KB

Looks like something I committed today broke the patch in #30. I can't seem to find why - yet.

borisson_’s picture

Status: Needs work » Needs review

Setting to NR for the bot.

Status: Needs review » Needs work

The last submitted patch, 38: views_block_display-2678486-38.patch, failed testing.

The last submitted patch, 38: views_block_display-2678486-38.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
778 bytes
26.37 KB
borisson_’s picture

FileSize
26.38 KB

Needed reroll.

borisson_’s picture

FileSize
16.61 KB

I rolled a new patch after the last commits and created this patch with git diff -M to make the patch smaller. I'm pretty sure this is ready - if we can get someone to set this to RTBC we can commit this.

Status: Needs review » Needs work

The last submitted patch, 44: views_block_display-2678486-44.patch, failed testing.

The last submitted patch, 44: views_block_display-2678486-44.patch, failed testing.

borisson_’s picture

Assigned: Unassigned » borisson_

Looks like I was wrong in #44 saying that his was almost done, I'll try to fix the test-failures.

borisson_’s picture

Assigned: borisson_ » Unassigned
Status: Needs work » Needs review
FileSize
544 bytes
16.63 KB

This should fix the test-failure.

borisson_’s picture

FileSize
1.4 KB
16.56 KB

I rerolled this patch again.

ademarco’s picture

Patch at #49 works great for me, thanks!

ademarco’s picture

Status: Needs review » Reviewed & tested by the community

  • borisson_ committed 196a64e on 8.x-1.x
    Issue #2678486 by borisson_, Leksat, Nick_vh: Views block display...
borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks for the reviews / patches.

Status: Fixed » Closed (fixed)

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