Help breaking down (explaining) some code

[zigojacko]

$CanoNi = $_SERVER['REQUEST_URI'];
$CheckCanoNi = explode('?',$CanoNi);
if($CheckCanoNi[1] != ''  && ($_REQUEST['p'] == '0' || $_REQUEST['p']=="") ){
$Conictag = "http://".$_SERVER[HTTP_HOST].$CheckCanoNi[0];
echo '<link rel="canonical" href="'.$Conictag.'"/>';
}
if($CheckCanoNi[1] != ''  && $_REQUEST['reset'] == '1' && ($_REQUEST['p'] == '1' || $_REQUEST['p']=="")){
$Conictag = "http://".$_SERVER[HTTP_HOST].$CheckCanoNi[0];
echo '<link rel="canonical" href="'.$Conictag.'"/>';
}

I'm trying to extend someone else's code that sets the canonical tags but please could someone break down what these if statements are doing please?

 

I can't for the life of me seem to set the canonical tag on pages with query parameters.

 

&& $_REQUEST['reset'] == '1' [/size]seems to set the canonical tag on ?=reset URL's but it doesn't if I change the reset to perpage for example.

 

Thanks in advance.

Edited March 19, 2014 by zigojacko

[ginerjm]

in English you are doing :

 

a)  if you have a query string AND the 'p' argument is 1 or nothing you set line 470  (although you left off the quotes on http_host)

 

b) if you have a query string AND the reset argument is 1 AND 'p' is 1 or nothing you set line 474 (and again, you are missing quotes).

 

So - is that what you expect?  (Try adding the quotes.)

[zigojacko]

Brilliant, starting to make sense. Thanks.

If the 'p' argument is 1, does that mean if it exists?

I've tested with if ($CheckCanoNi[1] != '' && $_REQUEST['perpage'] == '36')  and this works perfectly, canonical tag is rendered if the query parameter perpage=36. How could I set it to say if it contains anything at all? Thanks for your help on that.

(I didn't leave out the quotes, they just didn't exist in the first place - I've now added them my end - thanks).

[ginerjm]

No - it means it equals 1 (or possibly 'true')

 

As for your 2nd - if it is <> null, then wouldn't you assume that is equal to anything at all?

[zigojacko]

if ($CheckCanoNi[1] != '' && $_REQUEST['perpage'] == '') is if perpage=[nothing].

if ($CheckCanoNi[1] != '' && $_REQUEST['perpage'] == '36') is if perpage=36

 

I need to say if perpage contains any value... If it is null I don't think it achieves that?

 

Thanks

[ginerjm]

that's what you asked and that's what  I replied.  Actually you should be checking if isset and then check for a value.

 

if (isset($_GET['request']))

    if ($_GET['request'] <> '')

      request has a value

 

 

by the way - using $_request is not safe.  why aren't you simply using $_GET?

[zigojacko]

This is a completely bespoke ecommerce website and the code is an absolute mess. None of it has anything to do with me but unfortantely, I'm going to have to fix up some of it in order to get certain things to happen.

 

Thanks very much for taking the time to help on this.

[kicken]

Considering the body of the if statements is the same in both instances, I don't see much point in having them in the first place.

 

Near as I can tell the intention of the code is to output the canonical link tag if a query string is present. It seems to try and validate specific query string parameters but I'm not sure why since they are not used in the generation of the canonical url. The code could be effectively replaced with just:

if ($_SERVER['QUERY_STRING'] != ''){
   list($base) = explode('?', $_SERVER['REQUEST_URI']);
   $Conictag = "http://".$_SERVER['HTTP_HOST'].$base;
   echo '<link rel="canonical" href="'.$Conictag.'"/>';
}

Edit: I suppose the intent of the query parameter checks is it only outputs the tag for page 1 (which could be indicated by the 'p' parameter being missing, or 0, or 1) and not for other pages. I'm not sure what the reset parameter is for. Rather than try and figure out what is page 1 and only output the canonical URL for that page, I would change it to output the canonical URL always and just determine what URL to use based on the presence of different parameters.

 

list($base) = explode('?', $_SERVER['REQUEST_URI']);
$Canonical = 'http://'.$_SERVER['HTTP_HOST'].$base;
$parameters = array();
if (isset($_GET['p']) && $_GET['p'] > 1){
   $parameters['p'] = $_GET['p'];
}
if (isset($_GET['perpage']) && $_GET['perpage'] != 36/*or whatever your default is*/){
   $parameters['perpage'] = $_GET['perpage'];
}
if ($parameters) $Canoical .= '?'.http_build_query($parameters);
echo '<link rel="canonical" href="'.$Canonical.'"/>';

Edited March 19, 2014 by kicken

[zigojacko]

Considering the body of the if statements is the same in both instances, I don't see much point in having them in the first place.

 

Near as I can tell the intention of the code is to output the canonical link tag if a query string is present. It seems to try and validate specific query string parameters but I'm not sure why since they are not used in the generation of the canonical url. The code could be effectively replaced with just:

if ($_SERVER['QUERY_STRING'] != ''){
   list($base) = explode('?', $_SERVER['REQUEST_URI']);
   $Conictag = "http://".$_SERVER['HTTP_HOST'].$base;
   echo '<link rel="canonical" href="'.$Conictag.'"/>';
}

Edit: I suppose the intent of the query parameter checks is it only outputs the tag for page 1 (which could be indicated by the 'p' parameter being missing, or 0, or 1) and not for other pages. I'm not sure what the reset parameter is for. Rather than try and figure out what is page 1 and only output the canonical URL for that page, I would change it to output the canonical URL always and just determine what URL to use based on the presence of different parameters.

 

list($base) = explode('?', $_SERVER['REQUEST_URI']);
$Canonical = 'http://'.$_SERVER['HTTP_HOST'].$base;
$parameters = array();
if (isset($_GET['p']) && $_GET['p'] > 1){
   $parameters['p'] = $_GET['p'];
}
if (isset($_GET['perpage']) && $_GET['perpage'] != 36/*or whatever your default is*/){
   $parameters['perpage'] = $_GET['perpage'];
}
if ($parameters) $Canoical .= '?'.http_build_query($parameters);
echo '<link rel="canonical" href="'.$Canonical.'"/>';

Hah, brilliant, Yes this cuts out a ton of ridiculous code in the file and achieves the same (desired) result. Thanks very much