"Show all" result display only 10 items.

Re-rolling the patch for alpha3.

I'm raising the priority to major, because retrieving big results sets are a fundamental part of the module and it now gives surprising unexpected results.

Just to give it a bit of more context: The new hardcoded maximum is the default Elastic search maximum https://www.elastic.co/guide/en/elasticsearch/reference/current/index-mo...

index.max_result_window

Another workaround is to configure views with a limited number of results to 10000 instead of "Show all".

Re-rolling the patch for 8.0.x branch.

I am using this patch on some projects that use version 7 alpha3 and alpha4 of the module. Recently I noticed that when accessing some of the /admin/config/search/search-api/index/index_name pages (for large indexes) I was getting 500 errors, caused by PHP reaching its memory limit. After debuging, I found out that Search API themeing function (template_preprocess_search_api_index) does a query with offset 0 and limit 0 to check if the search index is alive. However, this patch turns that 0 rows limit into 10000 and that simple stats page tries to load tons of data.
I updated the patch to use !isset instead of empty. This will make a difference between Search API (using limit 0) and Display all page (using no limit, i.e. NULL).

I opted for Event subscriber solution without patching module:

// File my_module/src/EventSubscriber/SearchQueryAlterSubscriber.php

<?php

namespace Drupal\my_module\EventSubscriber;

use Drupal\elasticsearch_connector\Event\PrepareSearchQueryEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Event subscriber to alter search query config from ES connector.
 */
class SearchQueryAlterSubscriber implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    return [
      PrepareSearchQueryEvent::PREPARE_QUERY => ['onPrepareSearchQuery', 100],
    ];
  }

  /**
   * Sets the query limit to 1000.
   *
   * @param \Drupal\elasticsearch_connector\Event\PrepareSearchQueryEvent $event
   *   The event.
   */
  public function onPrepareSearchQuery(PrepareSearchQueryEvent $event) {
    // Get the Elasticsearch query from the event.
    /** @var \Drupal\elasticsearch_connector\Event\PrepareSearchQueryEvent $elastic_search_query */
    $elastic_search_query = $event->getElasticSearchQuery();
    $elastic_search_query['query_limit'] = 1000;

    // Set the altered query back to the event.
    $event->setElasticSearchQuery($elastic_search_query);
  }

}

// File my_module/my_module.services.yml

services:
  my_module.search_query_alter:
    class: Drupal\my_module\EventSubscriber\SearchQueryAlterSubscriber
    tags:
      - { name: event_subscriber }

My event subscriber actually wouldn't work because in `SearchBuilder.php` we already set `$query_limit` based on `$query_option` but inside `PrepareSearchQueryEvent` we don't have any original information from `$query_options` as we only pass `$elasticSearchQuery` and `$indexName`. If e.g. I want to check if query limit is set inside my event subscriber while `Display all items` is chosen in the view, I would get result that it is set and its value is 10. This isn't something I can use inside event subscriber therefore I would suggest that `getSearchQueryOptions()` needs to be changed and we either pass original query so it is accessible in an event subscriber or some other solution to pass relevant information from original query that is changed inside `getSearchQueryOptions()`.

I've created merge request !110 with the changes in the patch in #7.

All the tests pass (indicating no regressions to current functionality), and no lints: awesome!

I'm in the middle of preparing for a release, more specifically, ensuring that as many patches as possible will apply on the new release, i.e.: by resolving merge conflicts, i.e.: hopefully saving people time when the new release comes out. This means I'm only able to do a very quick code review (and use a few copy-pasted responses)! Off the top of my head, I think the approach in this merge request is good!

That being said, the new functionality will need tests, so I've added the "Needs tests" tag, and moved this back to "Needs work". Automated tests ultimately benefit you. They ensure that future changes (i.e.: by other contributors) will not break the functionality that you (or your client) depends on. If you need help writing tests, please ask (although I have a large backlog of tickets to work on, and I volunteer my time on the 8.x-7.x branch, so I cannot guarantee a speedy response - thanks in advance for understanding)!

We should also update the issue summary before release, so I've added the "Needs issue summary update" tag. An issue summary helps a maintainer understand why a change was necessary ("Problem/Motivation"), why you chose a particular solution ("Proposed resolution"), and how to test the change manually ("Steps to reproduce"). After an issue is fixed, a good issue summary documents what changed and what could be impacted ("User interface changes", "API changes", "Data model changes"), for people upgrading.

I've unchecked "Display" for the old patch files, because Testbot no longer tests them, and they no longer apply. If you need an updated patch file, you can download one from the merge request. For what its worth, the team at Lullabot recommends using local copies of patch files: from personal experience, local patch files often makes composer run faster and more reliably.