0
\$\begingroup\$

I made this function to test if the content has 301 redirections.. Please can you help me optimize it better because it's too slow

function contains_http_301_link($content, $current_site){
$pattern = '~<a(.*?)href="([^"]+)"(.*?)>~';
$links = array();
// Check if there is a url in the content
if(preg_match_all($pattern, $content, $urls)) {
    foreach($urls[0] as $url){
        // get external links

        if ( !strpos( $url, $current_site )) {                                   // Is an external link
            $vowels = array('"' ,"<a", " ", "href", "=", ">");
            $links[] = str_replace($vowels, "", $url);
        }
    }
}
foreach ($links as $link) {
    $_headers = @get_headers($link,1); 
    if($_headers && strpos( $_headers[0], '301')) { 
        return true;
    } 
}
return false;
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ How did you isolate the performance issue to this function? What do you consider too slow? \$\endgroup\$
    – pacmaninbw
    Mar 5, 2021 at 12:10
  • \$\begingroup\$ @pacmaninbw it works in the local but it gets crashed on production website due to timeout \$\endgroup\$
    – s_a_i_d
    Mar 5, 2021 at 13:51
  • 2
    \$\begingroup\$ This title does not describe what your code does. \$\endgroup\$ Mar 5, 2021 at 13:59
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Mar 5, 2021 at 16:15

1 Answer 1

1
\$\begingroup\$

I have never attempted such a task, but I will assume that you entertained the cURL techniques (including CURLOPT_NO_BODY and CURLINFO_HTTP_CODE) as suggest at 1 and 2.

As for parsing an html document, using regex may outperform a legitimate parser, but you might enjoy more accurate scaping if you use a DOM parser.

Since your focus is on performance, I'll [cringe] suggest regex to parse your html.

Your pattern ~<a(.*?)href="([^"]+)"(.*?)>~ uses lazy quantifiers and this can slow things down a little. Also, your <a(.*?)href will allow the matching of <a data-href and <abbr href -- which is not what you intend.

Your intention is to capture ONLY the url inside of the href attribute, so don't bother matching more than you need. You can probably get away with a wordboundary after a and a space before href, then reset the fullstring match with \K to only retain the href's value. If this isn't strong enough validation, then you could add a lookahead to confirm that the opening tag is completed properly.

  • ~<a\b.*? href="\K[^"]+~ or
  • ~<a\b.*? href="\K[^"]+(?=".*?>)~

Either way, your code can skip the sanitizing step. Thank goodness because $vowels contains data that are not all vowels.

strpos() can return false or 0 as the offset. This means that you must not rely on the type juggled condition using merely !. You must explicitly check for false or if you are on a php version that enjoys str_contains() -- that'll work.

I personally condemn the use of the "stfu operator" (@), but because I am not familiar with this task, I'll avoid calling it an abomination.

function contains_http_301_link(string $content, string $current_site): bool
{
    preg_match_all('~<a\b.*? href="\K[^"]+~', $content, $matches);
    foreach ($matches[0] as $url) {
        if (strpos($url, $current_site) !== false) {
            continue;
        }
        $headers = @get_headers($url, 1);
        if($headers && strpos($headers[0], '301') !== false) {
            return true;
        } 
    }
    return false;
}

I could possibly include the $current_site filtration into the regex but it may do more harm than good.


If you were going to entertain (and benchmark) a DOM parser script, then perhaps use this:

function contains_http_301_link(string $content, string $current_site): bool
{
    $dom = new DOMDocument; 
    $dom->loadHTML($content);
    $xpath = new DOMXPath($dom);
    foreach ($xpath->query("//a[not(contains(@href, '$current_site'))]/@href") as $href) {
        $headers = @get_headers($href->nodeValue, 1);
        if($headers && strpos($headers[0], '301') !== false) {
            return true;
        } 
    }
    return false;
}
\$\endgroup\$

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.