6
\$\begingroup\$

I am building a PHP framework and would like to get some feedback on a few different sections of the project so far. I consider myself still a neophyte in PHP so I would like to ask if I'm going about completing these different tasks in an efficient and or correct way.

I am currently working on a session management class. What I have done is create methods that deal with a specific action when dealing with sessions. Pinoniq has suggested to me:

Always think this: everything you code must solve some kind of problem. If it doesn't solve a problem. Remove it.

So I kept that in mind when creating this class.

What I am trying to do in this class is create methods that do most of the work for you when creating and working with sessions.

Methods like namePattern() and ParamBreak() are operations for use inside the class class method.

class Sessions {
/**
 * Start a session
 * 
 * @param $id string - Set sestion id
 * @return boolean or trigger_error
 */
public function StartSession($id = null){
    if(!session_start()){
        trigger_error('Unble to create session',E_USER_WARNING);
    }
    if($id != null && is_string($id)){
        //Check pattern to make sure it's supported
        if($this->namePattern($id)){
            return session_id($id);
        }
    }
    return true;
}
/**
 * Create a session value in the $_SESSION array
 * if key is not supplied then one will be
     * created and incremented
 * automaticly 
 * 
 * @param $key string - Index key for array
 * @param $value mixed - value to be stored
 */
public function CreateSession($key = null,$value = null){
    if(!$key && !$value){
        trigger_error('Missing argument for Sessions::CreateSession()');
    }
    if(empty($value)){
        $value = $key;
        $key = null;
    }
    $_SESSION[$this->FindSessionKey($key)] = $value;
    return true;
}
/**
 * Return or set the session_id
 * 
 * @param $id string - string to set the id to
 * @return current session_id
 */
public function SessionId($id = null){
    if(isset($id)){
        return session_id($id);
    }
    return session_id();

}
/**
 * Check if the key supplied exists in the $_SESSION array
 * if not increment to the next value
 * 
 * 
 * @param $key mixed - 
 */
private function FindSessionKey($key){
    if(!empty($_SESSION)){
        $index = count($_SESSION)-1;
        if(!isset($key)){
            return $index+1;
        }else{
            if(array_key_exists($key,$_SESSION)){
                return $index+1;
            }else{
                return $key;
            }
        }
    }
    if(isset($key)){
        return $key;
    }   
    return 0;
}

public function SearchSessions($key = null,$value = null){

}
/**
 * Remove a session using a key as the needle
 * 
 * @param $key mixed - key to find the value
 * @return void
 */
public function RemoveSessionByKey($key){
    if(is_array($key) || ($key = $this->ParamBreak($key))){
        for ($i=0; $i < count($key); $i++) {
            unset($_SESSION[$this->KeyCycle($key[$i])]);
        }
    }else{
        unset($_SESSION[$this->KeyCycle(func_get_arg(0))]);
    }
}
/**
 * Find a return a key value
 * 
 * @param $key mixed - key to compare to
 * @return $key found
 * 
 * To be honest looking at this again I
 * have no idea whats going on. It works
 * but "WHAT WAS I DOING?" I don't know.
 */
public function KeyCycle($key){
    if(is_string($key)){
        $key = trim($key);
    }
    while(current($_SESSION) !== false){
        if($key == ($index = key($_SESSION))){
            //Prevent the removing of index 0
            //if $key index are not found
            if($key == 0 && $index == $key){
                return $index;
            }
            return $index;
        }
        next($_SESSION);
    }
}
/**
 * Remove a unset a session value using its
 * value as the needle
 * 
 * @param $value string - 
 */
public function RemoveSessionByValue($value){
    if(is_array($value) || ($value = $this->ParamBreak($value))){
        for ($i=0; $i < count($value); $i++){
            for($j=0; $j < count($_SESSION); $j++){
                unset($_SESSION[$this->ValueCycle($value[$i])]);
            }   
        }
    }else{
        for($j=0; $j < count($_SESSION); $j++){
            unset($_SESSION[$this->ValueCycle(func_get_arg(0))]);
        }
    }
}

protected function ValueCycle($value){
    if(is_string($value)){
        $value = trim($value);
    }
    while(current($_SESSION) !== false){
        if($value === current($_SESSION)){
            return key($_SESSION);
        }
        next($_SESSION);
    }
}

public function RemoveSessionByValuei($pattern){
    for ($i=0; $i < count($_SESSION) ; $i++) {
        if(is_array($_SESSION[$i])){
            for ($j=0; $j < count($_SESSION[$i]); $j++) {
                if(preg_match($pattern,$_SESSION[$i][$j],$match) > 0){
                    unset($_SESSION[$i][$j]);
                    $this->RemoveSessionByValue($match[0]);
                }
            }
        }else{
            if(preg_match($pattern,$_SESSION[$i],$match) > 0){
                unset($_SESSION[$i]);
            }
        }
    }
}

public function ReturnSession($key){
    if(array_key_exists($key, $_SESSION)){
        return $_SESSION[$key];
    }
    return false;
}

public function ReturnAllSessions(){
    if(!empty($_SESSION)){
        return $_SESSION;
    }
    return false;
}

public function ShowSessions(){
    return var_dump($_SESSION);
}

public function DestroySessions(){
    return session_destroy();
}

protected function ParamBreak($string){
    if(strpos($string,',') !== false){          
        return explode(',', $string);
    }
    return false;
}

private function namePattern($string){
    if(preg_match('/^([a-zA-Z0-9\,\-])+$/',$string) > 0){
        return $string;
    }
    return false;
}

}
\$\endgroup\$
5
\$\begingroup\$

