3
\$\begingroup\$

This code performs 301 redirects based on the taxonomy terms associated with the node, as 301 redirects can adversely effect page load time. How could this be best optimized?

 <?php
    /**
     * Implementation of hook_init().
     *calling this hook causes the code below to execute before the page is loaded
     */


    function region_filter_init() {}
     //hook "nodeapi" is called when we are doing something to the node, in this "case" we are "viewing" the node.



    function region_filter_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {

      switch ($op) {

        case 'view':


            if( (arg(1) == 0)){ } //do not execute on homepage. if there are no aruments after the base url in arument one is thedelimagazine.com/argument1, if this is 0 then do nothing.


          else {

            $region = getRegion(); //get current region from deli.mmodule

            $national = 'national'; //store string 'national' in variable

            $count = count($node->taxonomy);//nstores number of key/value pairs in array,  if two terms are selected with the node/post then count == 2

            $terms = array();//initialize terms to array
            foreach($node->taxonomy as $term) {    
              $terms[$term->name] = $term->name; //prints array of individual terms




            }


        /*case 1 new england*/

        //if there are less than 9 terms  and more than 2 and new england is in the array redirect to new england
        if( $count < 9 &&  $count > 2 && in_array('newengland', $terms) && $region != 'newengland')
        {
          $link = 'http://newengland.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);
        }
        /*END case 1 new england*/



          /*case 2*/
              if($count == 2 && in_array('national', $terms) && is_numeric(arg(1))) { //if there are 2 terms in taxonomy && national is one of them & the first argument in url is a number such as thedelimagazine.com/12345 (before alias redirect)
              unset($terms['national']);        //remove national from array
              $value = array_shift($terms) ;//set value to the other term left. in array 
              if($value == 'los angeles') {//fix for los angelas, because in deli module taxonomy term and subdomain are different only for los angeles
                $value = 'la';
              }      

              if($region != $value) { //if region does not already = term (would cause redirect loop)

                  $link = 'http://' . $value . '.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//go to term left in $value

                  drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);//drupal redirect function
              }         
            }

            /* ++ END case 2++*/


            /* ++case 3 national+++*/ 
            else {

        if($count > 10  && in_array('national', $terms)  && $region != 'national' ) {    //if there are moer than 10 terms and national is in the array $terms, and the current region does not already =national           
              $link = 'http://national.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//then set $link to national.thedelimagazine.com.  drupl get path alias gets the path to the node and node id and converts it to clean readable urls that i implemented last year.
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);     //go to the $link
        }
             /* ++ END case 3 national++*/



             }
         }
          break;
      }
    }
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
    function region_filter_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {

      switch ($op) {

        case 'view':


            if( (arg(1) == 0)){ } //do not execute on homepage. if there are no aruments after the base url in arument one is thedelimagazine.com/argument1, if this is 0 then do nothing.

You get an extra pair of parens, and it would be easier to to check for arg(1) ~= 0 rathern the have an empty if block.

          else {

            $region = getRegion(); //get current region from deli.mmodule

            $national = 'national'; //store string 'national' in variable

Don't write dumb comments. Assume the reader understands PHP. Your comment should explain why you want to do that, not reiterate the code.

            $count = count($node->taxonomy);//nstores number of key/value pairs in array,  if two terms are selected with the node/post then count == 2

            $terms = array();//initialize terms to array
            foreach($node->taxonomy as $term) {    
              $terms[$term->name] = $term->name; //prints array of individual terms




            }

That was a lot of pointless empty lines.

        /*case 1 new england*/

        //if there are less than 9 terms  and more than 2 and new england is in the array redirect to new england
        if( $count < 9 &&  $count > 2 && in_array('newengland', $terms) && $region != 'newengland')

Again, your comment simply tells me what I could have already gotton from the code. But I'm left thinking: WHAT? What's the deal with the number of terms? What am I checking in the array? What has $region got to do with it?

        {
          $link = 'http://newengland.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);

Keep your indentation consistent. } /END case 1 new england/

Don't put end comment like that. I can already see the case is ending thanks to the } and your indentation.

          /*case 2*/
              if($count == 2 && in_array('national', $terms) && is_numeric(arg(1))) { //if there are 2 terms in taxonomy && national is one of them & the first argument in url is a number such as thedelimagazine.com/12345 (before alias redirect)

Again, don't repeat your code in the comments. Explain why your code is doing that.

              unset($terms['national']);        //remove national from array
              $value = array_shift($terms) ;//set value to the other term left. in array 
              if($value == 'los angeles') {//fix for los angelas, because in deli module taxonomy term and subdomain are different only for los angeles

See those are the comments that are helpful, because it explains what so special about los agenles.

                $value = 'la';
              }      

              if($region != $value) { //if region does not already = term (would cause redirect loop)

Here I'd just say // avoid redirect loop

                  $link = 'http://' . $value . '.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//go to term left in $value

                  drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);//drupal redirect function

This looks pretty much the same as the last case. You should refactor the code. }
}

            /* ++ END case 2++*/


            /* ++case 3 national+++*/ 
            else {

        if($count > 10  && in_array('national', $terms)  && $region != 'national' ) {    //if there are moer than 10 terms and national is in the array $terms, and the current region does not already =national           
              $link = 'http://national.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//then set $link to national.thedelimagazine.com.  drupl get path alias gets the path to the node and node id and converts it to clean readable urls that i implemented last year.
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);     //go to the $link
        }
             /* ++ END case 3 national++*/



             }
         }
          break;
      }
    }

