2
\$\begingroup\$

I've written a module whose purpose is to allow a user to simply add their email and click 'Save Search'. The details of the search are stored in two straight MySql tables which are not in any way related to the rest of the tables which are used by Drupal.

The class is close to complete (Dotmailer API integration needs to be done) and I've been applying PSR-2 standards as best as possible but my question regarding the class are:

1) Over all, am I doing anything drastically wrong and or what bad practices exist that I can correct?

I simply want to improve my overall PHP coding and any tips/constructive crit would be welcomed.

Latest PHP version is been used, all SQL queries use the Drupal 8 DB API.

Many thanks

<?php
declare(strict_types=1);

namespace Drupal\lrg_search_saves;

use Drupal;
use Drupal\Core\Database\Database;
use Drupal\lrg_search_form\PropertyCard;
use Drupal\lrg_search_form\PropertySearch;
use Drupal\lrg_search_form\PropertyTypes;
use Drupal\node\Entity\Node;

use Drupal\lrg_search_form\Validators\{
    Validator,
    Integer,
    Options,
    NoValidation
};
use InvalidArgumentException;

class SearchesSaved
{
    private $db;
    private $searchParam;
    private $debugDrush;
    private $dotMailerSearchPackets;
    private $base_url;

    private function __construct()
    {
        $this->db = Database::getConnection();

        $this->searchParam = self::initSearchParamsArray();

        $this->debugDrush = false;
        $this->dotMailerSearchPackets = [];

        $this->base_url = \Drupal::request()->getHost();

        // For localhost testing, force port number used by docker container
        if ($this->base_url === 'localhost') {
            $this->base_url = 'http://localhost:81';
        }
    }

    /**
     * Factory method
     * -----------------------------------------------------------------------------------------------------------------
     * @return SearchesSaved
     */
    public static function create(): SearchesSaved
    {
        return new SearchesSaved();
    }

    /**
     * Init the search parameters array
     * -----------------------------------------------------------------------------------------------------------------
     * @return array
     */
    private function initSearchParamsArray(): array
    {
        return [
            'search_type' => new Options('buy', 'rent'),
            'search_location_key' => new NoValidation(),
            'search_location' => new NoValidation(),
            'buying_min_price' => new Integer(0, 0),
            'buying_max_price' => new Integer(0, 0),
            'renting_min_price' => new Integer(0, 0),
            'renting_max_price' => new Integer(0, 0),
            'min_bedrooms' => new Integer(0, 6),
            'property_type' => new Options('Any', 'Terraced House', 'Semi Detached House', 'Detached House',
                'Flat / Apartment', 'Bungalow'),
            'filter_options' => new Options('Any', 'INVESTMENT', 'AUCTION', 'NEW_HOMES', 'COMMERCIAL',
                'SHARED_OWNERSHIP')
        ];
    }

    /**
     * Set a specific search parameter
     * -----------------------------------------------------------------------------------------------------------------
     * @param $paramName
     * @param $paramValue
     */
    private function setInitSearchParam($paramName, $paramValue)
    {
        if (array_key_exists($paramName, $this->searchParam)) {
            $this->searchParam[$paramName] = $paramValue;
        }
    }

    /**
     * Get the search parameters from the querystring and validate them and store
     * -----------------------------------------------------------------------------------------------------------------
     * @return array
     */
    public function setSearchParams(): void
    {
        $queryArgs = [
            'search_type' => new Options('buy', 'rent'),
            'search_location_key' => new NoValidation(),
            'search_location' => new NoValidation(),
            'buying_min_price' => new Integer(0, 9000000),
            'buying_max_price' => new Integer(0, 9000000),
            'renting_min_price' => new Integer(0, 8000),
            'renting_max_price' => new Integer(0, 8000),
            'min_bedrooms' => new Integer(0, 6),
            'property_type' => new Options('Any', 'Terraced House', 'Semi Detached House', 'Detached House',
                'Flat / Apartment', 'Bungalow'),
            'filter_options' => new Options('Any', 'INVESTMENT', 'AUCTION', 'NEW_HOMES', 'COMMERCIAL',
                'SHARED_OWNERSHIP'),
        ];

        $args = [];

        foreach ($queryArgs as $arg => $validator) {
            $args[$arg] = $validator->sanitise(Drupal::request()->query->get($arg));
        }

        $this->searchParam = $args;

    }

