3
\$\begingroup\$

Are there any aspects I could improve of my HttpSession wrapper class?

Other suggestions are also welcome.

namespace Http;

class HttpSession
{
    private $data;

    public function __construct($name = null, array $iniSettings = [])
    {
        if ($name) {
            session_name($name);
        }

        foreach ($iniSettings as $key => $value) {
            ini_set($key, $value);
        }
    }

    public function findParameter($name)
    {
        $this->start();

        return isset($this->data[$name]) ? $this->data[$name] : null;
    }

    public function setParameter($name, $value)
    {
        $this->start();

        $this->data[$name] = $value;
    }

    public function unsetParameter($name)
    {
        $this->start();

        unset($this->data[$name]);
    }

    public function regenerateId($deleteOldSessionFile = true)
    {
        $this->start();

        session_regenerate_id($deleteOldSessionFile);
    }

    public function destroy()
    {
        $this->start();

        $this->data = [];

        if (isset($_COOKIE[session_name()])) {
            $scParams = session_get_cookie_params();

            setcookie(
                session_name(), '', time() - 3600,
                $scParams['path'],
                $scParams['domain'],
                $scParams['secure'],
                isset($scParams['httponly'])
            );
        }

        session_destroy();
    }

    private function start()
    {
        if (isset($this->data)) {
            return null;
        }

        if (!isset($_SESSION)) {
            session_start();
        }

        $this->data = &$_SESSION;
    }
}
\$\endgroup\$
1
\$\begingroup\$

There are a few things I see:

  • Work in the constructor - Don't do work in the constructor. Ideally the only things that should be in the constructor are assignments.
  • DRY - Why are you repeating $this->start() all over the place?
  • What's up with that destroy() method? You're doing stuff there other than destroying the session! Why?
  • Your session_start() should happen once, in the constructor.
  • It would be more flexible if you passed the $_SESSION array as a parameter in the constructor, rather than relying on globals.
\$\endgroup\$
4
  • \$\begingroup\$ I have $this->start() as lazy initialization. I don't see the point of actually starting a session (session_start()) when there is no actual interaction with the session cookie that belongs to it. The destroy() method destroys the entire session, including the session cookie. Thing is, the $_SESSION array superglobal isn't available until after I've called session_start(), so it's not injectable. Otherwise I would have to call session_start() outside of the class, just to be able to inject it. \$\endgroup\$ – Kid Diamond Aug 29 '14 at 10:57
  • \$\begingroup\$ If you've initiated the Session object, you're interacting with the session by definition. It's the job of whoever called the Session object to determine when to start the session, not of the Session object itself. \$\endgroup\$ – Madara's Ghost Aug 29 '14 at 10:58
  • \$\begingroup\$ The cookie doesn't have to be destroyed. session_destroy is plenty. \$\endgroup\$ – Madara's Ghost Aug 29 '14 at 10:59
  • \$\begingroup\$ In that case, I'd call session_start() as part of every page load in the bootstrap stage. It costs practically nothing, and prevents repetition, even at the slight cost of leaking abstraction. PHP's sessions are kind of broken, so you can't really help it. Think about someone extending your class, he would have to dig into the code to realize why his session isn't working (because he has to call $this->start() every time!) \$\endgroup\$ – Madara's Ghost Aug 29 '14 at 11:01

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.