[OOP] Erste PHP-Klasse bitte checken und habe Problem mit einer Funktion

Hallo,

ich habe folgende Login-Klasse erstellt, ist allerdings so ziemlich das erst OOP-Projekt. Deswegen möchte ich die Erfahrenen mal bitten, sich die Klasse anzusehen und Tipps zu geben.

Ausserdem hab ich noch ein Problem:
Die Funktion checkProfile() soll in der Funktion checkUser() nur dann ausgeführt werden, wenn bei der Funktion loginUser() der Parameter $profile gesetzt wurde. Allerdings weis ich nicht, wie ich herausfinden soll, dass dieser gesetzt wurde, ausser über ne Session-Variable oder ne include-Datei, was ich allerdings für nicht elegant gelöst halte. Zu prüfen ob der Paramter $autologin gesetzt wurde ist ja nicht schwer, da muss man ja nur prüfen, ob ein Cookie gesetzt ist.

Hier der Code:

PHP-Code:

<?php

    class user
    {
        protected $db;
        protected $u_id;
        protected $u_name;
        protected $u_pw;
        protected $profile;
        
        
        //SESS starten und DB_CONN angeben und angeben nach welcher Zeit (Min.) automatisch ausgeloggt werden soll
        public function __construct($db, $logout_time = 15, $profile = false)
        {
            session_start();
            $this->db = $db;
            $this->autoLogout($logout_time);
            $this->profile = $profile;
        }
        
        
        //NAME und PW mit DB vergleichen
        public function loginUser($u_name, $u_pw, $autologin = false)
        {
            $this->u_name = $u_name;
            $this->u_pw   = $u_pw;
            
            $sql    = 'SELECT
                           id
                       FROM
                           user
                       WHERE
                           name = "' .$this->db->real_escape_string($this->u_name). '"
                       AND
                           pw = SHA1("' .$this->db->real_escape_string($this->u_pw). '")';
            
            $result = $this->db->query($sql);
            $row    = $result->fetch_assoc();
            
            $this->u_id = $row['id'];
            
            if ($result->num_rows == 1) {
                $_SESSION['u_id']   = $this->u_id;
                $_SESSION['u_name'] = $this->u_name;
                
                if ($this->profile) {
                    $this->loginProfile();
                }
                
                if ($autologin) {
                    $this->setAutologin();
                }
                
                return true;
            } else {
                return false;
            }
        }
        
        
        //PROFIL aktualisieren (einloggen)
        protected function loginProfile()
        {
            $sql    = 'UPDATE
                           user
                       SET
                           sess_id = "' .session_id(). '",
                           last_action = NOW(),
                           last_login = NOW()
                       WHERE
                           id = "' .$this->u_id. '"';
            $result = $this->db->query($sql);
            
            return $result;
        }
        
        
        //AUTOLOGIN aktivieren
        protected function setAutologin()
        {
            $autologin_id = sha1(mt_rand().$_SERVER['REMOTE_ADDR'].microtime());
            
            setCookie('autologin', $autologin_id, time()+60*60*24*365);
                    
            $sql    = 'UPDATE
                           user
                       SET
                           autologin = "' .$autologin_id. '"
                       WHERE
                           id = "' .$this->u_id. '"'; 
            $result = $this->db->query($sql);
            
            return $result;
        }
        
        
        //SESS zerstören
        public function logoutUser()
        {           
            if ($this->profile) {
                $this->logoutProfile();
            }
            
            if (isset($_COOKIE['autologin'])) {
                $this->unsetAutologin();
            }
            
            if (session_unregister('u_id') && session_unregister('u_name')) {
                return true;
            } else {
                return false;
            }
        }
        
        
        //PROFIL aktualisieren (ausloggen)
        protected function logoutProfile()
        {           
            $sql = 'UPDATE
                        user
                    SET
                        sess_id = NULL,
                        autologin = NULL
                    WHERE
                        id = "'.$_SESSION['u_id'].'"';
            $result = $this->db->query($sql);
            
            return $result;
        }
        
        
        //AUTOLOGIN deaktivieren
        protected function unsetAutologin()
        {
            $sql = 'UPDATE
                        user
                    SET
                        autologin = NULL
                    WHERE
                        id = "'.$_SESSION['u_id'].'"';
            $result = $this->db->query($sql);
            
            return setCookie('autologin', '', time()-60*60);
        }
        
        
        //Prüfen ob USER eingeloggt ist
        public function checkUser()
        {
            if (isset($_SESSION['u_id'], $_SESSION['u_name'])) {
                if ($this->profile) {
                    $this->loginProfile();
                }
                
                return true;
            } else if (isset($_COOKIE['autologin'])) {
                return $this->checkCookie();
            } else {
                return false;
            }
        }
        
        
        //Prüfen ob COOKIE gesetzt ist
        protected function checkCookie()
        {
                $sql    = 'SELECT
                              id, name
                          FROM
                              user
                          WHERE
                              autologin = "' .$this->db->real_escape_string($_COOKIE['autologin']). '"';
                $result = $this->db->query($sql);
                $row    = $result->fetch_assoc();
                
                $this->u_id   = $row['id'];
                $this->u_name = $row['name'];
                
                if ($result->num_rows == 1) {
                    $_SESSION['u_id']   = $this->u_id;
                    $_SESSION['u_name'] = $this->u_name;
                    
                    return $this->checkUser();
                }
        }
        
        
        //PROFIL aktualisieren (eingeloggt)
        protected function checkProfile()
        {
            $sql    = 'SELECT
                           sess_id
                       FROM
                           user
                       WHERE
                           id = "' .$_SESSION['u_id']. '"';
            $result = $this->db->query($sql);
            $row    = $result->fetch_assoc();
                
            if (session_id() != $row['sess_id']) {
                $this->logoutUser();
                return false;
            } else {
                $sql    = 'UPDATE
                               user
                           SET
                               last_action = NOW() 
                           WHERE
                               id = "' .$_SESSION['u_id']. '"';  
                $result = $this->db->query($sql);
                
                return $result;
            }
        }
        
        
        //USER nach einer bestimmten Zeit von Inaktivität ausloggen
        private function autoLogout($logout_time)
        {
            $sql    = 'UPDATE
                           user
                       SET
                           sess_id = NULL
                       WHERE
                           last_action < DATE_SUB(NOW(), INTERVAL ' .$logout_time. ' MINUTE)
                       AND
                           autologin = ""';
            $result = $this->db->query($sql);
            
            return $result;
        }
    }
    
    $db = new mysqli('localhost', 'root', '', 'test');
    $user = new user($db, 15, 1);