    /**
     * Check the search parameters are within specifications
     * -----------------------------------------------------------------------------------------------------------------
     * @return bool
     */
    private function validateSearchParams(): bool
    {
        if (is_null($this->searchParam['search_location'])) {
            return false;
        }

        if (!is_string($this->searchParam['search_location'])) {
            return false;
        }

        if (strlen($this->searchParam['search_location']) <= 0) {
            return false;
        }

        return true;
    }

    /**
     * Unsubscribe based upon hash and email address
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $hash
     * @param string $email_address
     * @return int
     */
    public function unsubscribe(string $hash, string $email_address): ?int
    {
        $searchUser = null;

        $status = 0;

        try {
            $searchUser = $this->dbGetSearchUserByEmail($email_address);
        } catch (\Exception $ex) {
            $status = 5;
        }

        if (isset($searchUser)) {
            $unsubscribeAllSavedSearches = ($searchUser['unique_hash'] === $hash);
            $status = $this->dbDeleteSearch($hash, $email_address, $unsubscribeAllSavedSearches);
        }

        return $status;
    }

    /**
     * Conduct a search against DB
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $hash
     * @param string $email
     * @return array|null
     */
    public function runSearchByHash(string $hash): ?array
    {
        $propertyTypes = new PropertyTypes();

        $savedSearchData = $this->dbGetSearchDataForHash($hash);

        $paramArgs = [
            'search_type' => $savedSearchData['param_search_type'],
            'search_location_key' => $savedSearchData['param_search_location_key'],
            'search_location' => $savedSearchData['param_search_location'],
            'buying_min_price' => (int)$savedSearchData['param_buying_min_price'],
            'buying_max_price' => $savedSearchData['param_buying_max_price'] ? (int)$savedSearchData['param_buying_max_price'] : null,
            'renting_min_price' => $savedSearchData['param_renting_min_price'] ? (int)$savedSearchData['param_renting_min_price'] : null,
            'renting_max_price' => $savedSearchData['param_renting_max_price'] ? (int)$savedSearchData['param_renting_max_price'] : null,
            'min_bedrooms' => $savedSearchData['param_min_bedrooms'] ? (int)$savedSearchData['param_min_bedrooms'] : null,
            'property_type' => $savedSearchData['param_property_type'],
            'filter_options' => $savedSearchData['param_filter_options'],
            'radius' => 0,
        ];

        $searchEngine = new PropertySearch($this->db, $paramArgs, $propertyTypes);

        $returnedResults = $searchEngine->entityQuery();

        return (count($returnedResults) > 0) ? $returnedResults : null;
    }

    /**
     * Save search to database
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $email_address
     */
    public function saveSearch(string $email_address): void
    {
        $searchIsValid = false;
        $uniqueHash = null;
        $searchUserData = self::dbGetSearchUserByEmail($email_address);

        if ($searchUserData == null) {
            self::dbInsertSearchUser($email_address);
        }
        $searchUserData = self::dbGetSearchUserByEmail($email_address);

        if (self::validateSearchParams()) {
            $searchIsValid = true;
        }

        if ($searchIsValid) {
            $uniqueHash = $this->generateMD5Hash($email_address);

            if (!is_string($uniqueHash) || self::dbIsHashUsed($uniqueHash)) {
                $searchIsValid = false;
            }
        }

        if ($searchIsValid && !is_null($searchUserData)) {
            $this->dbInsertSearch($email_address, $uniqueHash, $searchUserData);
        }
    }

    /**
     * Loop through each saved search and process it
     * -----------------------------------------------------------------------------------------------------------------
     * @return int
     */
    private function processAllSavedSearches(): void
    {
        $allSearches = null;

        $allSearches = $this->dbGetAllSearchesToProcess(); // Return array(array(email, hash), array(email, hash))

        foreach ($allSearches as $search) {
            $searchResultData = $this->runSearchByHash($search['unique_hash']);

            if (count($searchResultData) > 0) {
                $packet = $this->buildDotmailerPacket($searchResultData, $search);
                if (!is_null($packet)) {
                    $this->dotMailerSearchPackets[] = $packet;
                }
            }
        }

        foreach ($this->dotMailerSearchPackets as $packet) {
            $this->triggerDotmailerEmail($packet);
        }
    }