First off the code is hard to make sense of, because I only see part of it. Most importantly I don't have the source for SessionsStruction class, which you are extending, and the name "Struction" doesn't make any sense to me (is it even in dictionary?).

It would also help to have a code sample of using this class.

The class also has no documentation describing its purpose - and after reading it all, I'm still wondering what's this all about.

Some immediate problems I see with this class:

  • It has too many responsibilities - parsing config file and debugging the sessions data (in ShowSessions method) should be handled outside of this class.

  • It has too wide API - an entire 13 public methods. Do you really need to provide such fine-grained interface?

  • It depends on global state: APP_PATH, CURRENT_FILE are defined somewhere else. Consider passing them in as parameters instead. Also the $_SESSION var.

  • why the empty __destroy() and SearchSessions() methods?

  • why the RemoveSession() which just delegates to RemoveSessionByKey()?

  • why the unused $active and $maltivalue class variables?

  • you really should check your spelling.

  • -
\$\endgroup\$
  • \$\begingroup\$ Thank you for your feedback. I have added more to the post in hope that it will help. I have removed the portions of the class that are not needed to explain and run the class. \$\endgroup\$ – andrewnite Apr 7 '14 at 6:11
  • \$\begingroup\$ Now that I've seen some examples of using this class, it makes me wonder, why should one use this whole complicated thing instead of directly using the $_SESSION variable? All the features it adds, look kinda weird and not very useful. Like when I ask it to create a field with already existing name, it assigns a numeric field instead, but I have no way of knowing, which field did it assign to. Why would I ever want that? \$\endgroup\$ – Rene Saarsoo Apr 7 '14 at 21:41
  • \$\begingroup\$ I understand, thank you for your comment. I am building a PHP framework to learn and experiment with PHP. I am currently working on the Session handling portion of it. I am posting my code to get feedback on its structure and code layout in hope to get any suggestions or advice on how it might be better. \$\endgroup\$ – andrewnite Apr 8 '14 at 3:29
2
\$\begingroup\$

First of I think you have written the start of some good functionality, but there are some aspects of the session that I think you have interpreted a little wrong.

