1
\$\begingroup\$

I intend to make some changes in an HTML document, like remove, replace, append some nodes.

I have several arrays with same structure like the following example

$patterns[] = array(
    'xpath'   => '/html/body/div/p',
    'insert'  => $new_a,
    'task'    => 'replace'
);

...

$patterns[] = array(
    'xpath'   => '/html/body/header',
    'insert'  => $new_b,
    'task'    => 'remove'
);

and I use the following snippet

$dom = new DOMDocument();

if ((empty($html) !== true) && ($dom->loadHTML($html) === true))
{
    $dom->formatOutput = true;

    // Use DomXPath
    $xpath = new DomXPath($dom);

    // Process Dom changes
    foreach ($patterns as $key => $value)
    {
        $xpath_results = $xpath->query($patterns[$key]['xpath']);

        if ($xpath_results->length) 
        {
            // Assign some values
            $xpath_results = $xpath_results->item(0);
            $task          = $patterns[$key]['task'];
            $replacement   = $patterns[$key]['insert'];

            if ($task != 'remove')
            {
                // Create the replacement
                $newNode = $dom->createDocumentFragment();
                $newNode->appendXML($replacement);
            }

            switch ($task)
            {
                case 'remove':
                    $xpath_results->parentNode->removeChild($xpath_results);
                    break;
                case 'replace':
                    $xpath_results->parentNode->replaceChild($newNode, $xpath_results);
                    break;
                case 'append':
                    $xpath_results->appendChild($newNode);
                    break;
                case 'prepend':
                    $xpath_results->parentNode->insertBefore($newNode, $xpath_results);
                    break;
            }
        }
    }

    // Save Dom 
    $html = $dom->saveXML($dom->documentElement);
}

I would like your expert comments about if this is done the right way. The foreach to loop through the array elements, the switch to choose what will be done and if the use of createDocumentFragment is appropriate in this case.

\$\endgroup\$
1
\$\begingroup\$

Your code seems to globally do the right job, using a right way, and I can't see how you could really improve performance.
I only noticed a few bad points:

  1. Unless I missed some subtleness, there is a typo in the case 'append': branch: it should be using $xpath_results->**parentNode**,like others.
  2. You didn't take in account a possible error condition at $newNode->appendXML($replacement);: it may return FALSE, so you should cancel the process in that case.
  3. The same applies to a possible wrong value in the task index of a pattern: it should be taken in account through the default: option of the switch().

In the other hand I would suggest some improvements at the coding level itself, essentially for readability.

  1. You're using foreach ($patterns as $key => $value), but then always write $patterns[$key][...] so you force PHP to a few more work, while $value is never used.
    I suggest to rather use foreach ($patterns as $value), then $value[...].
    (or even more readable foreach ($patterns as $pattern), then $pattern[...])
  2. Still for readability I would add a separate variable to clearly distinguish the successive "avatars" of $xpath_results, which is first a DOMNodeList then a DOMNode.
    So you could change your DOMNode extraction to $node = $xpath_results->item(0);, and all following code becomes much more obvious.
  3. Your // Assing some valuespart could be reduced to the $node assignation above: the two other variables are only used once so you could merely write $newNode->appendXML($pattern['insert']); and switch ($pattern['task']).

So here is my suggestion for the whole snippet after the remarks above:

if ($html) {
    $dom = new DOMDocument();
    if (!$dom->loadHTML($html)) {
        $error = 'Error while loading source HTML!';
    } else {
        $dom->formatOutput = true;
        // Use DomXPath
        $xpath = new DomXPath($dom);
        // Process Dom changes
        foreach ($patterns as $pattern) {
            $xpath_results = $xpath->query($pattern['xpath']);
            if ($xpath_results->length) {
                $node = $xpath_results->item(0);

                if ($task != 'remove') {
                    // Create the replacement
                    $newNode = $dom->createDocumentFragment();
                    if (!$newNode->appendXML($pattern['insert'])) {
                        $error = "Wrong insert pattern: $pattern['insert']!";
                        break;
                    }
                }
                if (!@$error) {
                    switch ($pattern['task']) {
                        case 'remove':
                            $node->parentNode->removeChild($node);
                            break;
                        case 'replace':
                            $node->parentNode->replaceChild($newNode, $node);
                            break;
                        case 'append':
                            $node->parentNode->appendChild($newNode);
                            break;
                        case 'prepend':
                            $node->parentNode->insertBefore($newNode, $node);
                            break;
                        default:
                            $error = "Wrong task pattern: $pattern['task']!";
                    }
                }
            }
        }
    }
    if (@$error) {
        // print or return $error...
    } else {
        // Save Dom
        $html = $dom->saveXML($dom->documentElement);
    }
}

For the error processing, I arbitrarily choosed one simple option among numerous different possiilities. You can notice:

  1. The @$error form: it is a simple and clear way to allow using variables set only in certain cases without having defined them previously.
  2. Still for readability, I changed the initial tests into more simple (and sufficient) forms: if ($html) and if (!dom->loadHTML($html)).
\$\endgroup\$
  • \$\begingroup\$ @cFeed More clean and readable, indeed. I completely ignored the error handling in my snippet :( Corrected now. Thank you very much for your tips. \$\endgroup\$ – n.h. Jun 21 '15 at 9:00

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.