Phishing Project Sending Email with Configuration Classes

As suggested by @MikeBrant in the previous question, I've added in two configuration classes to simplify and more broadly organize the objects required to send my mail messages. This also removes the need to actively read and write with the .env file. I did not have time to write new PHPDocs for any of the changes today, so methods have their old PHPDoc followed by a generated PHPDoc (if needed). EmailException and ConfigurationException currently don't do anything other than extend Exception. They are merely just a name for now. This may change later after review of the code for specific details the exception need's access to.

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) {
    $projectName = $request['projectName'];
    $projectId = intval($projectName,strpos($projectName,'_'));
    $projectName = substr($projectName,0,strpos($projectName,'_')-1);
    $period = 4;

    try {
        $templateConfig = new TemplateConfiguration(
            array(
                'templateName'=>$request['emailTemplate'],
                'companyName'=>$request['companyName'],
                'projectName'=>$projectName,
                'projectId'=>$projectId
            )
        );

        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request['hostName'],
                'port'=>$request['port'],
                'authUsername'=>$request['username'],
                'authPassword'=>$request['password'],
                'fromEmail'=>$request['fromEmail'],
                'subject'=>$request['subject']
            )
        );

        Email::executeEmail($emailConfig,$templateConfig,$period);
    } catch(ConfigurationException $ce) {

    } catch(EmailException $ee) {

    }
}

EmailConfiguration

```
private

I completely skipped over the part where you mentioned that you weren't done writing the documentation yet! My mistake. That said, perhaps you'll find some parts of this answer of use when you do have to write the documentation.

Looks like you forgot to update your documentation...

public static function executeEmail(EmailConfiguration $emailConfig, TemplateConfiguration $templateConfig, $period) {

The method signature mentions $emailConfig, $templateConfig and $period. The comments above the method describe $emailSettings, $template and $params. I think you'll be able to cut most of the comments away; but documenting what $period is for would help.

Looking in greater detail at the comments you have...

/**
 * EmailConfiguration constructor.
 * @param $emailSettings
 */
public function __construct($emailSettings) {

Obvious comment, and thus not needed. What's $emailSettings for? That's something I'd like to know, but that part of the commentary is missing. If I go in blind, I'll get an ConfigurationException back... telling me that my $emailSettings are invalid. That's ... not helpful. I guess I could read the nested exception, but it'd be great if you were to describe the constructor arguments in greater detail.

/**
 * @param $emailSettings
 * @return bool
 */
private function areSettingsValid($emailSettings) {

Ah, a validator! Stuff goes in, boolean flag "valid yes no" comes out. Looking at the function name, it seems I'll get true back if the object is valid, so I guess I'll get false back if the object is not valid.

Except you don't.

throw new OutOfBoundsException('No settings specified.');
throw new OutOfBoundsException('Expected array, received ' . get_class($emailSettings) . ' Object');
throw new OutOfBoundsException($message);
throw new InvalidArgumentException($message);
...
return true;

So, either you get back true, or exceptions. ... So what's the boolean for? If the method returns normally, it's always going to be true! What's more, you've got a function signature that seems to suggest non-throwing code, and the comments for that function make no mention of exceptions either!

/**
 * @param $domainName
 * @return int
 */
private function isValidDomainName($domainName) {

This one is weird. isX usually returns a boolean, not int. Either explain in a comment or change the type? It's good that you documented the return type, or I'd have thought it was boolean. ... still, what does a result of 0 mean? Is that good? Bad? Is the domain name valid if I get 0 back?

/**
 * TemplateConfiguration constructor.
 * @param $templateSettings
 */
public function __construct($templateSettings) {

Almost the same as the previous constructor. But this one is a ton better. Imagine I use this one blindly. I put crap in... and I get back...

throw new ConfigurationException('Invalid template name.',0,$fnfe);

Well, at least I know I did something wrong with the template name. That's a lot better than "your settings are wrong!".

Detailed specification of the input parameters would still be nice, though.

/**
 * @param $templateSettings
 * @return bool
 */
private function areSettingsValid($templateSettings) {

Same thing here, says it returns bool, but throws exceptions instead. Don't be like that.

Non-documentation comments

private function getUrlId($user,$projectId) {
    $db = new DBManager();
    if(!is_null($user['USR_UniqueURLId'])) {
        $urlId = $user['USR_UniqueURLId'];
    } else {
        $urlId = $this->random_str(15) . $projectId;
        $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=? WHERE USR_UserId=?;";
        $bindings = array($urlId,$user['USR_UserId']);
        $db->query($sql,$bindings);
    }
    return $urlId;
}

There's no need to create a DBManager here if the user has a USR_UniqueURLId, so delay creation until needed:

private function getUrlId($user,$projectId) {
    if(!is_null($user['USR_UniqueURLId'])) {
        $urlId = $user['USR_UniqueURLId'];
    } else {
        $db = new DBManager();
        $urlId = $this->random_str(15) . $projectId;
        $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=? WHERE USR_UserId=?;";
        $bindings = array($urlId,$user['USR_UserId']);
        $db->query($sql,$bindings);
    }
    return $urlId;
}

$period             Number of weeks

From the documentation, it turns out that $period represents a number of weeks (that some action has to be taken). If you add this to the variable name, like $periodInWeeks, then the meaning of assignments and usages becomes more obvious. Specifically this part in sendEmail:

$period = 4;

```
} else {
if(filter_var($emailSettings['host'],FILTER_VALIDATE_IP)) {
if(!filter_var($emailSettings['port'],FILTER_VALIDATE_INT)) {
$message .= 'Port is not a valid integer. Value provided: ' . var_export(['port'],true) . PHP_EOL;
}

StackExchange Code Review Q#134902, answer score: 2