    /**
     * Build the email HTML and send to dotmailer via Transactional email API call
     * -----------------------------------------------------------------------------------------------------------------
     * @param array $savedSearchPacket
     */
    private function triggerDotmailerEmail(array $savedSearchPacket): void
    {
        $this->drushDebug('Packet sent to dotMailer: ');
        $this->drushDebug(json_encode($savedSearchPacket, JSON_PRETTY_PRINT));
    }

    /**
     * Prepare the data to be sent to the dotMailer system.
     * -----------------------------------------------------------------------------------------------------------------
     * @param array $srchResult
     * @param array $srchParam
     * @return array
     */
    private function buildDotmailerPacket(array $srchResult, array $srchParam): ?array
    {
        $propIdsUsed = explode(",", $srchParam['log_property_ids_used']);
        $propIdsToGet = [];

        $searchUser = $this->dbGetSearchUserByEmail($srchParam['email']);
        $hashUnsubscribeAll = $searchUser['unique_hash'];

        $dotMailerPacket = [
            'replyEmailAddress' => '',
            'recipientEmailAddress' => $srchParam['email'],
            'searchLocation' => $srchParam['search_location'],
            'url_unsubscribe' => $this->base_url . '/property-search/unsubscribe/' . $srchParam['unique_hash'] . '/' . $srchParam['email'],
            'url_unsubscribe_all' => $this->base_url . '/property-search/unsubscribe/' . $hashUnsubscribeAll . '/' . $srchParam['email'],
        ];
        $propertyTypes = new PropertyTypes();

        $propertyMaxCount = 5;
        $propertyCounter = 0;
        foreach ($srchResult as $propertyId) {
            if (!in_array($propertyId, $propIdsUsed) && ($propertyCounter < $propertyMaxCount)) {
                $propIdsToGet[] = $propertyId;
                $propIdsUsed[] = $propertyId;

                $nodeLoaded = Node::load($propertyId);

                if (isset($nodeLoaded)) {
                    $property = new PropertyCard((int)$propertyId, $propertyTypes);

                    $dotMailerPacket['properties'][] = [
                        'nid' => $propertyId,
                        'title' => $nodeLoaded->getTitle(),
                        'thumbnail' => $this->base_url . $property->mainImageThumbnailURL,
                        'url' => $this->base_url . $property->mainPropertyURL,
                        'displayAddress' => $property->displayAddress,
                        'price' => $property->price,
                        'formattedDeposit' => $property->formattedDeposit,
                        'buyorlet' => $property->isSale ? 'buy' : 'let',
                        'features' => $property->features,
                    ];

                    $propertyCounter++;
                }
            }
        }

        $this->dbUpdatePropertiesUsed($srchParam['unique_hash'], implode(",", array_filter($propIdsUsed)));

//        if(is_array($dotMailerPacket['properties'])){
        return (is_array($dotMailerPacket['properties']) && count($dotMailerPacket['properties']) > 0) ? $dotMailerPacket : null;
//        }else{
//
//        }

        //return $dotMailerPacket;
    }

    /**
     * Output a given message for debugging if debug parameter was passed via drush
     * -----------------------------------------------------------------------------------------------------------------
     * @param $msg
     */
    private function drushDebug($msg)
    {
        if ($this->debugDrush) {
            drush_print($msg);
        }
    }

    /**
     * Entry point for the drush command to process all saved searches
     * -----------------------------------------------------------------------------------------------------------------
     * @return bool|int
     */
    public function drushProcessSearches(?string $debug): void
    {
        if ($debug === 'debug') {
            $this->debugDrush = true;
        }

        $this->drushDebug('Attempting to process all saved searches');

        $drushProcessStatus = null;

        try {
            $this->processAllSavedSearches();
        } catch (\Exception $ex) {
            \Drupal::logger('lrg_search_saves')
                ->error('processAllSavedSearches failed when calling processAllSavedSearches: Exception: ' . $ex->getMessage());
        }
    }

    /**
     * Create MD5 hash
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $email_address
     * @param string|null $additionalSalt
     * @param string $forTable
     * @return string
     */
    private function generateMD5Hash(
        string $email_address,
        string $additionalSalt = null,
        string $forTable = 'SAVED_SEARCHES'
    ): string {
        $salt = microtime();
        $salt2 = (!is_null($additionalSalt)) ? $additionalSalt : '';
        $stringToHash = $salt . $salt2 . $email_address;

        return md5($stringToHash);
    }

