DbM Framework - Autorska aplikacja frameworka opartego na wzorcu MVC Opcje

[Malinaa]

Witajcie,
miałem chwilę i usiadłem do autorskiego frameworka opartego na wzorcu MVC.
Chciałbym go dopracować, utworzyć wersje stabilną.

Kod frameworka jest dostępny pod adresem:
https://github.com/artimman/dbmframework

Zakończyłem pierwszy test aplikacji na serwerze zdalnym - powodzeniem.
Wydaje się, że już jest całkiem Ok i ciekawe, czy ktoś się ze mną zgodzi?

[nospor]

ok, to na plus:
+ znajomosc jako tako klas
+ uzywanie typowania w wiekszosci miejsc.
+ prawie uzywanie najnowszej wersji php (IMG:style_emoticons/default/wink.gif)


a teraz troche krytki:
w ogole na poczatek to zaprzyjaznij sie z takmi narzedziami jak :
- php-cs-fixer
- phpstan
- inne do statycznej analizy kodu
One wylapia ci na dzien dobry naprawde duzo bledow
I generalnie mowie tutaj o bledach ktore nie sa moze jakos krytyczne, ale w dluzszej perspektywie beda uciazliwe do poprawnej pracy z twoim kodem.


NIe wiem jak planujesz docelowo dostarczac swoj FW, ale na chwile obecna widze wrzuciles go fo VENDOR a ten katalog jest zastrzezony dla COMPOSER. wywal go wiec stamtad i wsadz nie wiem, np do LIBS.

No i jesli twoj kod uzywa np phpmailer, ktory jest zapisany w composer.json to nie zapisuj go w GIT. Katalog VENDOR generalnie w GIT ma sie nie znalezc w ogole. po to jest composer

plik config.php rowniez powinien nazywac sie np config.php.dist i dopiero ludzie kopiuja go sobie jako config.php lokalnie. Dlaczego? bo z kazdym updatem z twojego gita, ludzie straca swoje zmiany gdy to zostanie jak teraz

declare(strict_types=1); ma byc w kazdym pliku php a nie w co drugim

Potworki w stylu
IF
IF
IF
IF
IF
az oczy bola (IMG:style_emoticons/default/wink.gif) NIe bede omawial wszystkich bo w pyte tego masz, ale przyklad jak to sie poprawia
Jesli masz kod
IF (costam) {
//blabla
return 'cos tam'
}

return null;

To zeby uniknac zagniezdzenia duzego to sie robi poprostu negacje na pocatku, wali nullem na poczatku a reszta duzego kodu leci juz bez zagniezdzenia czyli:
IF (!costam) {
return null;
}
//blabla
return 'cos tam'

Jak widzisz w zagniezdzeniu jest tylko return null a nie milion linijek

public function __construct()
{
try {
$this->connect = new PDO("mysql:host=" . DB_HOST . ";dbname=" . DB_DATABASE, DB_USER, DB_PASSWORD, array(PDO::ATTR_ERRMODE => PDO::ERRMODE_WARNING));
$this->connect->exec("SET NAMES utf8");

return $this->connect;
} catch (PDOException $exception) {
throw new DbmException($exception->getMessage(), $exception->getCode());
}
}
No przeciez konstruktow nigdy nic nie zwraca wiec po grzyba tam return? konstruktor sam w sobie jest "returnem" (IMG:style_emoticons/default/wink.gif)

public function querySql(string $sql, ?string $fetch = null): PDOStatement
{
if ($fetch == 'assoc') {
$stmt = $this->connect->query($sql, PDO::FETCH_ASSOC);
} else {
$stmt = $this->connect->query($sql);
}

if (!$stmt) {
throw new DbmException($this->connect->errorInfo()[2], $this->connect->errorInfo()[1]);
} else {
return $stmt;
}
}

po co tu ten fetch jest raz ze stringiem a na dodatek nullem? skoro on odpowiada tylko za dwie mozliwosci, to zrob z niego boola i juz. Dodatkowo ten drugi ELSE na dole jest totalnei zbedny. :

public function querySql(string $sql, bool $fetch = false): PDOStatement
{
if ($fetch) {
$stmt = $this->connect->query($sql, PDO::FETCH_ASSOC);
} else {
$stmt = $this->connect->query($sql);
}

if (!$stmt) {
throw new DbmException($this->connect->errorInfo()[2], $this->connect->errorInfo()[1]);
}
return $stmt;

}
Analogicznie cala masa innych funkcji tam