?>

Hab bei jeder Funktion noch den Rückgabewert dabei, obwohl nur der von den Funktionen loginUser(), checkUser() und logoutUser() benötigt wird. Habe das noch drin, weil ich gerne prüfen wollte, ob die Funktionen auch richtig ausgeführt wurden. Da wäre es doch angebracht Exceptions zu benutzen, falls die Funktionen nicht richtig ausgeführt werden, oder?

Hoffe ihr könnt mir helfen.

MfG,
ill-maestro

Zum Check: Das is viel zu viel Code um da mal schnell durchzusteigen, was hast du denn für Angst was falsch sein sollte und weswegen testest du sie nicht einfach selbst aus?

Zu $profile: Wie wärs denn, wenn du das entsprechende Attribut in der loginUser() auch mal setzt, und es dann einfach in der entsprechenden Funktion wieder abfragst?

Setze doch einfach eine Property in loginUser() abhängig vom Parameter $profile. Diese Property kannst du in checkUser() den Wert dieser Property prüfen und ggf. checkCookie() aufrufen.

Noch ein paar allgemeine Worte: Deine Klasse macht jede Menge Dinge, die du besser in andere Klassen auslagern solltest. Man erkennt es schon an den Methodennamen. So hast du beispielsweise viele Methoden ***User() genannt, andere nicht. Das deutet schon darauf hin, dass die anderen Methoden nichts mit dem User zu tun haben. Session, Cookies, DB, das sind alles Dinge, von denen ein User-Objekt überhaupt nichts wissen sollte.

@ArSeN: Naja, ich möchte halt eine gut wiederverwertbare Klasse haben. Ich weis halt auch nicht, ob es besser ist die protected Funktionen so wie hier nur innerhalb der public Funktionen aufzurufen, oder diese immer einzeln aufzurufen, also die protected Funktionen zu public zu machen.

Das mit dem Attribut setzen geht ja nicht, denn die loginUser() wird in einer anderen Datei als die checkUser(). Deswegen meinte ich ja auch, dass das wohl nur mit ner Session-Variable, bzw. mit einer Variable die man in einer include-Datei setzt geht.

@onemorenerd: Was ist eine Property?
Was sollte ich denn deiner Meinung nach auslagern?

Hab hier mal einen Auszug wie ich die Funktionen benutze:

login.php

PHP-Code:

    if (isset($_POST['login']) && $user->loginUser($_POST['u_name'], $_POST['u_pw'])) {
        header('Location: index.php');
    } else {
        echo 'Login-Formular';
    } 


index.php

PHP-Code:

    if ($user->checkUser()) {
        echo 'Check-Content<br>';
        echo '<a href="logout.php">Logout</a>';
    } else {
        header('Location: login.php');
    } 