    /**
     * Return true if the search parameters already exist for this email address
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $email_address
     * @param array $searchUserData
     * @return bool
     */
    private function searchExists(string $email_address): bool
    {
        $sqlStatement = "SELECT `lrg_saved_searches`.`unique_hash`, `lrg_savedSearchUsers`.`id` 
            FROM `lrg_saved_searches`
            INNER JOIN `lrg_savedSearchUsers`
            ON `lrg_savedSearchUsers`.`id` = `lrg_saved_searches`.`refid_unsuball`
            WHERE
            `lrg_saved_searches`.`param_search_type` = :p_st AND
            `lrg_saved_searches`.`param_search_location_key` = :p_slk AND
            `lrg_saved_searches`.`param_search_location` = :p_sl AND
            `lrg_saved_searches`.`param_buying_min_price` = :p_bminp AND
            `lrg_saved_searches`.`param_buying_max_price` = :p_bmaxp AND
            `lrg_saved_searches`.`param_renting_min_price` = :p_rminp AND
            `lrg_saved_searches`.`param_renting_max_price` = :p_rmaxp AND
            `lrg_saved_searches`.`param_min_bedrooms` = :p_mb AND
            `lrg_saved_searches`.`param_property_type` = :p_pt AND
            `lrg_saved_searches`.`param_filter_options` = :p_fo AND
            `lrg_savedSearchUsers`.`email_address` = :email;";

        $args = [
            ':p_st' => $this->searchParam['search_type'],
            ':p_slk' => $this->searchParam['search_type'],
            ':p_sl' => $this->searchParam['search_type'],
            ':p_bminp' => $this->searchParam['search_type'],
            ':p_bmaxp' => $this->searchParam['search_type'],
            ':p_rminp' => $this->searchParam['search_type'],
            ':p_rmaxp' => $this->searchParam['search_type'],
            ':p_mb' => $this->searchParam['search_type'],
            ':p_pt' => $this->searchParam['search_type'],
            ':p_fo' => $this->searchParam['search_type'],
            ':email' => $this->searchParam['search_type'],
        ];

        $query = $this->db->query($sqlStatement, $args);

        $status = $query->fetchAssoc() ? true : false;

        return $status;
    }

    /**
     * Is hash in use
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $hash
     * @param string $tableToCheck
     * @return bool
     */
    private function dbIsHashUsed(string $hash, string $tableToCheck = 'SAVED_SEARCHES'): bool
    {
        $result = null;

        $tbl = ($tableToCheck === 'SAVED_SEARCHES') ? 'lrg_saved_searches' : 'lrg_savedSearchUsers';

        $queryStatement = "SELECT COUNT(`id`) as `HashNumCount` 
          FROM $tbl WHERE `unique_hash` LIKE :unique_hash;";

        $args = [
            ':unique_hash' => $hash,
        ];

        try {
            $query = $this->db->query($queryStatement, $args);
            $result = $query->fetchAssoc();
        } catch (\Exception $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbIsHashUsed() Exception: ' . $ex->getMessage());
        }

        if ($result) {
            $numTimesUsed = $result['HashNumCount'];

            if ($numTimesUsed > 0) {
                return true;
            } else {
                return false;
            }
        } else {
            return false;
        }
    }

    /**
     * Find user by email from `lrg_savedSearchUsers` table
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $email_address
     * @return array|null
     */
    private function dbGetSearchUserByEmail(string $email_address): ?array
    {
        $result = null;

        $queryStatement = "SELECT * FROM `lrg_savedSearchUsers` 
          WHERE `email_address` = :email";

        try {
            $query = $this->db->query($queryStatement, array(':email' => $email_address));
            $result = $query->fetchAssoc();
        } catch (\Exception $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbGetSearchUserByEmail() Exception: ' . $ex->getMessage());
        }

        if ($result) {
            return $result;
        }

        return null;
    }

