7
\$\begingroup\$

I'm working on my graduate project and I had stumbled upon a somewhat dilemma, I've managed to solve it with some workarounds but I have my doubts that this is the most efficient way to deal with this problem.

I'm writing a class to deal with the Plesk API and making it as flexible as possible for easy use.

Here is the function for generating the XML that will be send out as request:

private function emailCreatePacket($domain, $params){
    // Create new XML document.
    $xml = new DomDocument('1.0', 'UTF-8');
    $xml->formatOutput = true;

    // Create packet
    $packet = $xml->createElement('packet');
    $packet->setAttribute('version', '1.4.2.0');
    $xml->appendChild($packet);
    $mail = $xml->createElement('mail');
    $packet->appendChild($mail);
    $create = $xml->createElement('create');
    $mail->appendChild($create);
    $filter = $xml->createElement('filter');
    $create->appendChild($filter);
    $domain_id = $xml->createElement('domain_id', $domain->id);
    $filter->appendChild($domain_id);
    $mailname = $xml->createElement('mailname');
    $filter->appendChild($mailname);
    foreach ($params as $key => $value) { 
        $node = $mailname;
        if(strpos($key, ':') !== false){
            $split = explode(':', $key);
            $key = $split[1];
            $node = ${$split[0]};
            if(!isset(${$split[0]})){
                ${$split[0]} = $xml->createElement($split[0]);
                $mailname->appendChild(${$split[0]});
            }
            $xmlelement = $xml->createElement($key, $value);
            ${$split[0]}->appendChild($xmlelement);
        }else{
            $xmlelement = $xml->createElement($key, $value); 
            $node->appendChild($xmlelement);
        }
    }
    return $xml->saveXML();
}

Here's my public function:

public function createEmail($host, $domain, $params){
    $curl = $this->curlInit($host);
    $packet = $this->emailCreatePacket($domain, $params);
    echo $packet;
    $response = $this->sendRequest($curl, $packet);
    echo $response;
    if($this->parseResponse($response)){
        return true;
    }
}

Here are the functions called:

private function curlInit($host)
    {
        $security = new Security();
        $password = $security->decode($host['host_pass']);
        $curl = curl_init();
        curl_setopt($curl, CURLOPT_URL, "https://{$host['host_address']}:{$host['host_port']}/{$host['host_path']}");
        curl_setopt($curl, CURLOPT_RETURNTRANSFER, true);
        curl_setopt($curl, CURLOPT_POST,           true);
        curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false);
        curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, false);    
        curl_setopt($curl, CURLOPT_HTTPHEADER,
             array("HTTP_AUTH_LOGIN: {$host['host_user']}",
                    "HTTP_AUTH_PASSWD: {$password}",
                    "HTTP_PRETTY_PRINT: TRUE",
                    "Content-Type: text/xml")
        );    
        return $curl;
    }

private function sendRequest($curl, $packet){
        curl_setopt($curl, CURLOPT_POSTFIELDS, $packet);
        $result = curl_exec($curl);
        if (curl_errno($curl)) {
            $error  = curl_error($curl);
            $errorcode = curl_errno($curl);
            curl_close($curl);
            throw new Exception("Er is iets mis gegaan: <br /><br /> Foutcode: " . $errorcode . "<br /> Foutmelding: " . $error );
        }
        curl_close($curl);
        return $result;
    }

private function parseResponse($response){
    $xml = new SimpleXMLElement($response);
    $status = $xml->xpath('//status');
    if($status[0] == "error"){
        $errorcode = $xml->xpath('//errcode');
        $errortext = $xml->xpath('//errtext');
        throw new Exception("Er is iets mis gegaan: <br /><br /> Foutcode: ". $errorcode[0] . "<br /> Foutmelding: " . $errortext[0]);
    }else{
        return $response;
    }
}

Here is the code I use to call my function:

<?php

/**
 * @author Jeffro
 * @copyright 2011
 */

require('init.php');

$security = new Security();

