3
\$\begingroup\$

I'm new here. I recently found myself in charge of a large PHP-based system. The guy that wrote it produced some of the worst code I've ever seen. I've never been in charge and have had the luxury of working under some very good programmers who took the time to "groom" me - but with stricter languages like python, insisting if I went back to PHP I would regret it. I do. As I'm now the "head" of a department (just one under me for now) that hopes to grow and rewriting everything, it's important to me to set good standards from the beginning and stick to them. Please, if you have time, take a look at this and feel free to absolutely shred it - keep standards in mind, that's what I'm trying to learn here.

Purpose: connect to gmail, get new emails that change format periodically (worth noting) from particular automated account, parse the text for values (for db in future script), and email a summary to the sales team (I've stopped short of mailing the output to come her). Runs as a cron job.

Constraints: everything outside of this script is a poopoo show. I am weak on interacting with a database without some sort of layer doing the gruntwork for me - welcoming advice. I'm considering PHPDoc for the future. Thoughts?

I have FOOBARified any potentially sensitive text. This script runs.

Edit: I realized that my regular expressions are flawed in the case that someone enters something like "Email:" in a field. If I'm not mistaken, that would happen in order... so they'd actually have to put "Email:" in the Name field and those are taken from a lead source so it's not a concern to me at this time. In the case that they put something similar in the "Comments" field in some natural case, I believe everything should be fine. Does this sound correct or am I making an unsafe assumption?

/**
 * @author Me
 * Temporary script to mimic the behavior of
 * the getMyMail and FOOBAR scripts in order
 * to fix the FOOBAR parsing issues
 * /
 * 
 * Steps:
 *     Fetch the mail from the server
 *     Parse out data
 *     insert into FOOBAR.FOOBARFOOBAR
 *     send response to lead's email
 *     send notice to salesperson
 *     update row on FOOBAR.FOOBARFOOBAR to say replied after reply succeeds
 */
ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);
require_once(__DIR__ . '/../config/config.inc.php');
require_once(__DIR__ . '/../FOOBAR/PHPMailer/PHPMailerAutoload.php');