    /**
     * Search for a specific saved search
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $hash
     * @return array
     */
    private function dbGetSearchDataForHash(string $hash): ?array
    {
        $result = null;

        $sqlGetSearchData = "SELECT `lrg_saved_searches`.* FROM `lrg_saved_searches`
            INNER JOIN `lrg_savedSearchUsers` ON `lrg_savedSearchUsers`.`id` = `lrg_saved_searches`.`refid_unsuball`
            WHERE `lrg_saved_searches`.`unique_hash` = :unique_hash";

        try {
            $query = $this->db->query($sqlGetSearchData, array(':unique_hash' => $hash));
            $result = $query->fetchAssoc();
        } catch (\Exception $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbGetSearchDataForHash() Exception: ' . $ex->getMessage());
        }

        if ($result) {
            return $result;
        } else {
            return null;
        }
    }

    /**
     * Return array of emails and hashes of searches to conduct
     * -----------------------------------------------------------------------------------------------------------------
     * @return array|null
     */
    private function dbGetAllSearchesToProcess(): ?array
    {
        $searchCollection = null;

        $sqlGetAllSearches = "SELECT `lrg_saved_searches`.*, 
            `lrg_savedSearchUsers`.`email_address` as `email`
            FROM `lrg_saved_searches`
            INNER JOIN `lrg_savedSearchUsers` ON 
            `lrg_savedSearchUsers`.`id` = `lrg_saved_searches`.`refid_unsuball`";

        try {
            $query = $this->db->query($sqlGetAllSearches);

            while ($result = $query->fetchAssoc()) {
                $searchCollection[] = $result;
            }
        } catch (\Exception $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbGetAllSearchesToProcess() Exception: ' . $ex->getMessage());
        }

        if (count($searchCollection) > 0) {
            return $searchCollection;
        } else {
            return null;
        }
    }

    /**
     * Delete a single search or delete all searches and the user owning them
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $hash
     * @param string $email_address
     * @param bool $deleteAllSearches
     * @return int
     */
    private function dbDeleteSearch(string $hash, string $email_address, $deleteAllSearches = false): int
    {
        if ($deleteAllSearches) {
            $queryStatement = "DELETE `lrg_savedSearchUsers`, `lrg_saved_searches` FROM `lrg_saved_searches` 
                RIGHT JOIN `lrg_savedSearchUsers` 
                ON `lrg_saved_searches`.`refid_unsuball` = `lrg_savedSearchUsers`.`id` 
                WHERE `lrg_savedSearchUsers`.`email_address` = :email AND
                    `lrg_savedSearchUsers`.`unique_hash` = :unique_hash";

            $args = [
                ':email' => $email_address,
                ':unique_hash' => $hash,
            ];

            try {
                $this->db->query($queryStatement, $args);
                return 1;
            } catch (\Exception $ex) {
                return 3;
            }
        } else {
            $queryStatement = "DELETE FROM `lrg_saved_searches` WHERE `unique_hash` = :unique_hash";

            try {
                $this->db->query($queryStatement, array(':unique_hash' => $hash));
                return 2;
            } catch (\Exception $ex) {
                return 4;
            }
        }
    }

    /**
     * Update a searches' list of properties that were previously sent.
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $hash
     * @param string $propIds
     */
    private function dbUpdatePropertiesUsed(string $hash, string $propIds): void
    {
        $sqlUpdate = "UPDATE `lrg_saved_searches` 
          SET `log_property_ids_used` = :propIds, 
          `date_updated` = NOW() 
          WHERE `unique_hash` = :unique_hash";

        $args = [
            ':propIds' => $propIds,
            ':unique_hash' => $hash,
        ];

        try {
            $this->db->query($sqlUpdate, $args);
        } catch (\Exception $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbUpdatePropertiesUsed() Exception: ' . $ex->getMessage());
        }
    }