The session is a mechanism is designed be very generic while storing data specific to an id. This id could then be linked to a login or something similar. But the values of an session could be anything or be stored in any order and this are therefore never known. So you cannot rely of the value if you do a look up, in fact that would ruin the purpose of sessions in the first place. If you already know the value, then why use sessions? Consider from the mentioned example about a login where a username and a user id (an integer) is stored. The user id is not always the first element in the sessions array, so a call to $_SESSION[0] could possibly return the username (a string) and make trouble in for code because of the wrong type and wrong values. Because of such inconsistencies session elements are always stored with an unique key, which can be used for retrieval. When I say session can store anything keep in mind that best practice is to keep only what is required in the session. Remember the session can be hijacked by attackers and a session with a bunch of information takes longer to load, this is especially true when using file- or database-based storing mechanism.

Now to the code itself. Thinking about the things I have outlined above, some of your methods should be restructured or lose their purpose entirely. First the FindSessionKey() method. Because all values should be associated with a KNOWN unique key, auto-incrementing (while making the key that started as a string an integer) an array key at an unknown index almost spells trouble for you down the road. How would you retrieve that again? With the key (string) that does not work or an unknown integer that could change from time to time? This leads me the to CreateSession() method. Here the key and value arguments are optional (why both?). Such should not happen. If you do not have a value then why store it? And if you do not have an unique key then how would you retrieve the value again? The value could be nice car as well as bumble bee or any other thing you can imagine. This would be quite hard to code. For the same reasons KeyCycle() (as you said yourself, what is it doing?), RemoveSessionByValue(), ValueCycle and RemoveSessionByValuei() should be removed as we do not rely on the value.

Now when all these methods that could spell trouble are removed, you have a good foundation, but there is more (there always is :D). The SearchSessions() method could be extracted into its own class along with other utility methods. I have utility class named SessionUtilities which declares the methods increment($key, $interval = 1) and decrement($key, $interval = 1) (increment() and decrement() checks if the value is an integer) and a compare($key, $value, $strict = false) etc. These are used for easier manipulation of the session array.

Now to the code structure. The FindSessionKey method could use early returns to make the code more readable. I feel like the RemoveSessionByKey() method should only take one key. This will make your calling code a little more verbose, but help readability as another developer (or yourself) shouldn't keep track of which values are stored in the array defining the keys to be removed. When we have removed this feature the ReturnSession() method does the same, but with less code. If you want the functionality to remove from an array, then make it clear in the method name and arguments list. Example: RemoveSessionByArray(array $keys)?

The StartSession() method could be extracted to another class which would give you more flexibility. PHP provides a way to overwrite the standard behavior when working with sessions. Take a look at the SessionHandler class or the SessionHandlerInterface interface. By creating a class extending the SessionHandler or implementing the SessionHandlerInterface you wouldn't have to write methods operations such as opening, closing and destroying the session as the native PHP functions would use your implementation. Now session_start(), session_write_close() and session_destroy() all responds to the behavior you have provided. This will also make it easier for other developers not aware of your class, as they can stick with the native functions.

Then you should add some error handling. Consider using a constructor that can check if sessions are enabled at all before using the class.

public function __construct() {

    $status = session_status();

    // Check if sessions are enabled.
    if($status == PHP_SESSION_DISABLED) {
        throw new \LogicException('Sessions are disabled.');
    }

    // Check if sessions are started.
    if($status == PHP_SESSION_NONE) {
        session_start();
    }

}

Now you have checked if sessions are enabled on the server and made sure that a session is active before working with them. See the session_status() documentation for more information.

Now to some less important things. Experience have learned me that methods which can both set and get are very prone to errors. Consider your method SessionId(). If you provide an $id it will get, otherwise it will set the id. Consider making that two different methods setSessionId($id) and getSessionId(). That will make your code more readable as well. Even though we have removed the KeyCycle() and ValueCycle() methods I am going to write a little about their visibility. Methods only used by the class should be declared private. That is also true of properties. A good rule is to keep as many methods private and only declare internal methods protected if they are known to be used by child classes. If the situation arises where a private method could be used by a child class, then change the visibility of that method to protected. The reason is that child classes can now declare their own methods of the same name (helps if the name is used often) and child classes is now unable to overwrite the behavior (change the return type etc.) of methods the parent class relies on. At last before I will give you my version of a session class I must write a little about naming.