class FOOBAREmailCronjob
{
  private function getEmailsFromServer()
  {
    $strConnectionFailed = "Connection to mail server failed: ";
    $strEmailHost = "{imap.gmail.com:993/imap/ssl}INBOX";
    $strEmailUsername = '[email protected]';
    $strEmailPassword = 'FOOBAR';
    $strFetchEmailsFrom = '[email protected]';
    $arrEmails = array();
    $arrParsedEmails = array();
    $objInboxConnection = imap_open($strEmailHost, $strEmailUsername,
      $strEmailPassword) or die($strConnectionFailed . imap_last_error());
    $arrEmails = imap_search($objInboxConnection, 'UNSEEN FROM "' .$strFetchEmailsFrom .'"');
    if($arrEmails) {
      foreach($arrEmails as $intEmailID)
      {
        $clsEmail = new \stdClass();
        $clsEmail->user = new \stdClass();
        $clsEmail->overview = imap_fetch_overview($objInboxConnection, $intEmailID, 0);
        $clsEmail->message = imap_fetchbody($objInboxConnection, $intEmailID, 1);
        $clsEmail->message = quoted_printable_decode($clsEmail->message);
        $clsEmail->date = date('Y-m-d H:i:s', strtotime($clsEmail->overview[0]->date));
        $clsEmail->header = imap_headerinfo($objInboxConnection, $intEmailID);
        $clsEmail->subject= $clsEmail->overview[0]->subject;
        $clsEmail->senderEmail = $clsEmail->header->fromaddress;
        $clsEmail->emailID = imap_uid($objInboxConnection, $intEmailID);
        array_push($arrParsedEmails, $clsEmail);
      }
    }
    imap_close($objInboxConnection);
    return $arrParsedEmails;
  }
  // other than cleaning and reorganizing, the functionality
  // in this bit is taken from old code that currently works.
  private function legacyOperations($clsEmail) 
  {
    $clsLegacyData = new \stdClass();
    $clsEmail->clsLegacyData = $clsLegacyData;
    $intMaxDisplayPrice = 99999998;
    $intMinDisplayPrice = 1;
    $strNoDisplayPrice = "Call For Price";
    $arrFOOBARData = $this->getFOOBARData($clsEmail->clsParsedFields->id);
    $clsEmail->clsLegacyData->owner = 
      $this->getOwner($arrFOOBARData['category_id']);
    $clsEmail->clsLegacyData->manufacturer = 
      $this->getManufacturer($arrFOOBARData['manufacturer_id']);
    $clsEmail->clsLegacyData->headline = $arrFOOBARData['headline'];
    /**
     * use "headline" to create path after root domain
     * which is later converted by .htaccess to show listings
     * ->this is essentially an inexplicably bad router
     */
    // first turn spaces into hyphyens
    $strPath = str_replace(' ', '-', $arrFOOBARData['headline']);
    // then turn any remaining spaces at the beginning into dashes
    $strPath = preg_replace('/[^ \w]+/', '-', $strPath);
    // now use 'Sold' as a boolean and convert it
    // cuz that's smart af
    if ($arrFOOBARData['status'] === 'Sold')
    {
      $strPath .= '-Archived-';
    } else {
      $strPath .= '-FOOBAR-For-Sale-';
    }
    // throw the FOOBAR ID on there too, why not
    $strPath .=  $arrFOOBARData['id'];
    $clsEmail->clsLegacyData->url = $strPath;
    if ($arrFOOBARData['price'] >= $intMaxDisplayPrice or
      $arrFOOBARData['price'] <= $intMinDisplayPrice)
    {
      $clsEmail->clsLegacyData->price = $strNoDisplayPrice;
    } else {
      // NOTE: money_format does not exist on windows
      $clsEmail->clsLegacyData->price = money_format('%.2n', $arrFOOBARData['price']);
    }
    return $clsEmail;
  }
  private function getFOOBARData($intID)
  {
    // TODO: stupid
    $dbFOOBAR = $this->dbConnection("FOOBAR"); // TODO: this is stupid
    $clsQueryData = $dbFOOBAR->query("SELECT * FROM listings WHERE id = $intID LIMIT 1");
    if ($clsQueryData->num_rows > 0)
    {
      $arrResult = $clsQueryData->fetch_assoc();
    } else {
      $arrResult = array();
    }
    $dbFOOBAR->close();
    return $arrResult;
  }
  private function getManufacturer($intID)
  {
    $dbFOOBAR = $this->dbConnection("FOOBAR"); // TODO: this is stupid
    $clsQueryData = $dbFOOBAR->query("SELECT * FROM manufacturers WHERE id = $intID LIMIT 1");
    if ($clsQueryData->num_rows > 0)
    {
      $arrResult = $clsQueryData->fetch_assoc();
    } else {
      $arrResult = array();
    }
    $dbFOOBAR->close();
    return $arrResult;
  }
  private function getOwner($intCatID)
  {
    $dbFOOBAR2 = $this->dbConnection("FOOBAR2"); // TODO: this is stupid
    $clsQueryData = $dbFOOBAR2->query("SELECT * FROM FOOBAR_categories WHERE id = $intCatID LIMIT 1");
    if ($clsQueryData->num_rows > 0)
    {
      $arrResult = $clsQueryData->fetch_assoc();
    } else {
      $arrResult = array();
    }
    $dbFOOBAR2->close();
    return $arrResult;
  }
  private function getEmailVariables($clsEmail)
  {
    $arrEmailVariables = array(
      "{{lead.name}}" => $clsEmail->clsParsedFields->name,
      "{{lead.email}}" => $clsEmail->clsParsedFields->email,
      "{{lead.phone}}" => $clsEmail->clsParsedFields->phone,
      "{{lead.company}}" => $clsEmail->clsParsedFields->company,
      "{{lead.location}}" => $clsEmail->clsParsedFields->location,
      "{{lead.comments}}" => $clsEmail->clsParsedFields->comments,
      "{{FOOBAR.name}}" => $clsEmail->clsLegacyData->headline,
      "{{FOOBAR.price}}" => $clsEmail->clsLegacyData->price,
      "{{FOOBAR.url}}" => $clsEmail->clsLegacyData->url
    );
    return $arrEmailVariables;
  }
  private function parseEmailBody($clsEmail)
  {
    // store the strings (escaped) that surround the values we want to
    // capture to be nice to future us.
    // (yes the code looks worse but changing the expressions will
    // be much easier when FOOBAR changes the emails to stop
    // us from doing this.)
    $clsParsedFields = new \stdClass();
    $clsEmail->clsParsedFields = $clsParsedFields;
    $regManufacturerID = ['\(#', '\)'];
    $regInternalID = ['(?:Stock / ID: \D*?-)', '\n'];
    $regName = ['Name: ', '\n'];
    $regEmail = ['Email: ', '\n'];
    $regPhone = ['Phone: ', '\n'];
    $regCompany = ['Company: ', '\n'];
    $regLocation = ['Location: ', '\n'];
    $regZipcode = ['Zip Code: ', '\n'];
    $regPrice = ['Price: ', '\n'];
    $regComments = ['Comments: ', '\n'];
    // create array of regular expressions and the associated fields
    $arrFields = array(
      'manufacturerID'=> 
      '@(?:' . $regManufacturerID[0] .')(.*)(?:' . $regManufacturerID[1] .')@',
      'id' => '@(?:' . $regInternalID[0] .')(.*)(?:' .$regInternalID[1] .')@',
      'name' => '@(?:' . $regName[0] .')(.*)(?:' .$regName[1] .')@',
      'email' => '@(?:' . $regEmail[0] .')(.*)(?:' .$regEmail[1] .')@',
      'phone' => '@(?:' . $regPhone[0] .')(.*)(?:'. $regPhone[1] .')@',
      'company' => '@(?:' . $regCompany[0] .')(.*)(?:'. $regCompany[1] .')@',
      'location' => '@(?:' . $regLocation[0].')(.*)(?:'. $regLocation[1] .')@',
      'zipcode' => '@(?:' . $regZipcode[0] .')(.*)(?:'. $regZipcode[1] .')@',
      'price' => '@(?:' . $regPrice[0] .')(.*)(?:'. $regPrice[1] .')@',
      'comments' => '@(?:' . $regComments[0].')(.*)(?:'. $regComments[1] .')@'
    );
    foreach($arrFields as $keyField => $valExpression) {
      preg_match($valExpression, $clsEmail->message, $arrMatches);
      $clsParsedFields->$keyField = ($arrMatches ? $arrMatches[1] : NULL);
    }
    return $clsEmail;
  }
  private function getTemplate($strTemplateName = "email", $strTemplatePath = "templates/")
  {
    $strTemplateHeader = file_get_contents(__DIR__ . '/' . 
      $strTemplatePath . 'inc/header.html');
    $strTemplateFooter = file_get_contents(__DIR__ . '/' . 
      $strTemplatePath . 'inc/footer.html');
    // "/" is only here because of my code highlighter thinking a
    // string like /blah/ is a regex and it bugs me
    // any ideas?
    $strTemplateBody = file_get_contents(__DIR__ . '/' .
      $strTemplatePath . $strTemplateName . '.html');
    
    return $strTemplateHeader . $strTemplateBody . $strTemplateFooter;
  }
  private function populateTemplate($strTemplate, $arrVariables) 
  {
    $strOutput = strtr($strTemplate, $arrVariables);
    return $strOutput;
  }
  private function dbConnection($strDBName = "FOOBAR")
  {
    $strDBServer = $GLOBALS['db_server'];
    $strDBUsername = $GLOBALS['db_username'];
    $strDBPassword = $GLOBALS['db_password'];
    return new mysqli($strDBServer, $strDBUsername, $strDBPassword, $strDBName);
  }
  function run()
  {
    $arrEmails = $this->getEmailsFromServer();
    foreach($arrEmails as $clsEmail) 
    {
      $clsEmail = $this->parseEmailBody($clsEmail);
      $clsEmail = $this->legacyOperations($clsEmail);
      $strEmailTemplate = $this->getTemplate("sales");
      $arrEmailVariables = $this->getEmailVariables($clsEmail);
      $strEmailOutput = $this->populateTemplate($strEmailTemplate, $arrEmailVariables);
      //TODO: pickup here
      echo($strEmailOutput);
    }
  }
}
if (!function_exists('imap_open')) {
  echo "IMAP is not configured.";
}
$FOOBAREmailCronjob = new FOOBAREmailCronjob();
$FOOBAREmailCronjob->run();
```
\$\endgroup\$
8
  • 2
    \$\begingroup\$ Is this an example of the "worst code", or is this after you've made an effort to improve it to the best of your ability? \$\endgroup\$ Commented Oct 8, 2021 at 7:09
  • \$\begingroup\$ This is my rewrite. I really hope you're not asking because this is that bad :( \$\endgroup\$ Commented Oct 8, 2021 at 13:01
  • \$\begingroup\$ No, just confirming that we're not wasting time with something you already know how to improve. PHP isn't my language at all, so I have no opinion on the code here! \$\endgroup\$ Commented Oct 8, 2021 at 13:04
  • \$\begingroup\$ I feel that language itself is irrelevant in this case. Any feedback is welcome on organization, variable naming, redundancy, etc. Does this look fairly self documenting? \$\endgroup\$ Commented Oct 8, 2021 at 13:09
  • 1
    \$\begingroup\$ What is the version of php you're using \$\endgroup\$
    – Tropus
    Commented Oct 8, 2021 at 15:07

3 Answers 3

2
\$\begingroup\$

I had a short look at your code. It is a pity you haven't included an example e-mail in your question. It makes reviewing your e-mail parsing code more difficult than it needs to be.

Object Oriented Programming

The very first thing I noticed is that you use a single class as a container for everything. Using OOP is a good thing, but you're not leveraging its power. Actually if you wrote this code just with functions, without the class around it, it would be virtually the same.

Your code has several functionalities that would also be useful in other code. There's a way to connect to the database, interact with an IMAP server, parse an e-mail, work with a template. It would make sense to write separate classes for all of these. Need to interact with an IMAP server again? Then you don't have to copy that method, but you can reuse the same class, literally the same file, you've already created to do this. It is common practice to use one PHP file per class.

A good basis for programming are the SOLID principles. These principles are worded quite abstractly, which makes them harder to understand, but they are very useful. What I mentioned above would fall under the single-responsibility principle. It states that every module, class or function in a computer program should have responsibility over a single part of that program's functionality, and it should encapsulate that part.

Prepared Statements

Another thing, that stands out in your code, is a more practical one. You're not using prepared statements. They are used to prevent SQL injection. In PHP it really is a standard to always use prepared statements, and not doing so is really frowned upon. There is no excuse not to do it.

Don't repeat yourself

Although there isn't much of it, there are sections of repeated code. parseEmailBody(), for instance, could be written a lot cleaner, and easier to understand. It feels messy. See: DRY. This might be partly due to the legacy software you're dealing with.

Names should clearly, and only, convey meaning

What I mean by that is that the names, you choose in your code, should clearly indicated what they represent. You break this convention by using str, obj, cls and int at the beginning of your variable names. That is not meaning, that is type. At first I didn't get that, and I was wrecking my brain over what the "cls" in clsParsedFields could mean. Another inconsistency occurs when you start to use reg when the type is an array. I think reg stands for "regular Expression"? That's another thing: Don't let people guess what an abbreviated name could mean. If you mean a regular expression, use, as a minimum; "regexp". It's a little thing, but it just adds to the frustration when you read someones code and you have no idea what a name stands for. I'm sure you know what I mean.

Here are two very useful links:

https://phptherightway.com

https://www.php-fig.org

\$\endgroup\$
2
  • \$\begingroup\$ I feel like I might have found my old mentor here. For the OOP stuff, I agree whole-heartedly. I'm very grateful for the variable naming feedback. I was trying to be a good citizen and was thinking "why am I doing this if it's a friggin local variable?" and so on. I'm glad you told me you couldn't tell what the prefixes were for - I want to code for the guy who fills my chair when I get hit by a bus. Looking at it now with the points you made, I feel a bit procedural - exactly what I want to replace. All I did was fix a script, I didn't build anything. Gonna go read up on solid. Thanks! \$\endgroup\$ Commented Oct 8, 2021 at 20:52
  • \$\begingroup\$ Also - thanks for prepared statements comment. I'm glad I came here. I should go lend a hand where I can. \$\endgroup\$ Commented Oct 8, 2021 at 20:54
2
\$\begingroup\$

Okay, not withstanding the php version. I'd really need that to know what can, and can't be done. Since there's been quite a few changes these past few years.

First I'd suggest breaking up your code, you're creating a db connection, you're making a connection with an Imap, you're mapping the imap data into a stdClass. Then you're parsing the Imap returned data; doing regex searches and more. In short, the class is doing too much.

I'd suggest creating either local properties (one for the database connection, and one for the imap connection) or creating seperate connection classes that handle the imap and db connection. And let those classes handle all the connection stuff.

I'd also recommend creating data classes for the Imap emails. By adding getters and setters you can hammer down what you're allowed to do and what you have.

I'd also suggest adding some constants for error handling, that way you can define at the top of your class what you expect to go wrong, and add a simple error handler to handle these cases.

As for variable names, since 7.1 it's possible to do a lot more type casting. if you use that properly, you will have no need for prefixing variable names with type shortcuts such as str, int, cls If you feel you need to give a variable a name that includes it's type. You should probably see if you can't extract the logic breaking up your code more

I also feel your code is very focused from getting data from point a to point b, mapping it along the way. This usually ends up creating code that has logic all over the place. It's generally better to change it to what you'd like to work with (external data -> internal structure -> process -> map for database -> database) Using that process you use code that is decoupled from external code/api/etc and your database structure untill it is relevant.

As it is currently coded your connection to the Imap seems pretty hard coded, you could look into making it variable, so you can reuse some of your code (if you split up the Imap mapping, you could reuse it whenever you do things with Imap emails)

\$\endgroup\$
1
  • \$\begingroup\$ That was fantastic! There's no excuse but my justification for a lot of the structure here was it's the first short term problem I had to fix for the bossman before rebuilding the whole thing. You made some excellent points I've taken note of. If you don't mind... I read up on what I'd missed in php land and saw that casting was a thing now. I originally had things like $content = (string) getContent(); for example. I took it out and planned on researching forcing types. As in "integer expected" rather than "oh hey this is a string but I want a bool, ok, I guess it's true." Any advice there? \$\endgroup\$ Commented Oct 8, 2021 at 20:48
2
\$\begingroup\$

General comments

Overall this code looks okay, though I'd suggest following recommended style guides like PSR-1 and PSR-12 (similar to PEP-8). Indentation is consistent. I'm not fond of new lines for brackets - I understand it is recommended for methods and classes but not for control structures like conditionals and loops.

I honestly didn't comb through everything but did notice these lines within legacyOperations():

// first turn spaces into hyphyens
$strPath = str_replace(' ', '-', $arrFOOBARData['headline']);
// then turn any remaining spaces at the beginning into dashes
$strPath = preg_replace('/[^ \w]+/', '-', $strPath);

The regular expression /[^ \w]+/ is a negated character class that will match any character that is not a space or a word character anywhere in the string. Was the use of ^ supposed to the starting anchor? i.e. /^[ \w]+/?

Suggestions

SQL injection

Bearing in mind this has already been mentioned - this script might not be as susceptible as others (e.g. this) but it is still wise to bind parameters and use prepared statements.

For example:

private function getFOOBARData($intID)
{
  // TODO: stupid
  $dbFOOBAR = $this->dbConnection("FOOBAR"); // TODO: this is stupid
  $clsQueryData = $dbFOOBAR->query("SELECT * FROM listings WHERE id = $intID LIMIT 1");

It appears that this method is called in legacyOperations()

$arrFOOBARData = $this->getFOOBARData($clsEmail->clsParsedFields->id);

and it appears that the id property is parsed using a regular expression in parseEmailBody. It would be wise to

  • ensure the method parameter is an integer - might need to be cast from string after it is parsed
  • bind the parameter to the query instead of concatenating the value

Variable Naming

I've worked on a codebase that started with systems hungarian notation for more than a decade. I've grown used to it but also not fond of it. IDEs have better support and modern versions of PHP do as well for declaring types of function arguments, return values and class properties1. Inline with PSR-12 I'd prefer to see camelCase variable names though snake_case could be used if you (and any teammates) prefer that style.

Config variables

There are two variables for credentials in the method getEmailsFromServer()

$strEmailPassword = 'FOOBAR';
$strFetchEmailsFrom = '[email protected]'

Those seem like something that should be included from a config file - perhaps config.inc.php or else us a dot env file.

money_format function

// NOTE: money_format does not exist on windows
$clsEmail->clsLegacyData->price = money_format('%.2n', $arrFOOBARData['price']);

I'd suggest checking to ensure the function exists before calling it - e.g. using function_exists(). I haven't used money_format() but I see a warning on the PHP documentation:

Warning This function has been DEPRECATED as of PHP 7.4.0, and REMOVED as of PHP 8.0.0. Relying on this function is highly discouraged. 2

Array syntax

There isn't anything wrong with using array() but as of the time of writing, PHP has Active support for versions 8.0 and 7.4, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4) - e.g. $arrEmails = [].

\$\endgroup\$
2
  • \$\begingroup\$ This was remarkably helpful. Very specific points. Much of the OOP stuff I "know" but tossed aside in favor of "just replace the old script for now," and these points help me a lot. Thanks! \$\endgroup\$ Commented Oct 9, 2021 at 0:57
  • \$\begingroup\$ 🆒 if you believe it is useful, consider giving it an upvote. For more information, see What should I do when someone answers my question?. \$\endgroup\$ Commented Oct 9, 2021 at 1:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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