3
\$\begingroup\$

The following PHP code does exactly what I need it to do but, as a complete PHP novice, I am wondering: what can be improved here?

$theurl='./section-1.html';
function getbody1($filename) {
  $file = file_get_contents($filename);
  $bodypattern = ".*";
  $bodyendpattern = ".*";
  $thecontainer = "id=\"container\"";
  $thecleanup = "";
  $noheader = preg_replace("/".$bodypattern."/", "", $file);
  $noheader = preg_replace("~".$bodyendpattern."~", "", $noheader);
  $noheader = preg_replace("|".$thecontainer."|", "", $noheader);
  $noheader = preg_replace("#".$thecleanup."#", "", $noheader);
  return $noheader;
}
echo getbody1("$theurl");
\$\endgroup\$
  • 1
    \$\begingroup\$ Can you please add a little description of what it is that your code does, and what you are using it for? \$\endgroup\$ – Simon Forsberg Feb 24 '14 at 19:34
  • 1
    \$\begingroup\$ I have 5 pages created by an html generator. I want to strip out everything before and after the body tag so I am left with just the content of the body section. I will then use php include (I assume??) to pull each of these stripped down pages into a single page as articles in an html5 page. \$\endgroup\$ – PBB Feb 24 '14 at 19:45
  • \$\begingroup\$ Thank you, that's the kind of information that will help reviewers. If you put such information into your question you can expect more up-votes. It's all about making it easier for reviewers to review your code :) \$\endgroup\$ – Simon Forsberg Feb 24 '14 at 20:04
2
\$\begingroup\$

First things first, you are not using the right tool for the job. Regular expressions can't reliably parse markup. Full stop. But more on that later, let's first take a look at your patterns, as this may help you in the future:

$bodypattern = ".*";//for example
//later
preg_replace('/'.$bodypattern.'/',...);

Now, whenever you quote a string into a regex pattern, you have to stop and think: what if the string happens to contain special chars?. I mean: half of the characters can mean a variety of things depending on the context. Thankfully, there's a handy function called preg_quote to help you with that. The safe option here would be:

preg_replace('/'.preg_quote($bodypattern, '/').'/', ...);

Now, you seem to be under the impression that a dot matches anything. And it does, except line-feeds. If I were to apply preg_match('/.+/', $thisAnswer, $matches); to this answer, the pattern would only match the text until a \n is encountered.
You need a PCRE modifier: s:

preg_match('/.+/s', $thisAnswer, $match);

That's more like it. However, this is just as an aside, because what you really should be looking into are ways to actually treat markup as markup: a DOM needs to be parsed, if you want to get the most out of it. Here's my actual (and as-originally-posted) answer:

Ouch, The first thing I noticed is that you're processing markup, and have chosen to do so using regular expressions. That's a bad idea from the off.
It's especially bad considering you have a tool at your disposal that is specifically built to parse markup, the DOMDocument object.

Basically, what you're trying to do can be written as simple as this:

$DOM = new DOMDocument;//create DOM
$DOM->loadHTMLFile($theUrl);//parse HTML
$container = $DOM->getElementById('container');//get the id=container element
if ($container)
{//make sure it was found
    $container->removeAttribute('id');//remove the id attribute
}
$body = $contents = $dom->getElementsByTagName('body')->item(0);//get body
//remove attributes
while ($body->hasAttributes())
{//while tag has attributes
    //remove attribute
    $body->removeAttribute($body->attributes->item(0)->nodeName);
}
//Get the body as string, use saveXML
//as saveHTML adds DOCTYPE, html, head and title tag again
$body = substr($DOM->saveXML($body), 6, -7);

The reason why you need to remove the attributes for the body tag, is because DOMDocument::saveXML will return the markup string with the outer <body> and </body> tags. Their length is a given, but only if the opening <body> tag hasn't got any attributes. Thankfully, we can remove them quite easily, and slice them off with substr.

If you want, you could easily use:

$body = str_replace(
    array('<body>', '</body>'),
    '',
    $DOM->saveXML($body)
);

Perhaps this is a bit easier to understand. Anyway: read the DOMDocument docs, there's a lot you can do there. Just click on the methods I used, and click the objects they return, too, so you know what instances you're working with.

Now wrap all this in a function, and we can add some checks to avoid exceptions or errors from showing up:

function getBody($file, DOMDocument $dom = null)
{//optionally pass existing DOMDocument instance
 //to avoid creating a new one each time we call this function
    if (!file_exists($file))
    {//check if the file exists, if not:
        throw new InvalidArgumentException($file. ' does not exist!');
    }
    $dom = $dom instanceof DOMDocument ? $dom : new DOMDocument;
    $dom->loadHTMLFile($file);
    $container = $dom->getElementById('container');
    if ($container)
        $container->removeAttribute('id');
    $body = $dom->getElementsByTagName('body')->item(0);
    while($body->hasAttributes())
    {
        $body->removeAttribute(
            $body->attributes->item(0)->nodeName
        );
    }
    $body = substr($dom->saveXML($body), 6, -7);
    //optionally remove trailing whitespace:
    return trim($body);
}

Call this function either like this:

$body1 = getBody('path/to/file1.html');

or, since you're going to process more than 1 file, and we can avoid creating a new DOMDocument instance for every function call:

$dom = new DOMDocument;
$files = array('file1.html', 'file2.html');
$bodyStrings = array();
foreach ($files as $file)
    $bodyStrings[] = getBody($file, $dom);

