Refactor record loader to use helper classes/methods.

- Improves code clarity.

module/VuFind/src/VuFind/Record/Loader.php

@@ -71,9 +71,9 @@ class Loader implements \Zend\Log\LoggerAwareInterface
     /**
      * Constructor
      *
-     * @param SearchService $searchService Search service
-     * @param RecordFactory $recordFactory Record loader
-     * @param Cache         $recordCache   Record Cache
+     * @param SearchService  $searchService  Search service
+     * @param RecordFactory  $recordFactory  Record loader
+     * @param Cache          $recordCache    Record Cache
      */
     public function __construct(SearchService $searchService,
         RecordFactory $recordFactory, Cache $recordCache = null
@@ -145,25 +145,23 @@ class Loader implements \Zend\Log\LoggerAwareInterface
     public function loadBatchForSource($ids, $source = DEFAULT_SEARCH_BACKEND,
         $tolerateBackendExceptions = false
     ) {
+        $list = new Checklist($ids);
         $cachedRecords = [];
         if (null !== $this->recordCache && $this->recordCache->isPrimary($source)) {
             // Try to load records from cache if source is cachable
             $cachedRecords = $this->recordCache->lookupBatch($ids, $source);
             // Check which records could not be loaded from the record cache
             foreach ($cachedRecords as $cachedRecord) {
-                $key = array_search($cachedRecord->getUniqueId(), $ids);
-                if ($key !== false) {
-                    unset($ids[$key]);
-                }
+                $list->check($cachedRecord->getUniqueId());
             }
         }
 
         // Try to load the uncached records from the original $source
         $genuineRecords = [];
-        if (!empty($ids)) {
+        if ($list->hasUnchecked()) {
             try {
-                $genuineRecords = $this->searchService->retrieveBatch($source, $ids)
-                    ->getRecords();
+                $genuineRecords = $this->searchService
+                    ->retrieveBatch($source, $list->getUnchecked())->getRecords();
             } catch (\VuFindSearch\Backend\Exception\BackendException $e) {
                 if (!$tolerateBackendExceptions) {
                     throw $e;
@@ -175,18 +173,16 @@ class Loader implements \Zend\Log\LoggerAwareInterface
             }
 
             foreach ($genuineRecords as $genuineRecord) {
-                $key = array_search($genuineRecord->getUniqueId(), $ids);
-                if ($key !== false) {
-                    unset($ids[$key]);
-                }
+                $list->check($genuineRecord->getUniqueId());
             }
         }
 
-        if (!empty($ids) && null !== $this->recordCache
+        if ($list->hasUnchecked() && null !== $this->recordCache
             && $this->recordCache->isFallback($source)
         ) {
             // Try to load missing records from cache if source is cachable
-            $cachedRecords = $this->recordCache->lookupBatch($ids, $source);
+            $cachedRecords = $this->recordCache
+                ->lookupBatch($list->getUnchecked(), $source);
         }
 
         // Merge records found in cache and records loaded from original $source
@@ -198,6 +194,24 @@ class Loader implements \Zend\Log\LoggerAwareInterface
         return $retVal;
     }
 
+    /**
+     * Build a "missing record" driver.
+     *
+     * @param array $details Associative array of record details (from a
+     * SourceAndIdList)
+     *
+     * @return \VuFind\RecordDriver\Missing
+     */
+    protected function buildMissingRecord($details)
+    {
+        $fields = $details['extra_fields'] ?? [];
+        $fields['id'] = $details['id'];
+        $record = $this->recordFactory->get('Missing');
+        $record->setRawData($fields);
+        $record->setSourceIdentifier($details['source']);
+        return $record;
+    }
+
     /**
      * Given an array of associative arrays with id and source keys (or pipe-
      * separated source|id strings), load all of the requested records in the
@@ -217,48 +231,28 @@ class Loader implements \Zend\Log\LoggerAwareInterface
      */
     public function loadBatch($ids, $tolerateBackendExceptions = false)
     {
-        // Sort the IDs by source -- we'll create an associative array indexed by
-        // source and record ID which points to the desired position of the indexed
-        // record in the final return array:
-        $idBySource = [];
-        foreach ($ids as $i => $details) {
-            // Convert source|id string to array if necessary:
-            if (!is_array($details)) {
-                $parts = explode('|', $details, 2);
-                $ids[$i] = $details = [
-                    'source' => $parts[0], 'id' => $parts[1]
-                ];
-            }
-            $idBySource[$details['source']][$details['id']] = $i;
-        }
+        // Create a SourceAndIdList object to help sort the IDs by source:
+        $list = new SourceAndIdList($ids);
 
         // Retrieve the records and put them back in order:
         $retVal = [];
-        foreach ($idBySource as $source => $details) {
+        foreach ($list->getIdsBySource() as $source => $currentIds) {
             $records = $this->loadBatchForSource(
-                array_keys($details), $source, $tolerateBackendExceptions
+                $currentIds, $source, $tolerateBackendExceptions
             );
             foreach ($records as $current) {
-                $id = $current->getUniqueId();
-                // In theory, we should be able to assume that $details[$id] is
-                // set... but in practice, we can't make that assumption. In some
-                // cases, Summon IDs will change, and requests for an old ID value
-                // will return a record with a different ID.
-                if (isset($details[$id])) {
-                    $retVal[$details[$id]] = $current;
+                $position = $list->getRecordPosition($current);
+                if ($position !== false) {
+                    $retVal[$position] = $current;
                 }
             }
         }
 
         // Check for missing records and fill gaps with \VuFind\RecordDriver\Missing
         // objects:
-        foreach ($ids as $i => $details) {
+        foreach ($list->getAll() as $i => $details) {
             if (!isset($retVal[$i]) || !is_object($retVal[$i])) {
-                $fields = $details['extra_fields'] ?? [];
-                $fields['id'] = $details['id'];
-                $retVal[$i] = $this->recordFactory->get('Missing');
-                $retVal[$i]->setRawData($fields);
-                $retVal[$i]->setSourceIdentifier($details['source']);
+                $retVal[$i] = $this->buildMissingRecord($details);
             }
         }
 

module/VuFind/src/VuFind/Record/SourceAndIdList.php

+<?php
+/**
+ * Record ID list (support class for Loader)
+ *
+ * PHP version 7
+ *
+ * Copyright (C) Villanova University 2018.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ * @category VuFind
+ * @package  Record
+ * @author   Demian Katz <demian.katz@villanova.edu>
+ * @license  http://opensource.org/licenses/gpl-2.0.php GNU General Public License
+ * @link     https://vufind.org Main Site
+ */
+namespace VuFind\Record;
+
+use VuFind\RecordDriver\AbstractBase as Record;
+
+/**
+ * Record ID list (support class for Loader)
+ *
+ * @category VuFind
+ * @package  Record
+ * @author   Demian Katz <demian.katz@villanova.edu>
+ * @license  http://opensource.org/licenses/gpl-2.0.php GNU General Public License
+ * @link     https://vufind.org Main Site
+ */
+class SourceAndIdList
+{
+    /**
+     * Processed ID data.
+     *
+     * @var array
+     */
+    protected $ids = [];
+
+    /**
+     * Record positions in the original list, indexed by source and ID.
+     *
+     * @var array
+     */
+    protected $bySource = [];
+
+    /**
+     * Constructor
+     *
+     * @param array $ids Array of associative arrays with id/source keys or strings
+     * in source|id format. In associative array formats, there is also an optional
+     * "extra_fields" key which can be used to pass in data formatted as if it
+     * belongs to the Solr schema; this is used to create a mock driver object if
+     * the real data source is unavailable.
+     */
+    public function __construct($ids)
+    {
+        // Sort the IDs by source -- we'll create an associative array indexed by
+        // source and record ID which points to the desired position of the indexed
+        // record in the final return array:
+        foreach ($ids as $i => $details) {
+            // Convert source|id string to array if necessary:
+            if (!is_array($details)) {
+                $parts = explode('|', $details, 2);
+                $ids[$i] = $details = [
+                    'source' => $parts[0], 'id' => $parts[1]
+                ];
+            }
+            $this->bySource[$details['source']][$details['id']] = $i;
+        }
+        $this->ids = $ids;
+    }
+
+    /**
+     * Get the full list of IDs sent to the constructor, normalized to array
+     * format.
+     *
+     * @return array
+     */
+    public function getAll()
+    {
+        return $this->ids;
+    }
+
+    /**
+     * Get an associative source => id list array.
+     *
+     * @return array
+     */
+    public function getIdsBySource()
+    {
+        return array_map('array_keys', $this->bySource);
+    }
+
+    /**
+     * If the provided record driver corresponds with an ID in the list, return
+     * the associated position in the list. Otherwise, return false.
+     *
+     * @param Record $record Record
+     *
+     * @return int|bool
+     */
+    public function getRecordPosition(Record $record)
+    {
+        $id = $record->getUniqueId();
+        $source = $record->getSourceIdentifier();
+
+        // First check if the primary ID is set; in some cases (e.g. Summon),
+        // the ID may have changed, so also check the prior ID if available.
+        if (isset($this->bySource[$source][$id])) {
+            return $this->bySource[$source][$id];
+        }
+        $oldId = $record->tryMethod('getPreviousUniqueId');
+        if ($oldId !== null && isset($this->bySource[$source][$oldId])) {
+            return $this->bySource[$source][$oldId];
+        }
+        return false;
+    }
+}