patternphpMinor
Framed external link redirect - vBulletin
Viewed 0 times
framedredirectvbulletinexternallink
Problem
I'm just looking for some review for my code. This is a script that checks for outbound links in posts in vBulletin forums, and if it is an external link it will redirect to a template which will load the external site in an iFrame that I also coded, inspired by themeforest and stumbleupon's way of showing external links.
This is my first PHP script on my own that is a little more advanced. I'm only use to the basics, so any critique is good critique, it's the only way I can learn.
I don't even know if I went about doing this correctly, but it seems to work how I'm wanting currently. Let me know what I can do differently.
To get an idea of this you can see it in action - Here
HTTPS links don't work, however, I still want them redirected with this so all outbound links on my site will derive from the same page, so I included a button for if a certain page won't load you can click that button and it will take you to the page.
I would like to add a few things to it still but first wanted to get some suggestions on my code.
This is my first PHP script on my own that is a little more advanced. I'm only use to the basics, so any critique is good critique, it's the only way I can learn.
I don't even know if I went about doing this correctly, but it seems to work how I'm wanting currently. Let me know what I can do differently.
To get an idea of this you can see it in action - Here
HTTPS links don't work, however, I still want them redirected with this so all outbound links on my site will derive from the same page, so I included a button for if a certain page won't load you can click that button and it will take you to the page.
$curdir = getcwd ();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');
if ($_GET) {
foreach ($_GET as $key => $value) {
if ($key=="l") {
$url1 = $value;
}
else {
$url2 = "&". $key. "=". $value;
}
}
$out = strip_tags($url1. $url2);
}
$site = parse_url($_SERVER['HTTP_HOST']);
$site = $site[path];
$url = parse_url($out);
if ($url[scheme] == "http" OR "https" && $url[host]) {
if ($site == $url[host]) {
Header( "Location: ". $out );
}
else {
eval('print_output("' . fetch_template('drc_redirect') . '");');
}
}I would like to add a few things to it still but first wanted to get some suggestions on my code.
Solution
Here's how I optimized your code:
Below are the modifications that I did:
Explanations in detail:
$curdir = getcwd();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');
if (isset($_GET['l'])) {
//format passed url
$passed_url = $_GET['l'];
unset($_GET['l']);
if(count($_GET))
$passed_url = $passed_url . '&' . http_build_query($_GET);
//deals w/ the display if $passed_url is a valid URL
if (filter_var($passed_url, FILTER_VALIDATE_URL)) {
$site = parse_url($_SERVER['HTTP_HOST']);
$site = $site['path'];
$url = parse_url($passed_url);
//the passed url is not an external link
if ($site == $url['host']) {
Header( "Location: ". $_SERVER['QUERY_STRING'] );
}
//the passed url is an external link
else {
eval('print_output("' . fetch_template('drc_redirect') . '");');
}
}
}Below are the modifications that I did:
- Using
isset()for checking$_GET.
- Using
http_build_query()instead of looping through each element in$_GET.
- Using
filter_var()to check if the passed URL is valid.
Explanations in detail:
isset()allows for proper checking of the intended variable from$_GET. It also returns a boolean for the if condition, as opposed to just placing$_GETwhich is an array.
- The looping of the
$_GETvariable will backfire if there are at least 2 elements in it. The else condition on theforeachloop overwrites the previous values for$url2. Now this can be corrected to having every$key/$valuebe concatenated everytime. We can utilizehttp_build_query()instead because it can get the job done, and there's no need to loop the$_GETarray.
- Finally,
parse_url()sometimes is not able to return the scheme of a given url string.filter_var()along with theFILTER_VALIDATE_URLfilter can validate URLs regardless whether the scheme was specified or not.
Code Snippets
$curdir = getcwd();
chdir('./');
define('NO_REGISTER_GLOBALS', 1);
define('THIS_SCRIPT', 'drc-redirect');
$globaltemplates = array('drc_redirect');
require_once('./global.php');
if (isset($_GET['l'])) {
//format passed url
$passed_url = $_GET['l'];
unset($_GET['l']);
if(count($_GET))
$passed_url = $passed_url . '&' . http_build_query($_GET);
//deals w/ the display if $passed_url is a valid URL
if (filter_var($passed_url, FILTER_VALIDATE_URL)) {
$site = parse_url($_SERVER['HTTP_HOST']);
$site = $site['path'];
$url = parse_url($passed_url);
//the passed url is not an external link
if ($site == $url['host']) {
Header( "Location: ". $_SERVER['QUERY_STRING'] );
}
//the passed url is an external link
else {
eval('print_output("' . fetch_template('drc_redirect') . '");');
}
}
}Context
StackExchange Code Review Q#51767, answer score: 2
Revisions (0)
No revisions yet.