logout.php

PHP-Code:

    if ($user->checkUser() && $user->logoutUser()) {
        echo "Tschüss!";
    } else {
        header('Location: login.php');
    } 


Original geschrieben von ill-maestro
Das mit dem Attribut setzen geht ja nicht, denn die loginUser() wird in einer anderen Datei als die checkUser().

Kann leider nicht Hellsehen

Eine Property ist eine Eigenschaft, also deine Attribute $db, $u_id, etc. pp..

Wenn du das gleiche Objekt auf mehreren Seiten verwenden möchtest, wirst du wohl nicht umhinkommen, es irgendwo zwischenzuspeichern (z.B. Session), oder aber dich gegebenenfalls mal über SOAP schlau machen.

Eine Menge Quelltext ... aber

PHP-Code:

session_start(); 


in Deine Klasse (Konstruktor) zu integrieren wird Probleme bereiten, da dies immer am Anfang einer Seite stehen sollte! Ansonsten gildet:
1. Keine vorherige Ausgabe ... egal in welcher Form
2. Kein Zugriff auf Session-Daten vor session_start

Gruß php_fussel

Hmm, gibts keine andere Möglichkeit als das Objekt zwischenzuspeichern? Ich habe dabei irgendwie das Gefühl, dass meine Klasse nicht gut konzipiert ist...
Und SOAP ist soweit ich weis doch nicht auf jedem Webserver vorhanden... Ich wollte eigentlich nur mit den PHP-eigenen Möglichkeiten arbeiten.

Das mit dem session_start(); weis ich, die Klasse wird eh immer vor einer Ausgabe per Autoload in ner include-Datei geladen.

Was könnte ich denn anders machen?

Hmm, gibts keine andere Möglichkeit als das Objekt zwischenzuspeichern?

Nö.. woher soll denn die nächste Seite wissen, wleche Informationen sie nun braucht? HTTP ist nunmal ein zustandsloses Protokoll, und daran lässt sich nichts ändern.

Also die Frage war eher so gemeint, was ich denn an der Klasse ändern könnte, um dieses Problem zu umgehen.
Eine Möglichkeit wäre ja noch, die checkUser() ebenfalls wie die loginUser() mit Parameter aufzurufen. Aber ist auch nicht gerade elegant. Oder ich baue die Aufgaben der Funktionen loginProfile(), checkProfile() und logoutProfile() standardmäßig in die Funktionen in denen sie aufgerufen werden. Dann kann man halt keinen Login mehr ohne Profil coden...

Da kannst du soviel rumändern wie du willst. Wenn du Informationen auf einer Seite generierst, die du auf einer anderen Seite weiterverwenden möchtest, musst du Sie irgendwo zwischenspeichern damit sie auf der nächsten Seite wieder verfügbar sind.

Was ist also so verkehrt daran den Kram bei Bedarf zwischenzuspeichern? Verstehe das Problem ehrlichgesagt nicht so ganz.

Original geschrieben von ArSeN
Was ist also so verkehrt daran den Kram bei Bedarf zwischenzuspeichern? Verstehe das Problem ehrlichgesagt nicht so ganz.

Ich auch nicht.

Sowas wie den Login ueberpruefe ich doch nur ein mal - und dann wird die Information "Benutzer hat sich ordnungsgemaesz authentifiziert" irgendwo abgelegt, wo sie auch fuer spaeter aufgerufene Scriptinstanzen abfragbar ist - in einer Session beispielsweise.
Und wenn ich aus dieser Session die Info auslesen kann - der Schritt kann natuerlich auch noch mal gekapselt sein, in einer Methode checkLoginStatus o.ae. - dann interessiert mich doch im weiteren Verlauf die Methode, die die Logindaten effektiv checkt, gegen die Datenbank abprueft o.ae., gar nicht mehr wirklich.

Das mit dem session_start(); weis ich, die Klasse wird eh immer vor einer Ausgabe per Autoload in ner include-Datei geladen.

Auch wenn Du die externe Klasse als erstes ganz oben in die Datei lädst/ includierst, wird bei Deiner Vorgehensweise session_start() erst dann integriert, wenn von der Klasse ein Objekt erzeugt wird, denn erst dann wird der Konstruktor aufgerufen ...

Nach dem Location Header sollte ein exit() oder die() kommen.

Original geschrieben von deathcakeman
Nach dem Location Header sollte ein exit() oder die() kommen.

OK, aber wozu braucht man das?

Original geschrieben von ill-maestro
OK, aber wozu braucht man das?

Um zu sicherzustellen, dass evtl. noch folgender Code nicht ausgeführt wird.