Process list of e-mail addresses in PHP 5.3

Question

Part of an application that I am building in CakePHP 2.1 needs to send out an e-mail based on a template from a database. The end user is able to set the from, reply to, to, cc, and bcc addresses. Because the user could enter just about anything, I need to process those addresses into a usable array that I can pass to CakeEmail. To this end, I wrote the following code and I want to make sure that it is efficient before I put it into production use (and possibly write a tutorial about it).

<?php
$parseAddress = function(&$address) {
    preg_match_all('/"(.*?)"\s*\<(.*?)\>/', $address, $matches); // Check if $address is in the format ""Test Address" <[email protected]>". TODO: Make the quotes optional.

    if (!is_array($matches) || !isset($matches[1]) || empty($matches[1]) || !isset($matches[2]) || empty($matches[2]) || is_null(array_filter($matches))) {
        return $address; // Return the plain e-mail address if name is not provided
    }

    $parsedAddress = array();

    for ($i = 0; $i < count($matches[0]); $i++) {
        $parsedAddress[$matches[2][$i]] = $matches[1][$i]; // Add e-mail address to array as "email => name"
    }

    $addressCSV = str_getcsv($address); // Split e-mail addresses into array - seems to automatically strip anything between "<" and ">" characters
    foreach($addressCSV as $key => $val) {
        if (!filter_var($val, FILTER_VALIDATE_EMAIL)) { // Take advantage of afformentioned stripping by filtering out everything that is not an e-mail address
            unset($addressCSV[$key]); // Remove the array value that is not an e-mail address
        }
    }

    return array_merge($addressCSV, $parsedAddress); // Join the parsed e-mail addresses with the filtered addresses and return the resulting array
};

echo $parseAddress('[email protected]'); // returns (string) "[email protected]"
echo $parseAddress('"Test Address" <[email protected]>, [email protected], "Smith, John"<[email protected]>'); // returns (array) [0 => "[email protected]", "[email protected]" => "Test Address", "[email protected]" => "Smith, John"]

Answer 1

Alright, I have a number of suggestions. Some of which will invalidate others. The reason I'm keeping these "invalid" bits in is because they are just good practice and I'm trying to pass that along. In order to get the most from this you will have to read all the way through.

First off, why are you using lambda functions (also known as anonymous functions or closures)? This seems completely unnecessary. The thing to keep in mind about lambda functions is that they are not compiled at run time. This means that they should be used sparingly because each time they are called they must be compiled separately. Lambda functions should really only be used for callbacks. Though I'll be honest, I don't even use them then. They tend to make the functions they are used in bulky and therefore difficult to read. In the five, almost six, years I've been doing this I've never once used, or needed, lambda functionality. Best to just use a real function and set a return value. Though that may just be an opinion.

REGEX should only be used as a last resort. Yes it is much more powerful than any of the simpler string functions, but because of that power it requires more processing power as well, and is therefore usually slower. So, if you can do the same thing with a simpler function, it is more than likely better. For instance, you could use explode() here to separate the initial string by spaces to get the first name, last name, and email addresses. If multiple addresses are used you can use explode()'s limiter to only return 3 elements, then use explode() again on the email section. This appears to be what you are doing anyways and is more than likely quicker. Though you may not notice a difference unless you are doing it thousands of times. Besides, REGEX is almost impossible to read and if not documented properly just looks like a foreign language to those of us who come after you and try to figure out what it does. REGEX is another one of those things I've hardly ever needed to have, there is almost always another way.

$matches = explode( ' ', $address, 3 );
list( $firstName, $lastName, $addresses ) = $matches;//for illustration purposes only
$addresses = trim( $addresses, '<>' );//remove those brackets before exploding
$addresses = explode( ', ', $addresses );//could use str_getcsv() here if you wanted

Because explode() always creates an array unless the string passed to it was empty, you can now just check that the result isn't FALSE and then check the array's size to determine if the only thing it contains is an address. This looks much better than that long list of confusing if arguments.

if( $matches === FALSE ) {
    return FALSE;
}
if( count( $matches ) == 1 ) {
    return $address;
}

By the way, you should still validate and sanitize the address with filter_var() in the above scenario. Validate with FILTER_VALIDATE_EMAIL and sanitize with FILTER_SANITIZE_EMAIL. I believe you can even do both at once. Well, I know you can, but I'm not sure if it will return FALSE if its not valid, so you may have to play around with it.

$email = filter_var( $address, FILTER_VALIDATE_EMAIL | FILTER_SANITIZE_EMAIL );

For and while loops declare the functions passed to them as parameters on each iteration, so using count() as a parameter here is inefficient. This value isn't going to change so it should be declared before the loop. Foreach is the only loop that doesn't have this issue, though it is still better, in my opinion, to separate functions from foreach loops as well.

$length = count( $matches[ 0 ] );
for( $i = 0; $i < $length; $i++ ) {

Although, I believe you could get away with just using array_combine() here instead of a loop.

$parsedAddress = array_combine( $matches[ 2 ], $matches [ 1 ] );

str_getcsv() may do what you want, but only because it is a hack. CSV stands for "Comma Separated Values" and is used to store and read data separated by commas. I think, though I can't find it documented anywhere, that I remember reading somewhere that it also uses angle brackets "<>" to separate groups and that is why this works, though I'm not sure. I'm honestly not sure why this works, but you should not rely on hacks to get a job done. What if that function changes? Then if you ever upgrade your server you will be stuck trying to figure out how to fix it, or someone else with less knowledge of how this works will. Besides, you've already gotten those email addresses into an array, why do it again? Just use array_keys() on $parsedAddress to get an array of email addresses. Better yet, while you are looping over $matches the first time and setting them, why not just use filter_var() there? Or if you end up using the array_combine() example, you could use array_map() to apply a callback function, in this case filter_var(), to the array_keys(). You've not just doubled the work here, but quadrupled it. You loop over $matches, then you loop over $addressCSV, then when you merge them, PHP loops over both arrays again to find the differences.

Last thing. Why are you referencing $address? I can find no instance where you are changing its value. This is also unnecessary, and, in my opinion, is a big no-no. Variable referencing is a powerful tool, but, like most powerful tools, it does have some drawbacks. The biggest one is that they are difficult to spot and therefore make troubleshooting difficult. Another is that they aren't well known. So while I can spot this, someone else may have never seen it before. After all, it is just an ampersand and not an actual function that they can easily look up.

I hope this all helps. If you need any clarification, just ask.