Phishing Project Sending Email Reworked

@Pimgd gave some good feedback on the previous post. I've made the changes that I think best suit my application and am looking to see what people think of this implementation.

PhishingController

/**
 * sendEmail
 * Function mapped to Laravel route. Defines variable arrays and calls Email Class executeEmail.
 * 
 * @param   Request         $request            Request object passed via AJAX from client.
 */
public function sendEmail(Request $request) {
    $fromEmail = $request['fromEmail'];
    $fromPass = $request['fromPass'];
    $host = $request['hostName'];
    $port = $request['port'];
    $emailSettings = array($host,$port,$fromEmail,$fromPass);

    $emailTemplate = 'emails.' . $request['emailTemplate'];
    $emailTemplateType = substr($request['emailTemplate'],0,3);
    $emailTemplateTarget = substr($request['emailTemplate'],3,1);
    $template = array($emailTemplate,$emailTemplateType,$emailTemplateTarget);

    $period = 4;
    $subject = $request['subject'];
    $projectName = $request['projectName'];
    $projectId = intval($projectName,strpos($projectName,'_'));
    $projectName = substr($projectName,0,strpos($projectName,'_')-1);
    $companyName = $request['companyName'];
    $params = array($period,$projectName,$projectId,$companyName,$subject);

    try {
        Email::executeEmail($emailSettings,$template,$params);
    } catch(OutOfBoundsException $oobe) {
        //mail server settings not valid
    } catch(FailureException $fee) {
        //email failed to be sent to server
    } catch(\PDOException $pdoe) {
        DBManager::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
    } catch(QueryException $qe) {
        DBManager::logQueryError(__CLASS__,__FUNCTION__,$qe);
    }
}

Email

```
/**
* executeEmail
* Public-facing method to send an email to a database of users if they are a valid recipient.
*
* @param array $emailSettings Host, port, username, and password variable

1)

Looked at previous review and couldn't agree more about the complexity of the method signatures as well as the number of possible Exception types in play being very confusing an immediate red flags that your implementation is questionable.

You seem to be mainly struggling with composition in coding, which can take some time and practice to become more second nature. I do think you made some progress in trying to minimize number of parameters, but you still have a lot of complexity there.

Have you considered instead of passing arrays to the main method, passing objects? This would be taking more of a dependency injection approach as has been discussed previously in some of your other code reviews. This would also make your code MUCH more readable as passing numerically-indexed arrays makes code much harder to understand.

Pass this class the things it needs - DB object, template object, template data object, email configuration object, user object, etc. This allows you to enforce all the validations for these types of things and making sure you have these in the proper state BEFORE trying to work with them to actually send an email.

Don't ask your email class to understand how to interpret an AJAX request, instantiate and validate all its dependencies, how to validate email addresses, how to generate short URL's, how to set environmental variables (why would you be setting these in an email sending context anyway?), etc.

The email class should expect valid dependencies to be passed to it, so that your class code can focus on the actual email sending functionality.

2)

It seems you are struggling with when to use numerically-indexed arrays vs. associative arrays or objects as appropriate data structure.

Numerically-indexed arrays should typically only be used when you are working with an arbitrary length list of similar items or item order is important. Associative arrays or objects should typically be used in cases where items you are trying to store may be distinctly different and should therefore have a lookup key or property name that allows you to get directly to the information you want. Typically one does not care about order for associative array/objects and iterating over such objects is not as commonly done as with numerically-indexed arrays.

I personally tend to use objects (even stdClass objects) much more frequently than associative arrays when working with a related set of dissimilar data. I tend to use associative arrays more in PHP for hash table (hash map) use cases, since this is, in essence, PHP's implementation of that data structure. IMO, a lot of PHP examples out there in the wild overuse associative arrays for cases where objects are more appropriate (dealing with database result sets being one of the more frequent offenders in code examples).

I will use your email configuration array as an example of how this is a problem in your code, as this clearly should not be a numerically-indexed array as it is now. This array currently holds 4 distinct types of information - host name, port, username, password. Your code would read MUCH better if you you actually did something like:

// Associative array example

if(!preg_match($pattern,$emailSettings['host']) ||
   !filter_var($emailSettings['port'], FILTER_VALIDATE_INT) ||
   !filter_var($emailSettings['username'], FILTER_VALIDATE_EMAIL)) {

// object example (my personal preference here)
if(!preg_match($pattern,$emailSettings->host) ||
   !filter_var($emailSettings->port, FILTER_VALIDATE_INT) ||
   !filter_var($emailSettings->username, FILTER_VALIDATE_EMAIL)) {

You see here in the code that you now know EXACTLY what sorts of values I am dealing with. The code is more meaningful to the reader. (Note I also broke your code up across multiple lines as your code gets hard to read if you go beyond standard rule of thumb of ~80 characters per line.)

Of course maybe this example is moot here if you went with dependency injection approach, as you would then instantiate a emailCongfiguration object or similar and have all the data validation happening in that class, not in an email sending class.

3)

This code is redundant

if(!preg_match($pattern,$emailSettings[0]) || !filter_var($emailSettings[1],FILTER_VALIDATE_INT) ||
        !filter_var($emailSettings[2],FILTER_VALIDATE_EMAIL)) {
        $message = '';
        if(!preg_match($pattern,$emailSettings[0])) {
            $message .= 'Host is not a valid host name or IP address. host=' . $emailSettings[0] . '\n';
        }
        if(!filter_var($emailSettings[1],FILTER_VALIDATE_INT)) {
            $message .= 'Port is not a valid integer. port=' . $emailSettings[1] . '\n';
        }
        if(!filter_var($emailSettings[2],FILTER_VALIDATE_EMAIL)) {
            $message .= 'Username is not a valid email address. username=' . $emailSettings[2] . '\n';
        }
        throw new OutOfBoundsException($message);
    }

Why check each condition twice? You may be be

StackExchange Code Review Q#134770, answer score: 3