ViewsBlock::isRenderedInCurrentRequest() returns false negative when the block view is placed through layout builder

Thanks a lot for reporting this problem and already providing code to address it!

Please don’t use issue forks in this project, though, as the d.o tech team hasn’t been able to get automated tests working for them correctly yet. (Cf. #3190024: Problem with test dependencies when testing issue forks.)
I’m reposting your code as a patch, along with some modifications by me. (I prefer a slightly different style of DI in this module, which I like to keep consistent, and I also think the much “quicker” check for whether blocks are displayed should probably go before the more complex Layout Builder checks.

In any case, as I’m not using Layout Builder I cannot verify whether this issue a) actually exists and is b) fixed by your MR.
Would you be able to provide automated test coverage for this bug, so we can also make sure this keeps on working in the future?

Also, input from others would be very much appreciated, too.
Finally, do you think this could negatively impact performance, or should those additional checks go reasonably quickly?

NW for the tests.

The patch works fine if an entity has overridden layout builder, it doesn't work for default layout builder. However, the layout builder is only one scenario here, there are cases where the block is placed through the layout paragraphs or block reference field on entity and it doesn't work well. I think we should provide a hook to allow other modules (custom or contrib) to modify the status based on the logic.

Updated patch

@ miteshmap: Thanks for the suggestion, this also sounds sensible. However, I’m not sure whether to move this issue to that approach, or whether to first fix it for the main use case and then provide more flexibility in a follow-up. Does anyone else have an opinion on this?