As said in one of the other posts, method/function names usually starts with lowercase letters. Then it is up to you if you want to use the CamelCase or Snake_case naming convention. Usually in PHP only clasess, abstract classes, interfaces and traits starts with an uppercase letter. And a for the purists, abstract classes, interfaces and traits also have appended their type to the name such as ClassnameAbstract, SomeTrait or AwesomeInterface. Taking all that in consideration I think you can rename some of your methods. You often write methods where the word session takes place. Consider removing this entirely as we already know what class we are working with, the Sessions class. A good rule is to find the shortest, BUT most describing name. So FindSessionKey could be renamed to find or get? And the CreateSession could be renamed to set($key, $value)? Even though the shortest name is preferred don't make it an excuse for naming your methods or variables $i, i() or getFOrDName (get file or directory name) as that has no indication of what is doing/storing what so ever. The same goes for the RemoveSessionByKey. As we only work on keys in the session the ByKey part should be self evident and the method could be renamed to remove($key)?

That was quite a lot, so I have written a simple class as an example of all that I have mentioned.

class Session {

    public function __construct() {

        $status = session_status();

        if($status == PHP_SESSION_DISABLED) {
            throw new \LogicException('Sessions are disabled.');
        }

        if($status == PHP_SESSION_NONE) {
            session_start();
        }

    }

    /**
     * Gets a session value associated with the specified key.
     *
     * @param string $key
     *
     * @return mixed|null Returns the value on success. NULL if the key doesn't exist.
     */
    public function get($key) {
        if(array_key_exists($key, $_SESSION)) {
            return $_SESSION[$key];
        }
        return null;
    }

    /**
     * Set a new session elements or update an existing one.
     *
     * @param string $key
     * @param mixed  $value
     */
    public function set($key, $value) {
        $_SESSION[$key] = $value;
    }

    /**
     * Deletes a session element.
     *
     * @param string $key
     *
     * @return bool
     */
    public function delete($key) {
        if(array_key_exists($key, $_SESSION)) {
            unset($_SESSION[$key]);
            return true;
        }
        return false;
    }

    /**
     * Determines if a session key exists.
     *
     * @param string $key
     *
     * @return bool
     */
    public function exists($key) {
        return array_key_exists($key, $_SESSION);
    }

}

And a minor thing. In the KeyCycle() method you have written a comment. This is good, but remember to use the correct comment type. Single line comments are indicated by

// This is a single line comment.

and multi-line comments are indicated by:

/*
 * This is a multi-line
 * comment.
 */

But be aware the PHP documentation body looks like the multi-line comment, but starts with two star symbols /** and uses a clearly defined syntax.

I hope this can guide you, happy coding!

\$\endgroup\$
  • \$\begingroup\$ Thanks @anotherguy for the feedback. Once again very helpful. \$\endgroup\$ – andrewnite Mar 11 '15 at 10:08
  • \$\begingroup\$ This is what I call "some free time" :) \$\endgroup\$ – sitilge Dec 20 '15 at 18:58
1
\$\begingroup\$

Here is some examples in how this class can be used

$session = new Session();
//Start new session
$session->StartSession();

//Print sessions  ID
echo "SESSION ID: ".$session->SessionID();

$array = array(1=>'One',2=>'two',3=>'three', 4=>4);

 //Add value to session array
$session->CreateSession('Array',$array);
$session->CreateSession(1,'Zim');
$session->CreateSession('Tak');
$session->CreateSession('Gaz');

//.Uset session with ID 1
$session->RemoveSessionByKey(1);

//return value $array
$value = $session->ReturnSession('Array'); 

//Destroy session
$session->DestroySessions();


echo '<pre>';
echo $session->ShowSessions(); 
echo '</pre>';
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.