public function requestData(string $fieldName)
{
if ($_SERVER['REQUEST_METHOD'] == "POST" || $_SERVER['REQUEST_METHOD'] == 'post') {
if (array_key_exists($fieldName, $_POST)) {
return trim($_POST[$fieldName]);
}
} elseif ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'get') {
if (array_key_exists($fieldName, $_GET)) {
return trim($_GET[$fieldName]);
}
}
}
To ze ktos wyslal dane POSTem, nie znaczy ze dane nie znajduja sie tez w GET. wlasnie ta funkcja zlikwidowales polowe funkcjonalnosci dla ludzia.

public function setDataToDB($value)
{
$value = strip_tags($value);
$value = htmlspecialchars($value);

return $value;
}
A co ty mi tutaj kasujesz tagi z pola co chce wlozyc do bazy? Jak bede chcial skasowac to sam sobie skasuje. poza tym uzywasz bindowania wiec po co htmlspecialchars? To sie uzywa podczas wyswietlania a nie przed wkladaniem do bazy. Ta cala funkcja jest totalnie zbedna

public function userPermissions(int $user): string
{
$database = new DbmDatabase();

$query = "SELECT roles FROM dbm_user WHERE id = ?";

if ($database->queryExecute($query, [$user])) {
if ($database->rowCount() > 0) {
$data = $database->fetchObject();

return $data->roles;
} else {
return 'dataNotFound';
}
} else {
return 'dataQueryError';
}
}
Jesli funkcja zwraca role, to ma zwracac role a nie komunikaty bledow. Od bledow masz wyjatki.I znowu wpyta zagniezdzen tutaj


public static function temp_htmlUser($sessionUserId, $module = null): void
{
$database = new DatabaseClass();
$userId = (int) $sessionUserId;

$query = "SELECT user.login, user.avatar, user_details.fullname FROM dbm_user user"
. " INNER JOIN dbm_user_details user_details ON user_details.user_id = user.id"
. " WHERE user.id = '$userId'";

if ($database->queryExecute($query)) {
$data = $database->fetchObject();
Czemu ta funkcja radosnie tworzy polaczenie do bazy i jeszcze sama z siebie pobiera usera? polaczenie z baza ma byc stworzone rac w calej aplikacji i przekazywane potem do odpowiednich klas. W tym momecnie wczasie jednego request ty generujesz mase polaczen do bazy.
Rowniez obiekt usera ma byc pobrany raz i przekazywane do odpowiednich klas.

Klasa TranslationClass. Raz ze te Class w nazwie jest zbedne, a dwa raz funkcje z malej raz z z duzej... generalnie maja byc z malej.

W klasie DatabaseClass, znowy Class zbedne ale:
public function __construct()
{
try {
$this->connect = new PDO("mysql:host=" . DB_HOST . ";dbname=" . DB_DATABASE, DB_USER, DB_PASSWORD, array(PDO::ATTR_ERRMODE => PDO::ERRMODE_WARNING));
$this->connect->exec("SET NAMES utf8");

Ty tutaj znowu tworzyc nowy obiekt PDO, czyli nowe polaczenia. Kazdy model z tego dziedziczy, wiec jak odpalisz 3 modele to masz juz 3 nowe polaczenia do bazy. No tak sie nie robil .Jak pisalem wczesniej, jedno polaczenia do bazy masz tworzyc i ono ma byc przekazywan tam gdzie trzeba

public function getSection(int $id): object
{
$query = "SELECT * FROM dbm_article_sections WHERE id = '$id'";

$this->queryExecute($query);

if ($this->rowCount() > 0) {
return $this->fetchObject();
}

return (object) [];
}
I znowy strasznie mieszasz style. Raz jak nie ma rekordu to zwracasz NULL a tu radosnie zwracasz pusty obiekt. No sie zdecyduj na jedno i sie tego trzymaj


public function userSigninCorrect(array $params, string $password): ?string
{
$query = "SELECT * FROM dbm_user WHERE (login=:login OR email=:email) AND verified=true LIMIT 1";

if ($this->queryExecute($query, $params)) {
if ($this->rowCount() > 0) {
$result = $this->fetchObject();

if (password_verify($password, $result->password)) {
return $result->id;
} else {
return self::VALID_PASSWORD;
}
} else {
return self::VALID_LOGIN;
}
} else {
return null;
}
}
I tutaj znowu, funkcja ktora powinna zwracac info czy user sie zalogowal czy nie to zwraca albo jako text id usera albo komunikaty bledow... No prosze cie (IMG:style_emoticons/default/wink.gif)

public const VALID_LOGIN = 'loginNotFound';
public const VALID_PASSWORD = 'passwordNotMatched';
Chyba sie to powinno nazywac INVALID a nie VALID


Jeszcze tego tam jest torche ale juz mi sie nie chce sprawdzac. Generalnie idziesz w dobrym kierunku powiedzmy, ale musisz troche popracowac jeszcze