That's it.


As a guide-to-the-DOMDocument-docs:

Every class extends from the DOMNode class, except for the DOMNodeList class, which is a Traversable containing only DOMNode instances, accessible through the item method. Anyway, check the DOMNode methods and properties. Learn that class by heart, as almost every object you work with inherits its properties and methods.
The other class to check is the DOMElement class. Instances of this class represent a node (a tag if you will). It has some methods that I've used here (like removeAttribute).
Finally, everything, of course starts with the DOMDocument class. I had already linked to the docs, but an extra link never hurt anyone.

\$\endgroup\$
  • \$\begingroup\$ Err.....wow. Give me a while to digest this! \$\endgroup\$ – PBB Feb 25 '14 at 14:48
  • \$\begingroup\$ @PBB: No problem. Just added a couple of links to the important bits of the DOMDocument docs. That might help you digest my verbose CR, if it sometimes seem harsh, there's a reason for that. If I say something is bad, I try to back my claim up, and offer an alternative and explain how and why I chose that alternative. Hope my answer, and this comment, make sense and are helpful. Just keep in mind that my main goal is to help, not to be a "Debby-downer" :-P \$\endgroup\$ – Elias Van Ootegem Feb 25 '14 at 15:00
  • \$\begingroup\$ Elias, Worry not. You are clearly a good teacher and everything you have stated is very much appreciated. So far I am getting most of it and my initial tests show this really works very well. My next step is to try the code for the multiple files! \$\endgroup\$ – PBB Feb 25 '14 at 15:12
  • \$\begingroup\$ @PBB: If there is something you're not 100% about, or if there are some hickups, let me know. I wrote the code in this answer off the top of my head, without any testing, so it might contain some bugs, still. I'm off for now, so it may take some time before I can respond to any questions, but don't worry: I will follow up \$\endgroup\$ – Elias Van Ootegem Feb 25 '14 at 15:34
  • \$\begingroup\$ Elias, No, I'm all OK here. I have learnt a great deal and have a better webpage as a result of your code. Very much appreciated! \$\endgroup\$ – PBB Feb 25 '14 at 16:12
1
\$\begingroup\$

You can pass an array of patterns / replacements to preg_replace. So you could do:

$noheader = preg_replace(array("/" . $bodypattern . "/", "~" . $bodyendpattern . "~", "|" . $thecontainer . "|", "#" . $thecleanup . "#"), "", $file);

I haven't done much with preg_replace but it seems like you are removing all characters in your first replacement - /.*/?!

Also you don't need to quote your paramater:

echo getbody1("$theurl");

Just pass it like getbody1($theurl). You would only quote it if you wanted to do variable interpolation.. e.g. something like:

echo "There are $numBooks books available on that subject". 
\$\endgroup\$
  • \$\begingroup\$ On the variable interpolation remark: given that echo is a language construct, that just pushes everything to the stdout stream, the best approach there would be echo 'There are ', $numBooks, ' books available on that subject';. Comma separated avoids unnecessary string concatenation or creation (which causes overhead as it allocates memory, only to free it after echoing the new string), and leaves the variables' types unchanged ($i = 1; "$i"; <-- $i` was int, now is string) \$\endgroup\$ – Elias Van Ootegem Feb 25 '14 at 14:33
0
\$\begingroup\$
$theurl='./section-1.html';
function getbody1($filename) {
  $file = file_get_contents($filename);
  //$bodypattern = ".*<body>";
  //$bodyendpattern = "</body>.*";
  //$thecontainer = "id=\'container\'";
  //$thecleanup = "<div >";
  $noheader = preg_replace("/".".*<body>"."/", "", $file);
  $noheader = preg_replace("~"."</body>.*"."~", "", $noheader);
  $noheader = preg_replace("|"."id=\'container\'"."|", "", $noheader);
  $noheader = preg_replace("#"."<div >"."#", "<div>", $noheader);
  return $noheader;
}
echo getbody1("$theurl");

This might work, I just commented out the veriables you were not using and put them directly into the code.

Also there was an error in

$thecontainer = "id=\"container\"";

The second " will end the string, you need to use ' to have a string inside a string.

But honestly, I wouldn't do this, the amount of processing time/power you might save wont compare to the amount of readability you lose.

\$\endgroup\$
  • \$\begingroup\$ Ah, it seems when I copied the code it missed out some of the content in the variables. \$\endgroup\$ – PBB Feb 24 '14 at 19:00
  • \$\begingroup\$ OK, so you can see I am a complete novice. My intent is to scrape several of my own domain's pages and use php include to bring them into a single page. The original pages are created by an html generator and I want to remove everything before and including the body tag and everything after and including the ending body tag. Is there a better way? \$\endgroup\$ – PBB Feb 24 '14 at 19:29
  • \$\begingroup\$ I was trying this myself, depending on how many pages you have, this might be the smartest way. And when I did it I too used the technique you are using. With the file get and the replace. Although I was trying to scrape google plus pages. \$\endgroup\$ – Daniel Feb 24 '14 at 19:39
  • \$\begingroup\$ @Daniel: If you every want to try that again, have a look at my answer, too. regex + markup is not the smartest way... far from it actually \$\endgroup\$ – Elias Van Ootegem Feb 25 '14 at 14:30
  • \$\begingroup\$ Thanks, I will, I didn't know that about regex. \$\endgroup\$ – Daniel Feb 25 '14 at 21:35

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.