After receiving some good feedback on the previous question, I've refactored my database and have begun to refactor the associated code. As a result of implementing models for all tables in the database, it has become smoother to use Eloquence Builder instead of a custom class using PDO.Below find some of the changes made. For this review, I'm looking to see how effective my implementation of the Eloquence Builder is and how streamlined this email process seems. That being said, when I was testing the validateMailingList function, the $projects was coming back as an array(array(array()))... Not sure why, but I was able to get it working, but not sure about the best way to parse and clean up this data.PhishingController/** * retrieveProjects * Helper function to grab the 3 most recent projects for a user, then grab the project object of each project. *  * @param   int             $id         Mailing_list ID of the requested user. * @return  array */private static function retrieveProjects($id) {    $result = Sent_Mail::where('SML_UserId',$id)        ->limit(3)        ->get();    $result = json_decode($result,true);    $projects = array();    for($i = 0; $i < sizeof($result); $i++) {        $project = Project::where('PRJ_Id',$result[$i]['SML_ProjectId'])->get();        $projects[] = json_decode($project,true);    }    return $projects;}/** * validateMailingList * Validates all the mailing_list recipients. Returns only those that will receive the email. * * @param   array           $currentProject         The current project being validated against. * @param   int             $periodInWeeks          Number of weeks back to check for most recent email. * @return  array */private static function validateMailingList($currentProject, $periodInWeeks) {    $users = Mailing_List_User::all();    $users = json_decode($users,true);    $mailingList = array();    for($i = 0; $i < sizeof($users); $i++) {        $date = date('Y-m-d h:i:s',strtotime(-$periodInWeeks weeks));        $projects = self::retrieveProjects($users[$i]['MGL_Id']);        if($projects[0][0]['updated_at'] <= $date) {            $mailingList[] = $users[$i];        }        $complexity = $currentProject[0]['PRJ_ComplexityType'];        $target = $currentProject[0]['PRJ_TargetType'];        if((!is_null($projects[0]) &&                $complexity == $projects[0][0]['PRJ_ComplexityType'] &&                $target == $projects[0][0]['PRJ_TargetType'])            ||                (!is_null($projects[0][0]) && !is_null($projects[1][0]) &&                    $complexity == $projects[0][0]['PRJ_ComplexityType'] &&                    $complexity == $projects[1][0]['PRJ_ComplexityType'])            ||                (!is_null($projects[0][0]) && !is_null($projects[1][0]) && !is_null($projects[2][0]) &&                    $target == $projects[0][0]['PRJ_TargetType'] &&                    $target == $projects[1][0]['PRJ_TargetType'] &&                    $target == $projects[2][0]['PRJ_TargetType'])) {            //invalid recipient - something may happen here        }        else {            $mailingList[] = $users[$i];        }    }    return $mailingList;}/** * 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) {    try {        $templateConfig = new TemplateConfiguration(            array(                'templateName'=>$request->input('emailTemplate'),                'companyName'=>$request->input('companyText'),                'projectName'=>$request->input('projectData')['projectName'],                'projectId'=>intval($request->input('projectData')['projectId'])            )        );        $currentProject = json_decode(Project::where('PRJ_Id',$templateConfig->getProjectId())->get(),true);        $periodInWeeks = 4;        $emailConfig = new EmailConfiguration(            array(                'host'=>$request->input('mailServerText'),                'port'=>$request->input('mailPortText'),                'authUsername'=>$request->input('fromEmailText'),                'authPassword'=>'password',                'fromEmail'=>$request->input('fromEmailText'),                'subject'=>$request->input('subject'),                'users'=>self::validateMailingList($currentProject,$periodInWeeks)            )        );        Email::executeEmail($emailConfig,$templateConfig);    } catch(ConfigurationException $ce) {        //will be doing something here - what still has yet to be defined (likely just log the exception)    } catch(EmailException $ee) {        //will be doing something here - what still has yet to be defined (likely just log the exception)    }}Email/** * executeEmail * Public-facing method to send an email to a database of users if they are a valid recipient. * * @param   EmailConfiguration          $emailConfig            Email Configuration object containing required information to send an email * @param   TemplateConfiguration       $templateConfig         Template Configuration object containing required information to build a template * @throws  EmailException                                      Custom Exception to embody any exceptions thrown in this class */public static function executeEmail(    EmailConfiguration $emailConfig,    TemplateConfiguration $templateConfig){    self::setTemplateConfig($templateConfig);    self::setEmailConfig($emailConfig);    try {        foreach($emailConfig->getUsers() as $user) {            self::sendEmail($user);            self::logSentEmail($user);        }    } catch(Exception $e) {        throw new EmailException(__CLASS__ . ' Exception',0,$e);    }}/** * logSentEmail * Updates the user with the newest project and rotates the old projects down one. * * @param   array           $recipient           Mailing_List_User array */private static function logSentEmail($recipient) {    $sent_mail = Sent_Mail::create(        ['SML_UserId'=>$recipient['MGL_Id'],        'SML_ProjectId'=>self::$templateConfig->getProjectId(),        'SML_Timestamp'=>Carbon::now()]    );}/** * sendEmail * Sends them an email to the specified user. * * @param   User_test           $user           User object * @throws  FailureException */private static function sendEmail($user) {    $templateData = array(        'companyName'=>self::$templateConfig->getCompanyName(),        'projectName'=>self::$templateConfig->getProjectName(),        'projectId'=>self::$templateConfig->getProjectId(),        'lastName'=>$user['MGL_LastName'],        'username'=>$user['MGL_Username'],        'urlId'=>$user['MGL_UniqueURLId']    );    $subject = self::$emailConfig->getSubject();    $from = self::$emailConfig->getFromEmail();    $to = $user['MGL_Email'];    $mailResult = Mail::send(        ['html' => 'emails.phishing.' . self::$templateConfig->getTemplate()],        $templateData,        function($m) use ($from, $to, $subject) {            $m->from($from);            $m->to($to);            $m->subject($subject);        }    );    if(!$mailResult) {        throw new FailureException('Email failed to send to ' . $to . ', from ' . $from);    }}Currently, I just pass the json_decode result of the users to the sendEmail function in the Email class. I'm curious if it would be smarter to define a User class, not a model, that would take the associative array from the User model and create a User Object. It would look something like:public function __construct($userArray) {    //validate array    //assign private vars}I could then create getters to retrieve using object notation instead of array notation - $user->getLastName instead of $user['MGL_LastName'].Beyond this minor piece that I'm unsure about, it's pretty straight forward. I've got a few minor bugs on how the data is being passed to the server, but that doesn't affect the server implementation. How well have I implemented Eloquence Builder?Thanks for the review!
Phishing Project - Refactor Using Models
php;laravel;eloquent
I think you probably made a good decision in looking at a more full-featured ORM, as from earlier code review on your former DBManager class, I was of the opinion that the class was not really adding much value.  Now you have shifted to a model-based means to define the objects your application is working.  I think this is a good step forward.I agree with your thoughts that you should have a user object and perhaps a user list object, user list factory or something similar to control the creationof a user list.My comments have been added within your code below in multi-line quotes.PhishingController/*You should use a join here in this method and retrieve all emails thatmeet your project criteria in a single query.Any time you see yourself executing a query within a loop, this shouldbe a red flag to you that there is probably a better way to do this.*/private static function retrieveProjects($id) {/*Validation for $id?*/    $result = Sent_Mail::where('SML_UserId',$id)        ->limit(3)        ->get();/*$result from above should be an array, why are you using json_decode here?*/    $result = json_decode($result,true);    $projects = array();    for($i = 0; $i < sizeof($result); $i++) {        $project = Project::where('PRJ_Id',$result[$i]['SML_ProjectId'])->get();/*json_decode?*/        $projects[] = json_decode($project,true);    }    return $projects;}private static function validateMailingList($currentProject, $periodInWeeks) {    $users = Mailing_List_User::all();/*Why is JSON decode needed?*/    $users = json_decode($users,true);    $mailingList = array();/*Can you not apply filtering to your DB query and only return list ofapplicable users from DB rather than filter in application code?*/    for($i = 0; $i < sizeof($users); $i++) {        $date = date('Y-m-d h:i:s',strtotime(-$periodInWeeks weeks));/*You are retrieving projects in a loop and that method itself queries in loopsThis code is exponentially complex :(I know working with ORM's can make complex queries painful, but my guessis that you really need to look at these two related methods and figureout how to get only the meaningful user and related project recordsfrom the database in a single query. Don't query all users, iterate of overall of them, then for each of them iterate through all there projectsYou are doing a lot more work here than is needed.*/        $projects = self::retrieveProjects($users[$i]['MGL_Id']);/*All the code in this section is really hard to read because of allthe $project[0][0][*] stuff. Get away from querying in loops andyou will be able to clean this whole section up, likely also includingthese crazy conditionals.*/        if($projects[0][0]['updated_at'] <= $date) {            $mailingList[] = $users[$i];        }        $complexity = $currentProject[0]['PRJ_ComplexityType'];        $target = $currentProject[0]['PRJ_TargetType'];        if((!is_null($projects[0]) &&                $complexity == $projects[0][0]['PRJ_ComplexityType'] &&                $target == $projects[0][0]['PRJ_TargetType'])            ||                (!is_null($projects[0][0]) && !is_null($projects[1][0]) &&                    $complexity == $projects[0][0]['PRJ_ComplexityType'] &&                    $complexity == $projects[1][0]['PRJ_ComplexityType'])            ||                (!is_null($projects[0][0]) && !is_null($projects[1][0]) && !is_null($projects[2][0]) &&                    $target == $projects[0][0]['PRJ_TargetType'] &&                    $target == $projects[1][0]['PRJ_TargetType'] &&                    $target == $projects[2][0]['PRJ_TargetType'])) {            //invalid recipient - something may happen here        }        else {            $mailingList[] = $users[$i];        }    }    return $mailingList;}/*It doesn't look like this method has changed much.  I think my comments from last review are still valid here.  Mainly that is set up all yourdependencies here, including user list, then pass to Email::executeEmail()*/public function sendEmail(Request $request) {    try {        $templateConfig = new TemplateConfiguration(            array(                'templateName'=>$request->input('emailTemplate'),                'companyName'=>$request->input('companyText'),                'projectName'=>$request->input('projectData')['projectName'],                'projectId'=>intval($request->input('projectData')['projectId'])            )        );        $currentProject = json_decode(Project::where('PRJ_Id',$templateConfig->getProjectId())->get(),true);        $periodInWeeks = 4;        $emailConfig = new EmailConfiguration(            array(                'host'=>$request->input('mailServerText'),                'port'=>$request->input('mailPortText'),                'authUsername'=>$request->input('fromEmailText'),/*Can you get this password out of here and just have it as constant orsomething. The ideal scenario woudl be if you could populate fromenvironmental variable and not have passwords in your codebase at all.*/                'authPassword'=>'password',                'fromEmail'=>$request->input('fromEmailText'),                'subject'=>$request->input('subject'),                'users'=>self::validateMailingList($currentProject,$periodInWeeks)            )        );        Email::executeEmail($emailConfig,$templateConfig);    } catch(ConfigurationException $ce) {        //will be doing something here - what still has yet to be defined (likely just log the exception)    } catch(EmailException $ee) {        //will be doing something here - what still has yet to be defined (likely just log the exception)    }}Emailpublic static function executeEmail(    EmailConfiguration $emailConfig,    TemplateConfiguration $templateConfig){    self::setTemplateConfig($templateConfig);    self::setEmailConfig($emailConfig);    try {/*Same comment as before.  Not really sure why user list is created fromemail config object.*/        foreach($emailConfig->getUsers() as $user) {            self::sendEmail($user);            self::logSentEmail($user);        }    } catch(Exception $e) {        throw new EmailException(__CLASS__ . ' Exception',0,$e);    }}private static function logSentEmail($recipient) {/*I really like that you have moved this logging functionality into itsown method, though it is not clear how this method is called.*/    $sent_mail = Sent_Mail::create(        ['SML_UserId'=>$recipient['MGL_Id'],        'SML_ProjectId'=>self::$templateConfig->getProjectId(),        'SML_Timestamp'=>Carbon::now()]    );}private static function sendEmail($user) {    $templateData = array(        'companyName'=>self::$templateConfig->getCompanyName(),        'projectName'=>self::$templateConfig->getProjectName(),        'projectId'=>self::$templateConfig->getProjectId(),/*I agree with your thoughts above that user should be an object.Also consider when making it an object aliasing your DB field namesuch that the object properties are like 'lastName', 'username', etc.instead of 'MGL_LastName' and such. */        'lastName'=>$user['MGL_LastName'],        'username'=>$user['MGL_Username'],        'urlId'=>$user['MGL_UniqueURLId']    );    $subject = self::$emailConfig->getSubject();    $from = self::$emailConfig->getFromEmail();    $to = $user['MGL_Email'];    $mailResult = Mail::send(        ['html' => 'emails.phishing.' . self::$templateConfig->getTemplate()],        $templateData,        function($m) use ($from, $to, $subject) {            $m->from($from);            $m->to($to);            $m->subject($subject);        }    );    if(!$mailResult) {        throw new FailureException('Email failed to send to ' . $to . ', from ' . $from);    }}