if($security->sslactive()){
    try
    {
        $hosts = new Hosts();
        $host = $hosts->getHost("192.168.1.60");
        $pleskapi = new PleskApi();
        $client = $pleskapi->getClientInfo($host, 'janbham');
        $domain = $pleskapi->getDomainInfo($host, "lol.nl");
    $params = array('name' => 'ohecht', 'mailbox:enabled' => 'true', 'mailbox:quota' => '100000000', 'alias' => 'aapjesrollen', 'password' => 'testmail', 'permissions:manage_drweb' => 'true');
    $pleskapi->createEmail($host, $domain, $params);
    }
    catch (Exception $ex)
    {
        echo $ex->GetMessage();
    }
}else{
    ?>
    SSL needs to be active
    <?php
}

?>

The problem was regarding the structure the API uses with it deeper nesting every time (which I solved now using ':' as a delimiter)

\$\endgroup\$
  • \$\begingroup\$ Just out of curiosity, in your execution script (that last bit), why did you close and open your script to output a one line string? Why not just echo 'SSL needs to be active';? BTW, this isn't a critique - I'm just curious. \$\endgroup\$ – 65Fbef05 Mar 23 '11 at 14:30
  • \$\begingroup\$ I've been flamed/bashed a lot on the subject of echo'ing stuff. Echo'ing HTML is generally bad. Not that it matters that much in this example because its just a test.php to test my functions. \$\endgroup\$ – Jeffro Mar 24 '11 at 7:38
  • \$\begingroup\$ Really? echo just returns to stdout - I can't imagine why someone would get onto you about your output method (especially in a case like this). Now... If you're output is multiple lines, it is good practice to use an output buffer - but then you would echo the resulting var. I would like to hear someone's case against echo. Clearly it's there for a reason, and that reason is output. \$\endgroup\$ – 65Fbef05 Mar 24 '11 at 9:30
  • \$\begingroup\$ why not nested array instead of :? Conceptually the : is used for attributes, and not for children. Because attributes values have not hierarchy. So I think it's better (not rule, just better) build <mailbox quota="10000"> if you use :, <mailbox><quota>1000</quote></mailbox> if you use nested array. \$\endgroup\$ – Luca Rainone Feb 8 '15 at 12:01
2
\$\begingroup\$

Generally speaking, I like your code. I think it's well planned with few inefficiencies. I'm also impressed by your use of PHP's DOMDocument class. When creating XML via PHP, I usually opt for foreach loops and output buffering. I've been aware of this object class, but your example is the first I've ever seen it put to use. +1 for showing me something new! :D

\$\endgroup\$
2
\$\begingroup\$
  • In createEmail you only return true in one case; I'd just return the value of parseResponse directly, then again, you don't seem to use the return value.
  • Being nitpicky, the style is a bit inconsistent, that includes spacing, quotes, but otherwise looks good. The $params creation should be on multiple lines though, IMO 200 characters kind of stretches it.
  • You could often leave out the last else block when you've already thrown an exception in the true case.
  • What's with the random echo calls?

Now the XML creation is, let's say, a bit verbose. emailCreatePacket should use at least a helper function, or, if you have a recent enough version of PHP I understand that you could also use a local anonymous function instead. So for starters, how about something like this:

private function addChild($xml, $parent, name) {
    $child = $xml->createElement(name);
    $parent->appendChild(child);
    return $child;
}

private function emailCreatePacket($domain, $params){
    $xml = new DomDocument('1.0', 'UTF-8');
    $xml->formatOutput = true;

    $packet = addChild($xml, $xml, 'packet');
    $packet->setAttribute('version', '1.4.2.0');

    $mail = addChild($xml, $packet, 'mail');

    $create = addChild($xml, $mail, 'create');
    ...
}

You get the idea. Also check the PHP documentation, I think you'd be faster with using SimpleXML here as well, unless I'm missing a reason why you're using the DOM instead. Thirdly, I would think about using a separate PHP file to generate XML instead, after all, you're already basically using a templating engine.

The splitting of the parameters is probably fine, except I don't understand why you'd need that instead of passing nested arrays? Also the current code there is limited to two levels, right, so for extra points make that usable with arbitrarily nested parameters.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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