Improve search keyword

Question

I've been given the following PHP code:

if(empty($search)){
 $_SESSION['keyword_exists'] = "no";
 }else{
 if(isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search){
 $_SESSION['keyword_exists'] = "no";
 }
 }
 if(isset($_REQUEST['limitstart']) && $_REQUEST['limitstart'] > 0){
 if (!empty($search)) {
 if(isset($_SESSION['keyword_exists']) && $_SESSION['keyword_exists'] === "yes"){
 //intentionally left blank?
 }else{
 $limitstart = "0";
 $_SESSION['keyword_exists'] = "yes";
 $_SESSION['old_keyword'] = $search;
 }
 }
 }else{
 $limitstart = "0";
 }

I'm not entirely sure what this code does, as I'm fairly new to PHP and webdev. The one comment is mine. Is there any way to clean this up so that the next guy to see this is less confused than I am?

Answer 1

Let's start with the basics:

• $_SESSION: This is a global array that holds all your session information.
• $_REQUEST: This is a global array that contains the contents of $_GET, $_POST and $_COOKIE. Read more on PHP's superglobals.

if( empty($search) ) {
 $_SESSION['keyword_exists'] = "no";
} else {
 if( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search ) {
 $_SESSION['keyword_exists'] = "no";
 }
}

The outer if depends on whether the $search variable is empty or not. Empty in context could mean:

• "" (an empty string)
• 0 (0 as an integer)
• 0.0 (0 as a float)
• "0" (0 as a string)
• NULL
• FALSE
• array() (an empty array)
• var $var; (a variable declared, but without a value in a class)

If $search is empty, $_SESSION['keyword_exists'] is set to "no" (can't read the author's mind, but my choice would be false, not a string). From the naming we can assume that $search holds (or not) the keyword(s) of a search. Now if that is not empty:

 if( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search ) {
 $_SESSION['keyword_exists'] = "no";
 }

The first clause, isset($_SESSION['old_keyword']) essentially checks if there's an "old_keyword" index in the $_SESSION array (and whether it's null or not), that's a pretty typical check for arrays. The second check, that executes if and only if the first one passes, checks whether what's in $_SESSION['old_keyword'] is not the same as what's in $search. I'm assuming that the author had some reason for that, but can't imagine what that reason is.

Summarizing what happens here, if:

• $search is empty, or
• $search is not the same as $_SESSION['old_keyword']

...then $_SESSION['keyword_exists'] = "no". A perhaps simpler way to write that would be:

if(
 empty($search)
 || ( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search )
) {
 $_SESSION['keyword_exists'] = "no";
}

Moving on to the next part:

if( isset($_REQUEST['limitstart']) && $_REQUEST['limitstart'] > 0 ) {
 ...
} else {
 $limitstart = "0";
}

The clause is very similar to the one discussed previously, only this time other than checking if $_REQUEST has a "limitstart" index, the author also checks that the value is larger than zero. That's an unsafe check, because at this point we don't now what the type of the value in $_REQUEST['limitstart'] is, and if it's anything other than a number, there will be automatic type juggling involved, and the check is completely unreliable.

From the name and context, I'm assuming the variable should hold an integer (if anything) that limits the search. If the variable doesn't hold anything, the limit is set to zero ($limitstart = "0"), curiously using a string form of zero. I'd rewrite that check as:

$limitstart = 0;
if(
 isset($_REQUEST['limitstart']) 
 && ( is_int($_REQUEST['limitstart']) || ctype_digit($_REQUEST['limitstart']) )
 && $_REQUEST['limitstart'] > 0
) {
 ...
} 

is_int() checks whether the value is an integer and ctype_digit() whether it's a string that only contains digits (thus a integer in string form), any of the two is acceptable for the following check, $_REQUEST['limitstart'] > 0. I've also moved $limitstart = 0; out of the check, I'm initializing it to zero and will override if and only if there's a need. But let's see what happens if the check is true:

 if ( !empty($search) ) {
 if( isset($_SESSION['keyword_exists']) && $_SESSION['keyword_exists'] === "yes" ) {
 //intentionally left blank?
 } else {
 $limitstart = "0";
 $_SESSION['keyword_exists'] = "yes";
 $_SESSION['old_keyword'] = $search;
 }
 }

This is obviously incomplete. The outer check is simple enough, it's whether $search is empty or not. If it's empty, nothing happens, if it's not, the inner check is on whether _SESSION has a "keyword_exists" index, and if it's value is "yes". If that's true, something happens, but who knows what? I'm assuming one of the things that would happen would be to set a proper value to $limitstart. Anyways, if the check is false:

• $limitstart remains zero,
• $_SESSION['keyword_exists'] for some reason becomes "yes", and
• $_SESSION['old_keyword'] becomes $search.

Unfortunately, I have no idea why. Anyways, my full rewrite would be:

if(
 empty($search)
 || ( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search )
) {
 $_SESSION['keyword_exists'] = "no";
}
$limitstart = 0;
if(
 isset($_REQUEST['limitstart']) 
 && ( is_int($_REQUEST['limitstart']) || ctype_digit($_REQUEST['limitstart']) )
 && $_REQUEST['limitstart'] > 0
) {
 if ( !empty($search) ) {
 if( isset($_SESSION['keyword_exists']) && $_SESSION['keyword_exists'] === "yes" ) {
 //intentionally left blank?
 } else {
 $_SESSION['keyword_exists'] = "yes";
 $_SESSION['old_keyword'] = $search;
 }
 }
} 

The code is equivalent, and will work (?) if you replace it in your script. Hope it clarifies things a bit. The overall quality of the code is bad, there are some hints of an amateur developer there, and you shouldn't really worry that you didn't grasp what the code does, since you are unfamiliar with the language. It's an incomplete and mostly poorly written piece of code, good luck with it ;)

• SomeKittens: Thanks a lot, I learned quite a bit from this. I wish I could give you more than +25 rep, you deserve it. Thanks for the rewrite. It feels good knowing I'm not the only one confused by this (outsourced) code.
• mseancole: @SomeKittens: I'd only like to add that using filter_input() would look cleaner compared to is_int() and ctype_digit(). And also that $_REQUEST is not a very secure array to use. Using the proper array ($_POST, $_GET, etc...) if you know the source would be much better. Otherwise, this is a very good review +1