    /**
     * Insert Search into database
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $emailAddress
     * @param string $uniqueHash
     * @param array $searchUserData
     */
    private function dbInsertSearch(
        string $emailAddress,
        string $uniqueHash,
        array $searchUserData
    ): void {
        if ($this->searchExists($emailAddress)) {
            return;
        }

        $sqlInsertStatement = "INSERT INTO `lrg_saved_searches` 
          (
            `id`, `unique_hash`, `refid_unsuball`, `date_created`, 
            `date_updated`, `param_search_type`, `param_search_location_key`, `param_search_location`, 
            `param_buying_min_price`, `param_buying_max_price`, `param_renting_min_price`, `param_renting_max_price`, 
            `param_min_bedrooms`, `param_property_type`, `param_filter_options`, `log_property_ids_used`
          )VALUES(
            NULL, :uhash, :refid, NOW(), 
            NOW(), :p_st, :p_slk, :p_sl, 
            :p_bminp, :p_bmaxp, :p_rminp, :p_rmaxp, 
            :p_mb, :p_pt, :p_fo, ''
          );";

        $sqlArgs = [
            ":uhash" => $uniqueHash,
            ":refid" => $searchUserData['id'],
            ":p_st" => $this->searchParam['search_type'],
            ":p_slk" => $this->searchParam['search_location_key'],
            ":p_sl" => $this->searchParam['search_location'],
            ":p_bminp" => $this->searchParam['buying_min_price'],
            ":p_bmaxp" => $this->searchParam['buying_max_price'],
            ":p_rminp" => $this->searchParam['renting_min_price'],
            ":p_rmaxp" => $this->searchParam['renting_max_price'],
            ":p_mb" => $this->searchParam['min_bedrooms'],
            ":p_pt" => $this->searchParam['property_type'],
            ":p_fo" => $this->searchParam['filter_options'],
        ];

        try {
            $this->db->query($sqlInsertStatement, $sqlArgs);

            \Drupal::logger('SearchesSaved Class')->notice('Search successfully saved, parameters were: ' . json_encode($this->searchParam));
        } catch (InvalidArgumentException $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbInsertSearch() InvalidArgumentException: ' . $ex->getMessage());
        } catch (\Exception $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbInsertSearch() Exception: ' . $ex->getMessage());
        }

    }

