Witam, mam pewną klasę w niej obiekt pdo (uprzedzając podpowiedzi:)) i pewną funkcję, która odpowiada za sprawdzanie czy w bazie istnieje dany user czy nie jednak coś nie śmiga...

[PHP] pobierz, plaintext 
		public function login($login, $password) {
			$this->login = $login;
			$this->password = $password;
			$this->check_data = $this->pdo -> query("SELECT mail, haslo FROM users WHERE `mail`='$this->login' AND `haslo`='$this->password'");
			if (!empty($this->check_data)) {
				return "wszystko ok";
			}
		}
[PHP] pobierz, plaintext 

jak to "usprawnić" ?

Dzięki z góry!

Wybierasz dane które już posiadasz więc bez sensu. Zrób Select count(*) where... limit 1 przy założeniu że email posiada ograniczenie unique.
Czytałeś o prepared statements od ostatniego posta, prawda?

Przede wszystkim PDO jest po to, żeby uniknąć czystych zapytań MySQL, czyli powinno być coś w stylu:

$this->check_data = $this->pdo->select('users')
->fields(array('mail', 'haslo'))
->andWhere('mail = ?', $this->login)
->andWhere('haslo = ?', $this->password);

W ten sposób uzyskujesz minimum bezpieczeństwa. W swoim skrypcie masz bardzo poważny problem z SQJ injection.

Dodatkowo nie powinieneś mieć $this->login i $this->password tylko przekazać np obiekt usera i mieć $user->login i $user->password . Byłoby super gdyby było to pobierane przez metody np. $user->getLogin(), oraz $user->getPassword(). Przy czym password pamiętaj, że powinno być szyfrowane, czyli po zbindowaniu z formularza logowania ustawiasz najlepiej plainPasswod, które jest później konwertowane.

Za dużo w ogole operujesz na properiesach. Nie musisz ustawiac wszystkiego jako nowe pola w klasie. To bez sensu. Metoda ma zwócić tylko informację czy użytkownik może się zalogować czy nie. Przy czym return "wszystko ok"; raczej odpada. Daj return !empty($this->check_data) - zwróci true, albo false. Pomijasz wtedy w ogóle ifa.

Dodatkowo z tego co widzę to metoda nazywa się login.. przy czym nie loguje użytkownika tylko sprawdza czy istnieje w bazie danych. Powinna się nazywać zupełnie inaczej np. userExists($login, $password) i być wywoływaną przez login gdzie będzie ustawiana sesja.

Trochę chaotycznie, ale zawsze

Najlepiej wrzuć na githuba

Zapraszam też do głosowania na agendę http://www.phpcon.pl/2013/pl/agenda

Piotr Pasich

ok, wszystko jasne, dzięki za tak wyczerpującą odpowiedź, mam tylko pytanko, bo tego nie mogę znaleźć - jak przekazać obiekt $user ?


i mam błąd:

[XML] pobierz, plaintext 
 
Fatal error: Call to undefined method PDO::select() in rb.class.php on line 59
[XML] pobierz, plaintext 

[PHP] pobierz, plaintext 
		public function login($login, $password) {
			$this->login = $login;
			$this->password = $password;
			$this->check_data = $this->pdo->select('users')->fields(array('mail', 'haslo'))->andWhere('mail = ?', $this->login)->andWhere('haslo = ?', $this->password);
			return !empty($this->check_data);
		}
[PHP] pobierz, plaintext 

http://php.net/manual/en/book.pdo.php

Po prostu PDO nie zawiera takich metod o jakich wspomniał kolega powyżej...

Jeśli chodzi o zabezpieczenie skryptu, skorzystaj z dobrodziejstw jakie oferuje PDO - bindowanie (bindParam, bindValue) oraz tzw. "prepared statements" o których wspomniał już viking (KLIK).

Pamiętaj, manual jest Twoim przyjacielem Jeśli chcesz więcej wskazówek, zamieść zawartość całej swojej klasy, w której jest funkcja login.