Mój problem jest następujący: do klasy "user" przechowującej metody walidacji i sanityzacji planowałem dodać metody tworzące zapytania. Jednak jako że naruszałoby to SRP, postanowiłem stworzyć osobną klasę przechowującą owe metody i tutaj pojawia się pytanie: czy z obiektowego punktu widzenia poprawnie jest tworzyć klasę nie posiadającą struktur danych, a jedynie same metody? Może wystarczy po prostu plik przechowujący same funkcje niezapakowane w obiekt?
0
0
Chcesz mieć obiekt zwracający Ci samą treść zapytania? I co, będziesz za każdym razem osobno np. PDO wywoływał? ;-)
Połącz od razu PDO z tym obiektem i już funkcjonalność gotowa.
0
Planowałem zawrzeć w obiekcie np. 5 zapytań i jako zmienne podawać to, czego aktualnie szukam w bazie i łączyć zapytanie przez konkatenację :). Tylko tak: sama instancja obiektu nie posiadałaby sensu, bo co właściwie bym inicjalizował? Możesz rozwinąć pomysł połączenia tego z PDO? :)
0
Coś w ten deseń:
<?php
// w pliku AbstractRepository.php
abstract class AbstractRepository {
protected $db;
public function __construct() {
$this->db = cośtam;
}
}
// w pliku UserRepository.php
class UserRepository
extends AbstractRepository {
public function getById($userId) {
$stmt = $this->db->prepare('SELECT * FROM `users` WHERE `user_id` = ?');
$stmt->execute($userId);
return $stmt->fetch(PDO::FETCH_ASSOC);
}
}
Dzięki temu Twoje UserRepository zwraca od razu użytkownika, a nie samo zapytanie. Oczywiście lepiej by było, gdyby zwracało model, a nie suche dane, lecz na początek dobre i to.
Pamiętaj tylko, aby w AbstractRepository nie łączyć się z bazą, ani tym bardziej nie trzymać tam danych do łączenia się z bazą danych. Możesz wykorzystać np. jakiś singleton (bleeeh ;-P) w stylu Database::getPdoInstance().
0
czy z obiektowego punktu widzenia poprawnie jest tworzyć klasę nie posiadającą struktur danych, a jedynie same metody?
Myślę, że tak. Jeśli to bezstanowa klasa-serwis, to po co ma posiadać struktury danych?
BTW nie ma czegoś takiego jak poprawność z obiektowego punktu widzenia, bo nie ma ścisłej definicji obiektowego punktu widzenia (chociaż owszem są pewne popularne zasady jak SRP czy inne, ale i one są różnie interpretowane).
0
Patryk27 napisał(a):
Coś w ten deseń:
<?php // w pliku AbstractRepository.php abstract class AbstractRepository { protected $db; public function __construct() { $this->db = cośtam; } } // w pliku UserRepository.php class UserRepository extends AbstractRepository { public function getById($userId) { $stmt = $this->db->prepare('SELECT * FROM `users` WHERE `user_id` = ?'); $stmt->execute($userId); return $stmt->fetch(PDO::FETCH_ASSOC); } }Dzięki temu Twoje
UserRepositoryzwraca od razu użytkownika, a nie samo zapytanie. Oczywiście lepiej by było, gdyby zwracało model, a nie suche dane, lecz na początek dobre i to.Pamiętaj tylko, aby w
AbstractRepositorynie łączyć się z bazą, ani tym bardziej nie trzymać tam danych do łączenia się z bazą danych. Możesz wykorzystać np. jakiś singleton (bleeeh ;-P) w styluDatabase::getPdoInstance().
W porządku. Czyli "userRepository" rozszerza "abstractRepository" przechowujące w zmiennej "$db" jakąś referencję do danych potrzebnych do połączenia z bazą? Więc możemy wówczas określić, że nasza klasa posiada stan?
0
Tak, wtedy klasa ma już pewien stan.
0
Aktualnie pliki wyglądają tak:
Walidacja danych przy dodawaniu usera:
session_start();
require_once("../classes/user.php");
require_once("../classes/methodsRepository.php");
$methodsRepository = new methodsRepository();
if (isset($_POST['userLogin']))
{
$validateFlag[2] = [true, true]; // if finally flag is true, validation was succesful
$user = new User($_POST['userLogin'], $_POST['userPass'], $_POST['userConfirmedPass'], $_POST['userName'],
$_POST['userSurname'],$_POST['userBirthDate'],$_POST['listType']);
$validateFlag[0] = $user->validateAll();
$validateFlag[1] = $methodsRepository->validateUserByExisting($user->getLogin());
if ($validateFlag[0] && $validateFlag[1])
{
$user->setGroupType($methodsRepository->convertGroupNameToId($user->getGroupType()));
$methodsRepository->addUserToDatabase($user->getLogin(),$user->getPassword(),$user->getName(),$user->getSurname(),
$user->getGroupType(),$user->getBirthDate());
}
}
Konfiguracja bazy danych z wykorzystaniem Singletona:
class databaseConfig
{
private static $instance;
private $connection;
private $host = "localhost";
private $db_user = "root";
private $db_name = "groups_schema";
private $db_password = "";
/* Get an instance of the Database
* @return Instance
*/
public static function getInstance()
{
if(!self::$instance)
self::$instance = new self ();
return self::$instance;
}
public function getConncetion()
{
return $this->connection;
}
private function __construct()
{
$this->connection = new mysqli ($this->host, $this->db_user, $this->db_password, $this->db_name);
if (mysqli_connect_error())
{
trigger_error("Failed to connect database: ".mysqli_connect_error(), E_USER_ERROR);
}
}
// Magic method __clone is empty to prevent duplication of connection
private function __clone()
{
// TODO: Implement __clone() method.
}
}
I główny punkt, czyli klasa z metodami:
require_once ("abstractRepository.php");
class methodsRepository extends abstractRepository
{
public function validateUserByExisting ($login)
{
$validateFlag = true;
$mysqli = $this->db->getConncetion();
$result = $mysqli->query("SELECT login FROM users where login = '$login'");
if ($result->num_rows > 0)
{
$_SESSION['loginRepeatingError'] = "This login exists in the database already.";
$validateFlag = false;
}
return $validateFlag;
}
public function convertGroupNameToId ($groupName)
{
$mysqli = $this->db->getConncetion();
$result = $mysqli->query("SELECT groupId FROM groups WHERE name = '$groupName'");
$row = $result->fetch_assoc();
return $row['groupId'];
}
public function addUserToDatabase($login, $password, $name, $surname, $groupType, $birthDate)
{
$mysqli = $this->db->getConncetion();
if($mysqli->query("INSERT INTO users(login, password, name, surname, groupId, birthDate)
VALUES ('$login', '$password', '$name', '$surname', '$groupType', '$birthDate')"));
echo "New User has been added to database.";
}
public function getGroupNames()
{
$mysqli = $this->db->getConncetion();
$result = $mysqli->query("SELECT name from groups");
while ($row = $result->fetch_row())
echo '<option value='.$row[0].'>'.$row[0].'</option>';
}
}
Chyba jest o wiele poprawniej, niż w poprzednich wersjach. Wypadałoby jeszcze coś usprawnić? Mam zamiar pouczyć się teraz testowania kodu i MVC. Pora chyba zastosować już framework: zaplanowałem Symfony. Polecałbyś w jej miejsce inny?
0
Wciąż wiele do poprawy:
1.Nazwaj klasy w taki sposób: AbstractRepository, UserRepository, DatabaseConfig itd., tj. pierwszy wyraz także wielką literą - tak się przyjęło.
2.Skoro klasa nazywa się database configuration (databaseConfig), dlaczego zwraca ona instancję połączenia do bazy danych? Nijak ta funkcjonalność nie wynika z jej nazwy.
3.Repozytorium powinno zajmować się wyłącznie obsługą na linii serwis-baza danych, a nie generowaniem HTMLa (metoda getGroupNames), za co odpowiada widok.
4.validateUserByExisting co ta nazwa w ogóle znaczy? zwaliduj użytkownika po istniejącym?
5.addUserToDatabase - fragment ToDatabase jest zbędny: przecież wiadomo, że coś operujące na bazie danych operuje na bazie danych. W innym wypadku musiałbyś także pisać na przykład getGroupNamesFromDatabase, co niepotrzebnie marnowałoby Twój czas ;-)
6.Twój kod jest podatny na SQL injection.
Ale jest lepiej, to na pewno;
Z frameworków pisałem głownie w Zendzie, trochę w Yii oraz teraz tworzę projekt w Laravelu 5.4 - każdy ma jakieś wady, zalety, lecz najważniejsze to zacząć w ogóle, więc rzuć kostką i bierz się za pisanie ;-)
0
3.Repozytorium powinno zajmować się wyłącznie obsługą na linii serwis-baza danych, a nie generowaniem HTMLa (metoda
getGroupNames), za co odpowiada widok.
Właśnie z tą funkcją miałem największy problem. Wcześniej wyglądała tak:
public function getGroupNames()
{
$mysqli = $this->db->getConncetion();
$result = $mysqli->query("SELECT name from groups");
return $result->fetch_row;
}
I w widoku:
while ($row = methodsRepository->getGroupNames())
echo '<option value='.$row[0].'>'.$row[0].'</option>';
Niestety bezustannie była wczytywana tylko pierwsza nazwa grupy, która na dodatek w nieskończoność się wczytywała. Nie mam pojęcia, jak tutaj widok oddzielić od kontrolera (ale fakt, dostrzegam teraz logikę tego wszystkiego, bo nie mam pojęcia, jak testować tego typu funkcję).
4.
validateUserByExistingco ta nazwa w ogóle znaczy?zwaliduj użytkownika po istniejącym?
Dokładnie tak. Funkcja ma sprawdzać, czy użytkownik istnieje w bazie (o ile tutaj pasuje słowo "walidacja").
6.Twój kod jest podatny na SQL injection.
Zanim zostanie użyta jakakolwiek funkcja łącząca się z bazą, dokonywana jest walidacja i sanityzacja zmiennych. Więc czy jest potrzeba jeszcze zabezpieczać funkcje operujące na "oczyszczonych" danych?
Ale jest lepiej, to na pewno;
Z frameworków pisałem głownie w Zendzie, trochę w Yii oraz teraz tworzę projekt w Laravelu 5.4 - każdy ma jakieś wady, zalety, lecz najważniejsze to zacząć w ogóle, więc rzuć kostką i bierz się za pisanie ;-)
W porządku, więc pora zacząć działać :). Mógłbyś polecić jeszcze jakieś istotne elementy PHP warte uwagi? Póki co staram się dostosować projekty pod MVC, uczę się wzorców projektowych (jak Singleton, Observer) i działania testów.
0
Właśnie z tą funkcją miałem największy problem. Wcześniej wyglądała tak: (...)
fetch_row zwraca jeden wiersz, podczas gdy Ty potrzebujesz wszystkich - nic więc dziwnego, że nie działało to poprawnie.
Dokładnie tak. Funkcja ma sprawdzać, czy użytkownik istnieje w bazie (o ile tutaj pasuje słowo "walidacja").
walidować oznacza sprawdzać czy coś jest poprawne, zgodne ze wzorcem. Walidować możesz coś, co wprowadza użytkownik, pod kątem czy jest przykładowo liczbą. Sprawdzanie czy istnieje użytkownik o danym identyfikatorze to nie jest jego walidacja.
Dlatego właśnie Twoja metoda wyglądałaby lepiej nazwana doesUserExist.
Zanim zostanie użyta jakakolwiek funkcja łącząca się z bazą, dokonywana jest walidacja i sanityzacja zmiennych. Więc czy jest potrzeba jeszcze zabezpieczać funkcje operujące na "oczyszczonych" danych?
Tak:
1.Dla czytelności.
2.Niemalże na pewno czegoś albo teraz nie zabezpieczyłeś, albo nie zabezpieczysz w przyszłości - gdyby to było takie proste i oczywiste, nie byłoby żadnych dziur SQLi w aplikacjach, a te pojawiają się co chwilę.
Mógłbyś polecić jeszcze jakieś istotne elementy PHP warte uwagi? Póki co staram się dostosować projekty pod MVC, uczę się wzorców projektowych (jak Singleton, Observer) i działania testów.
Dokumentacja PHP, wybranego frameworka w połączeniu ze StackOverflow to wszystko, czego potrzebujesz ;-)
Btw, teraz zauważyłem, że Ty utworzyłeś sobie methodsRepository - to niszczy cały sens istnienia repozytorium. Repozytorium powinno dotyczyć jednego rodzaju obiektu (w uproszczeniu), tj. osobno UserRepository, OrderRepository, ProductRepository itd.
1
Z obiektowego punktu widzenia, żeby coś nazwać obiektem musi mieć stan i zachowanie. Jeśli którejś z tych składowych brakuje, to nie powinno się mówić o obiekcie. - somekind 2017-03-06 01:40
Na pewno? Nie widzę tego np w artykule: https://en.wikipedia.org/wiki/Object-oriented_programming Jest tam napisane:
Object-oriented programming (OOP) is a programming paradigm based on the concept of "objects", which may contain data, in the form of fields, often known as attributes; and code, in the form of procedures, often known as methods.
Wyraźnie jest napisane may, a nie must. Podaj mi jakąś definicję obiektu, która wymusza posiadanie pól.
Poza tym, czy niemutowalny singleton jest dalej obiektem czy już nie? Z punktu widzenia użytkownika takiego singletona nie ma znaczenia, czy stan jest zapisany w prywatnych polach klasy czy jako zmienne lokalne w metodach singletona.
0
Patryk27 napisał(a):
fetch_rowzwraca jeden wiersz, podczas gdy Ty potrzebujesz wszystkich - nic więc dziwnego, że nie działało to poprawnie.
W porządku, dałem 'fetch_all'. Funkcja wygląda tak:
public function getGroupNames()
{
$mysqli = $this->db->getConncetion();
$result = $mysqli->query("SELECT name from groups");
$result->fetch_all();
A widok:
<?php
foreach ($methodsRepository->getGroupNames() as $row)
echo '<option value='.$row[0].'>'.$row[0].'</option>';
?>
Teraz chyba jest okej? Wszystko działa.
walidowaćoznaczasprawdzać czy coś jest poprawne, zgodne ze wzorcem. Walidować możesz coś, co wprowadza użytkownik, pod kątem czy jest przykładowo liczbą. Sprawdzanie czy istnieje użytkownik o danym identyfikatorze to nie jest jego walidacja.
Dlatego właśnie Twoja metoda wyglądałaby lepiej nazwanadoesUserExist.
Poprawione
Zanim zostanie użyta jakakolwiek funkcja łącząca się z bazą, dokonywana jest walidacja i sanityzacja zmiennych. Więc czy jest potrzeba jeszcze zabezpieczać funkcje operujące na "oczyszczonych" danych?
Tak:
1.Dla czytelności.
2.Niemalże na pewno czegoś albo teraz nie zabezpieczyłeś, albo nie zabezpieczysz w przyszłości - gdyby to było takie proste i oczywiste, nie byłoby żadnych dziur SQLi w aplikacjach, a te pojawiają się co chwilę.
Czyli profilaktycznie zawsze skorzystać z 'htmlentities' i sprawdzenia pod kątem alfanumeryczności, jeśli byśmy wykorzystywali wykorzystywali za jakiś czas tej funkcji "nieoczyszczoną" daną?
Dokumentacja PHP, wybranego frameworka w połączeniu ze StackOverflow to wszystko, czego potrzebujesz ;-)
W takim razie zaczynam działać z Symfony:)
Btw, teraz zauważyłem, że Ty utworzyłeś sobie
methodsRepository- to niszczy cały sens istnienia repozytorium. Repozytorium powinno dotyczyć jednego rodzaju obiektu (w uproszczeniu), tj. osobnoUserRepository,OrderRepository,ProductRepositoryitd.
Czyli generalnie SRP: metody odpowiedzialne za dodawanie użytkownika w 'UserRepository', grupy 'GroupRepository', edycji pozycji 'EditRepository'?