In any case, we definitely won’t introduce a new hook, but an event, as we’re currently moving away from hooks (see #3023704: Convert hooks to events).
Also, if we add an event for this in this issue, we should then also provide a listener that fixes the actual problem discussed here. (Or keep the code solving this for layout builder right in that method, as done in #5 – but I think I’d then tend to add the event in a separate issue.)

Hi @miteshmap:

Thank You for the patch, but patch #8 is conflicting with the latest Security Update for the Search API module https://www.drupal.org/sa-contrib-2022-059

After Updating the security update I'm getting this Fatal Error throughout the site:

Fatal error: Cannot use Symfony\Component\DependencyInjection\ContainerInterface as ContainerInterface because the name is already in use in /var/www/html/docroot/modules/contrib/search_api/src/Plugin/search_api/display/ViewsBlock.php on line 13

Thank You!

Coming from #3315228: ViewsBlock::isRenderedInCurrentRequest() fails for Layout Builder .

I think that there're multiple issues in the facets issue queue that might be related as layout builder is used more and more.

I second drunken monkey to directly add the layout builder detection logic as a function instead of using a hook or event.

Thanks @drunken-monkey and @mkalkbrenner

I agree with adding the logic function to detect layout builder. But, we will also need Events (As we are moving away from hooks) to allow other modules to customise the logic.

There are other contrib modules that allows users to add blocks to layout builder. We may not be able to cover all the possible ways of how those module handles it. That's why I think we definitely need events (that can be a separate task).

Adding the event for 3rd party makes sense. But we should not subscribe to our own events.

OK, then let’s go ahead with this. I’m fine with adding the event in this issue, too, if it doesn’t delay the commit. Otherwise we’ll do that in a separate issue.
However, in any case, this still needs tests before I can commit it. Is anyone able to write some? As I have no experience with Layout Builder, I wouldn’t quite know where to start.

Attaching a re-roll of #5.

NW for the tests.

I got bug reports of a massive performance issue on one of our production sites. It tuned out that the reason was a facet_summary added as block via layout builder. Basically that block gets partly rendered on every page. So this patch is one part required of the solution.

But it turns out that the event is also required, because that logic is too simple:

        if ($param instanceof ContentEntityInterface
            && $param->hasField(OverridesSectionStorage::FIELD_NAME)
            && !$param->get(OverridesSectionStorage::FIELD_NAME)->isEmpty()) {

In our case the referenced entity in $param is not the one being rendered, but a referenced one. This happens through custom code. So we need the event to add our logic here, too.

I added the event. Implementing the subscriber gave our site a massive performance boost by avoiding to build our complex facets on every page.

Thanks for the additional code, looks pretty good. And good to know the event is also pretty important, then of course let’s add that, too.

1. +++ b/src/Plugin/search_api/display/ViewsBlock.php
   @@ -107,12 +111,13 @@
          if ($block->access('view')) {
   -        return TRUE;
   +        $is_rendered_in_current_request = TRUE;
   +        break;
          }
   

   Do we really also need the event in case of a visible block? Shouldn’t that always be accurate?

   Similarly, do we also want to send the event if Layout Builder module isn’t present? It seems to me like this would add needless overhead for simple sites, where we’d now dispatch one event for each facet (based on a Views block display) on every page.

2. +++ b/src/Event/SearchApiEvents.php
   @@ -209,4 +209,16 @@ final class SearchApiEvents {
   +   * Event fired when detecting if display is rendered in current request.
   +   *
   +   * When detecting if a display is rendered in current request the decision
   +   * could be overridden within this event..
   

   I think this is too unspecific, if we’re only gonna fire it for Views blocks (or, worse, only for Views blocks under certain circumstances). People might wonder why the event never fires for them.

3. +++ b/src/Event/IsRenderedInCurrentRequestEvent.php
   @@ -0,0 +1,87 @@
   +  /**
   +   * The display plugin ID.
   +   *
   +   * @var string
   +   */
   +  protected $pluginId;
   

   The way you currently initialize this, it is not the display plugin ID, but the block plugin ID. However, I don’t think we should include that, as it would make the event specific for Views blocks, with no clean way of later extending it to other use cases. Including the display plugin ID, on the other hand, would be pretty pointless as you can get that with $event->getDisplay()->getPluginId().

These and a few code style nitpicks would be changed/fixed by the attached patch revision.

Still needs tests, either way.

+++ b/src/Plugin/search_api/display/ViewsBlock.php
@@ -107,12 +111,13 @@
if ($block->access('view')) {
- return TRUE;
+ $is_rendered_in_current_request = TRUE;
+ break;
}

Do we really also need the event in case of a visible block? Shouldn’t that always be accurate?

Similarly, do we also want to send the event if Layout Builder module isn’t present? It seems to me like this would add needless overhead for simple sites, where we’d now dispatch one event for each facet (based on a Views block display) on every page.

I'm of the opinion that an event should always be fired. The way I implemented it you can use the event to overwrite the "TRUE" decision, too.
And I think that this is a valid use case for a search result block. In opposite to a page, such a block can be added on different pages and some might contain facets while others don't. Think about search results as part of simple news newsletters or RSS feeds. I know that these aren't common use-cases. But if you do such things, there's no block layout where you can place a condition to omit the facet. For sure it won't be rendered, but it will be populated with results! And if you also take facets features like combined facets into account, a "complete" API becomes valuable.

+++ b/src/Event/SearchApiEvents.php
@@ -209,4 +209,16 @@ final class SearchApiEvents {
+ * Event fired when detecting if display is rendered in current request.
+ *
+ * When detecting if a display is rendered in current request the decision
+ * could be overridden within this event..

I think this is too unspecific, if we’re only gonna fire it for Views blocks (or, worse, only for Views blocks under certain circumstances). People might wonder why the event never fires for them.

As described above, firing the event only under some circumstances in not a good design.
But I agree that we should limit it to blocks as the problem is specific to block displays.

The way you currently initialize this, it is not the display plugin ID, but the block plugin ID. However, I don’t think we should include that, as it would make the event specific for Views blocks, with no clean way of later extending it to other use cases. Including the display plugin ID, on the other hand, would be pretty pointless as you can get that with $event->getDisplay()->getPluginId().

OK, I agree to remove the pluginId.

I'm of the opinion that an event should always be fired. The way I implemented it you can use the event to overwrite the "TRUE" decision, too.
And I think that this is a valid use case for a search result block. In opposite to a page, such a block can be added on different pages and some might contain facets while others don't. Think about search results as part of simple news newsletters or RSS feeds. I know that these aren't common use-cases. But if you do such things, there's no block layout where you can place a condition to omit the facet. For sure it won't be rendered, but it will be populated with results! And if you also take facets features like combined facets into account, a "complete" API becomes valuable.

If I understand you correctly, you’re arguing that even though the search block is displayed, a user might still want to hide facets?
In that case, I’d argue that this is the wrong place to implement that functionality. The method is called isRenderedInCurrentRequest(), not shouldFacetsBeRendered() – if the search block will be displayed, this method should return TRUE. The method can, after all, also be used by other modules than Facets. If a user doesn’t want to display a facet for a search that is present on the page, they should implement that functionality using Facets, not by “hiding” the search.
Is there no way to just hide a facet for specific pages, or alter whether a facet is computed in a given request?

In any case, I’m not against also firing the event when the result is TRUE, but against firing it when we are sure about the result. And I think if we detect that a block is displayed using Drupal’s normal functionality, then that result will be reliable.

The issue in facets is that facets are always filled with results before any decision takes place. Depending on the facet, that could be very expensive.
At the moment isRenderedInCurrentRequest() returning FALSE is the only way to terminate facets processing early.

But I agree that this issue should be solved in facets itself, but it might require architectual changes. Maybe we should introduce a "terminate early" event in facets.

OK, then I’d suggest trying that approach, within the Facets module, first, and only resolving it via isRenderedInCurrentRequest() as a last ressort, in case implementing this in Facets would not be feasible for some reason. Is there already an issue in the Facets queue?

How do you propose we proceed in this issue, then? I guess the patch in #19 still makes sense?

Yes, we should go with #19.

There're multiple issues within the facets queue. But we need to consolidate them.

Re-roll the patch for #8 as previous patch conflict with Latest version 8.x-1.28

I finally got round to writing a test. Please test/review so we can finally resolve this issue, at least from the Search API point of view.
(Tests-only patch doubles as the interdiff, I didn’t change any non-test code.)

Long time since I added a whole new test class, apparently …

Does anyone time to test/review?

Verified the patch and works fine as expected, Good to merge!

Good to hear, thanks a lot for testing and reporting back!
Committed. Thanks again, everyone!