    /**
     * Save a new entry in the `lrg_savedSearchUsers` table. Used for tracking user emails and also for
     * un-subscribing ALL searches
     * -----------------------------------------------------------------------------------------------------------------
     * @param string $email_address
     */
    private function dbInsertSearchUser(string $email_address): void
    {
        $uniqueHash = $this->generateMD5Hash($email_address, 'NOTSAVEDSEARCHES');

        $sqlStatement = "INSERT INTO `lrg_savedSearchUsers` (`id`, `email_address`, `unique_hash`) 
          VALUES (NULL, :email, ':unique_hash');";

        try {
            $this->db->query($sqlStatement, array(':email' => $email_address, ':unique_hash' => $uniqueHash));
        } catch (\Exception $ex) {
            \Drupal::logger('SearchesSaved Class')->error('dbInsertSearchUser() Exception: ' . $ex->getMessage());
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

You'll have to take some of these at face value as I don't use Drupal.

I looked at this purely from a logical and structural view point, So there are probably a few things going on here i may not realize. So make sure to review my suggestions closely.

For example I don't know what a lot of the inputs are I may have suggested changing a few to constants such as this $tableToCheck = 'SAVED_SEARCHES' to $tbl = self::TBL_SAVED_USERS in dbIsHashUsed what I used was the tbl name.

Some of them I had to extensively rework, so pay attention to them. Most of it is simplification, removing some local variables etc.

Oh and I found a bug (I think) in generateMD5Hash the table name is not used $forTable the third argument.

etc..

private function __construct()
{
    //...
    //use static instead of self if you plan on extending this (Late static binding) - something to consider for all static calls
    $this->searchParam = self::initSearchParamsArray();
   //...
}

public static function create(): SearchesSaved
{
     return new SearchesSaved; //the () are not required here 
    //you can leave them for readability
    //but I find a lot of programmers don't know this
}

private function setInitSearchParam($paramName, $paramValue)
{
    //...
    if (isset($this->searchParam[$paramName]))
        //isset is much faster then array_key_exists (Language construct vs function call)
    //...
}

//use dependency injection if possible
//this is not called internally (that i could find) so it may be a good choice for it
public function setSearchParams(\WhateverClass $Query): void
{

     //...
         $args[$arg] = $validator->sanitise($Query->get($arg));
    //...
}

private function validateSearchParams(): bool
{
    //simplify
    return (
            is_null($this->searchParam['search_location']) || 
            !is_string($this->searchParam['search_location']) || 
            strlen($this->searchParam['search_location']) <= 0) 
        );
}

//combine and simplify
public function unsubscribe(string $hash, string $email_address): ?int
{

    try {
        $searchUser = $this->dbGetSearchUserByEmail($email_address);

        $status = $this->dbDeleteSearch(
               $hash,
               $email_address,
               ($searchUser['unique_hash'] === $hash)
        );
    } catch (\Exception $ex) {
        $status = 5;
    }

    return $status;
}


//Make sure to test this one I had to change a lot 
public function saveSearch(string $email_address): void
{
    if (self::validateSearchParams()) {
        if (is_null(self::dbGetSearchUserByEmail($email_address)) {
            //insert if no users
            self::dbInsertSearchUser($email_address); //1
        }

        //generate this
        $uniqueHash = $this->generateMD5Hash($email_address);

        //if uniqueHash is a string AND it's not used AND a user exists
        if (
            is_string($uniqueHash) &&
            !self::dbIsHashUsed($uniqueHash) &&
            !is_null($searchUserData = self::dbGetSearchUserByEmail($email_address)) //assignment
        ) {
             $this->dbInsertSearch($email_address, $uniqueHash, $searchUserData);
        }
    }
}

private function processAllSavedSearches(): void
{
    //allSearches  can be removed

    foreach ($this->dbGetAllSearchesToProcess() as $search) {
        $searchResultData = $this->runSearchByHash($search['unique_hash']);

        //you can just use if it's not empty insted of count which is faster
        if (!empty($searchResultData)) { 
            $packet = $this->buildDotmailerPacket($searchResultData, $search);
            if (!is_null($packet)) {
                $this->dotMailerSearchPackets[] = $packet;
            }
        }
    }

    foreach ($this->dotMailerSearchPackets as $packet) {
        $this->triggerDotmailerEmail($packet);
    }
}

//Not sure about this - it looks like it should be broken down some but not sure how
//private function buildDotmailerPacket(array $srchResult, array $srchParam)

//add this constant
const DEBUG_MODE = 'debug';

//this looks like a good place for a constant
//consider - $SearchesSaved->drushProcessSearches(SearchesSaved::DEBUG_MODE)
//less error prone, eg. did I do "debug" or "debuging" - you wonder 18 months from now?
public function drushProcessSearches(?string $debug): void
{
        if ($debug === self::DEBUG_MODE) {
            $this->debugDrush = true;
        }
         $this->drushDebug('Attempting to process all saved searches');

        //remove this ->   $drushProcessStatus = null; Unused

        //..
}

private function generateMD5Hash(
        string $email_address,
        string $additionalSalt = null,
        string $forTable = 'SAVED_SEARCHES' //<--- unused {BUG} ? - this should be a constant as well
): string {
    //unchanged
    $salt = microtime();
    $salt2 = (!is_null($additionalSalt)) ? $additionalSalt : '';
    $stringToHash = $salt . $salt2 . $email_address;

    return md5($stringToHash);
}

//add these constants
const TBL_SAVED_SEARCHES = 'lrg_saved_searches';
const TBL_SAVED_USERS = 'lrg_savedSearchUsers';

private function dbIsHashUsed(string $hash, string $tbl = self::TBL_SAVED_USERS): bool
{ 
    if($tableToCheck !== self::TBL_SAVED_SEARCHES){
        $tbl = self::TBL_SAVED_USERS;
    }

    $args = [
        ':unique_hash' => $hash,
    ];

    try {
        $query = $this->db->query(
            "SELECT COUNT(`id`) as `HashNumCount` FROM $tbl WHERE `unique_hash` LIKE :unique_hash;"
            , [':unique_hash' => $hash]
        );
        $result = $query->fetchAssoc(); //could do fetchColumn or however you Drupal guys do it

        return ( $result && $result['HashNumCount'] > 0 );

    } catch (\Exception $ex) {
        \Drupal::logger('SearchesSaved Class')->error('dbIsHashUsed() Exception: ' . $ex->getMessage());
    }
}

//there are a few like this one
private function dbGetSearchUserByEmail(string $email_address): ?array
{
    try {
        $query = $this->db->query("SELECT * FROM `lrg_savedSearchUsers` WHERE `email_address` = :email", [':email' => $email_address]); //consstancy
        return $query->fetchAssoc();
    } catch (\Exception $ex) {
        \Drupal::logger('SearchesSaved Class')->error('dbGetSearchUserByEmail() Exception: ' . $ex->getMessage());
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Cheers @ArtisticPhoenix appreciated. Wasn't concerned with the Drupal stuff, more the structure and general PHP stuff. Your rework has all been helpful and have applied some of the changes you showed to my other classes also. I appreciate your time and effort. \$\endgroup\$ – John Cogan Mar 19 '19 at 9:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.