Separate out NodeSearch::execute() into finding vs. processing results

Looks like a cool idea.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -258,10 +268,29 @@ public function execute() {
+    if (!$found) {

Should we strictly compare $found to null?

I don't have any other remarks on this. If core maintainers are happy with #9 and #10, this is RTBC for me.

If I understood #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? correctly, this is acceptable in the beta period and even could be raised as critical as contrib needs ugly workarounds.

Note: +1 on this commit - I've been working on DS search during DrupalCon and this patch makes our code ten times more cleaner :)

How about changing the patch like this (a diff on top of the current patch)?

diff --git a/core/modules/node/src/Plugin/Search/NodeSearch.php b/core/modules/node/src/Plugin/Search/NodeSearch.php
index 0cf8b80..3fdd1fb 100644
--- a/core/modules/node/src/Plugin/Search/NodeSearch.php
+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -13,6 +13,7 @@
 use Drupal\Core\Config\Config;
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Database\Query\SelectExtender;
+use Drupal\Core\Database\StatementInterface;
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\Core\Form\FormStateInterface;
@@ -166,20 +167,20 @@ public function isSearchExecutable() {
    * {@inheritdoc}
    */
   public function execute() {
-    $found = $this->findResults();
-    return $this->prepareResults($found);
+    $results = array();
+    if ($this->isSearchExecutable()) {
+      $results = $this->prepareResults($this->findResults());
+    }
+    return $results;
   }
 
   /**
    * Queries to find search results, and sets status messages.
    *
-   * @return \Drupal\Core\Database\StatementInterface|null
-   *   A search query result, or NULL if the query could not be run.
+   * @return \Drupal\Core\Database\StatementInterface
+   *   A search query result.
    */
   protected function findResults() {
-    if (!$this->isSearchExecutable()) {
-      return NULL;
-    }
     $keys = $this->keywords;
 
     // Build matching conditions.
@@ -265,18 +266,14 @@ protected function findResults() {
   /**
    * Prepares search results for rendering.
    *
-   * @param \Drupal\Core\Database\StatementInterface|null $found
-   *   Results found from the search query, or NULL if query failed.
+   * @param \Drupal\Core\Database\StatementInterface $found
+   *   Results found from the search query.
    *
    * @return array
    *   Array of search result item render arrays (empty array if no results).
    */
-  protected function prepareResults($found) {
+  protected function prepareResults(StatementInterface $found) {
     $results = array();
-    if (!$found) {
-      return $results;
-    }
-
     $node_storage = $this->entityManager->getStorage('node');
     $node_render = $this->entityManager->getViewBuilder('node');
     $keys = $this->keywords;

This way we have less conditions in the protected methods and stronger type hinting - making it easier to override and do the right thing.

I'm fine with this.

re #16 I don't understand what you mean. The point was to not random arrays of stuff between methods but objects with interfaces. The docs on the method say it all:
Results found from the search query, or NULL if query failed.
Well why call the method then? The point was not for simplification or performance it was for

This way we have less conditions in the protected methods and stronger type hinting - making it easier to override and do the right thing.

With the patch in #20 all implementations have to begin with

protected function prepareResults($found) {
  $results = array();
  if (!$found) {
    return $results;
  }

Why?

Looks good

Committed bd02a26 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.