It's been so long so I read the matching pieces of code that I have no idea what these pieces much up to. That a sign that your code is overly complicated.

Here is a quick reworking of the code:

<?php
    // return the region that the user should be on for this node
    function determine_region($node)
    {

        $taxonomy_count = count($node->taxonomy);

        // I doubt this is neccesary, I don't know what
        // $node->taxonomy is, but perhaps you can use that rather
        // then copying the data into an array.
        $terms = array();
        foreach($node->taxonomy as $term) {    
            $terms[$term->name] = $term->name; 
        }

        // I really have no idea what the logic behind the region is
        if($taxonomy_count == 2 && in_array('national', $terms) && is_numeric(arg(1))) 
        { 
            $region = array_shift($terms);
            // los ageneles needs special case
            if($region == "los angeles")
            {
                $region = 'la';
            }
            return $region;
        }
        else if($taxonomy_count > 2 && $taxonomy_count < 9 && in_array('newengland', $terms)
        {
            return 'newengland';
        }
        else if($taxonomy_count > 10 &&  in_array('national', $terms) )
        {
            return 'national';
        }
        else
        {
            // none of the cases fit, we'll just keep the current region
            return getRegion();
        }

    }

    // redirect the user to a particular region and node
    function send_redirect($region, $node)
    {
        $link = 'http://' . $region . '.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//go to term left in $value
        drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);
    }

    function region_filter_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
        // if viewing something other then the home page, consider redirecting
        if($op == 'view' && arg(1) != 0)
        {
            $correct_region = determine_region($node);
            // redirect to correct region if necessary.
            if( getRegion() != $correct_region )
            {
                send_redirect($correct_region, $node);
            }
        }
    }
?>

I think you'll find my version easier to follow. I've divided to task into pieces so that each piece can be simpler.

This code performs 301 redirects based on the taxonomy terms associated with the node, as 301 redirects can adversely effect page load time, how could this be best optimized.

The solution is not to do 301 redirects. Telling the browser to go download another page will make loading the page slower. Nothing you can do in your code will change that. All you can do is not tell them to go download another page. Do you really need to redirect the user to a different subdomain? Maybe you do, but if you can get away without doing it, don't do it.

\$\endgroup\$
4
  • \$\begingroup\$ arg(1) ~= 0 ??? What am I missing? \$\endgroup\$
    – yannis
    Commented Jan 1, 2012 at 16:19
  • \$\begingroup\$ @YannisRizos, language confusion. ~= means != in some languages. \$\endgroup\$ Commented Jan 1, 2012 at 16:50
  • \$\begingroup\$ sorry the comments were written for a non technical person to read \$\endgroup\$
    – albo
    Commented Jan 2, 2012 at 21:28
  • \$\begingroup\$ this doesn't really make sense to what I need to do, but thanks for taking a look \$\endgroup\$
    – albo
    Commented Jan 2, 2012 at 21:29

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.