Chciałbym prosić o ocenę kodu programowego (PHP + MySQL) mojej bardzo prostej aplikacji internetowej (sieć reklamowa). Całość napisana w dwóch wersjach (Symfony i własny szkielet aplikacji w PHP).

Aplikacja w Symfony + biblioteka do obsługi Api:

https://github.com/webeeq/symfony.eeq
https://github.com/webeeq/sieciq

Własny szkielet tej samej aplikacji w PHP:

https://github.com/webeeq/framework.eeq

Obydwa projekty poddawałem już ocenie jakieś dwa lata temu, ale nie widzę nigdzie tamtego wątku, więc tworzę nowy. Pewnie został skasowany. Obecne wersje zawierają liczne poprawki.

Prosiłbym zwrócić szczególną uwagę na zagadnienia:

1. Tworzenie klas encji tabel bazy danych w Symfony. Czy dobrze to robię? Chodzi mi głównie o brak deklaracji opcji "default".
2. Jakie wzorce projektowe poza Dependency Injection powinienem użyć w moich projektach. Jakie wzorce warto jeszcze znać?
3. Czy zapytania SQL (w cudzysłowie '...') mogą być w wielu liniach. Nie powoduje to błędu, ale czy to ładnie tak programować?
4. Czy z takim programowaniem, jak moje, jest sens w ogóle starć się o pracę? Bądź lepiej będzie, jeśli dam sobie z tym spokój?

$code = ($_GET['code']) ? (int) $_GET['code'] : 404;
po co ten pierwszy nawias?

$code = $_GET['code'] ? (int) $_GET['code'] : 404;

A jakbys przesiadl sie na php7 to juz w ogole bajka:
$code = (int) ($_GET['code'] 404);


nie $code == 403
a $code === 403
naucz sie uzywac ===

Plik index.php i ten zajebiscie duzy switch...case no wola o pomste do nieba Nie tworz takich kobyl, robijaj rzeczy na serwisy, klasy



user_admin` TINYINT(1) NOT NULL DEFAULT '0',
Jak cos jest liczba, to nie ustalaj temu czemus domyslnej wartosci jako tekst. Liczby to liczby, tekst to tekst.
Wywal te (1) z tinyint i inne podobne przy typach liczbowych bo to nie sluzy ograniczeniu zakresu liczby...

ten kod
if (!$editUserModel->isUserId($id, $user)) {
$editUserModel->dbClose();
header('Location: ' . $config->getUrl() . '/logowanie');
exit;
}
powielasz w wielu kontrolerach

po co takie
// src/Controller/ShowSiteController.php
wstawki w kontrolerach? Czemu to sluzy procz faktu ze niczemu?

W kazdym kontrolerze masz
->dbConnect()
->dbClose();
Cala masa zbednego i zduplikowanego kodu

I wiele wiele wiecej, nie mam czasu przegladac

Ludzie sie staraja o prace z gorszymi kodami i mniejsza wiedza

public function getSiteData(
int $site,
?int &$site_visible,
?string &$site_name,
?string &$site_url
Unikaj taki potworkow z referencjami. Jak funkcja ma zwracac dane, to zwracaj je jako tablica a nie przez referencje


return ($this->dbFetchArray($result)) ? true : false;
wystarczy
return (bool) $this->dbFetchArray($result);

CityListModel ktory jest katalagu model/Ajax
Nie tworz modeli pod akcje. Model to model, akcja to akcja. Model moze byc odpalony w kazdej akcji

nie "<?php echo $array['www']; ?>
a: "<?= $array['www'] ?>
Mamy juz XXi wiek

Mnie się jeszcze w oczy rzuciła sama struktura. Wszystko wrzucone do folderu / wliczając cssy i src. Ani to bezpieczne, ani uporządkowane. W src jest Config/cośtam. Dlaczego to zapisałeś w ten sposób zamiast stworzyć oddzielny folder poza strukturą src? Osobiście nie podoba mi się definiowane w każdej akcji layout itd (które oczywiście odnosi się do src).

PS. $code = ($_GET['code']) ? (int) $_GET['code'] : 404; Ja też tak piszę. Ma to zapobiec przypadkowemu złemu warunkowaniu w operatorze ternary.

Klasa Param to cala prawie do wyrzucenia. Jaki jest jej sens w ogole?

Czemu konfig bazy danych znajduje sie w repo? A co jak inna osoba ma inne dane do bazy niz ty?

Niby uzywasz jquery a potem i tak do ajaxa masz
function ajaxData(id, file) {
if (window.XMLHttpRequest) {
xmlhttp = new XMLHttpRequest();
} else {
xmlhttp = new ActiveXObject("Microsoft.XMLHTTP");
}
xmlhttp.onreadystatechange = function() {
if (xmlhttp.readyState == 4 && xmlhttp.status == 200) {
document.getElementById(id).innerHTML = xmlhttp.responseText;
}
}
xmlhttp.open("GET", file, true);
xmlhttp.send();
}
to po co te jquery?

Jakis przyklad takowego przypadkowego zlego warunku?

zas co do TINYINT(1) to najlepsze jest to ze miesiac temu o to pytales. Wyciagasz w ogole jakas nauke z naszych odpowiedzi czy walimy w powietrze wszystko?

Nawet z dokumentacji echo (true?'true':false?'t':'f'); Ogólnie wyrobiłem sobie nawyk nawiasowania zawsze chociaż to nadmiarowe. Zresztą w dokumentacji też używają nawiasu do przykładu z empty.

Mimo wszystko podany przez ciebie przyklad z dokumentacji to jednak troche inna sytuacja niz omawiany przypadek z kodu autora. O ile to podane przez ciebei tu echo jestem jeszcze w stanie strawic, to tamten zbedny nawias pierwszego kodu, by nie przeszedl code review.

CR są per firma. U mnie by nie przeszło bez nawiasu + cs fixer dodający spacje.

Przynajmniej w jednym sie zgadzamy

Myślałem, że dla typów logicznych ten (1) ma znaczenie. W całej reszcie zrobiłem bez liczb w nawiasach. W Symfony 4.4 używają TINYINT(1) dla logicznych:

https://github.com/webeeq/symfony.eeq/blob/...00326095423.php


Czy używanie dokładnego przyrównania jest bezwzględnie wymagane, jeśli nie przyrównuję do zera lub wartości logicznej? W dokumentacji chyba robią zamiennie.


Ten plik index.php odpala kontrolery w oparciu o parametr wywołanej strony. Kontrolery natomiast odpalają serwisy aplikacji. Jak to mogę zrobić lepiej i prościej?


Takie komentarze dodawali w dokumentacji Symfony, więc też tak robiłem. W sumie nie sprawdzałem, czy robią to nadal. I faktycznie nie ma to większego sensu.


Wzorowałem się na Symfony z tymi kontrolerami. Jedna podstrona = jeden kontroler. Jak to mogę zrobić lepiej, aby nie duplikować tego kodu w kontrolerach?


Co zrobić, gdy metoda zwraca tablicę z wynikiem zapytania do bazy i dodatkowo chcę zwrócić (zmienić) jakąś wartość dla parametru, który i tak muszę podać?


Czyli jak w Symfony? Oddzielny model dla zapytań do tabeli `users` i oddzielny do `sites`? Tylko jak realizować połączenia z bazą danych? W konstruktorach?

Kurczę. Napisałem się jak głupi, a tu błąd, że zbyt dużo cytatów. I nie mogę reszty dodać...

Przestan tak bezgranicznie patrzec na dokumentacje a juz szczegolnie przestan patrzec na elementy z tla w dokumentacji gdy czytasz o rozwiazaniu X. Dokumentacja moze byc troche nie swieza, moze miec blad itp.

tinyint to nie jest typ logiczny. Nie sluzy tylko do przechowywania 0 i 1.
(LICZBA) - obowiazuje kazdego typu INT. Miales to wyjasnione miesiac w temu w oddzielnym temacie. Wiec tak, przestan stosowac te liczby w nawiasie bo one nic nie robia. Robia dopiero jak uzyjessz ZEROFILL - tyle w temacie

=== masz uzywac zawsze. Nie wazne ze gdzies tam przy opisie funkcji X uzyli ==. Oni opisywali funkcje X a nie dzialanie ==/===
=== zaoszczedzi ci mase problemow w przyszlosci

index.php i duzy switch - ok, ale index.php nie musi wiedziec, ze kontroler x potrzewbuje parametroe x,y,z by dzialac. To kontroler/service ma to wiedziec i o oto dbac a nie index.php

W dokumnetacji symfony dodaja nazwe pliku jako komentarz bys wiedzial gdzie ten kod wstawic. W swoim edytorze czy na git juz wiesz w jakim pliku jestes. Informacje ze plik x.php to plik x.php jest zbedna

Zasad jednej odpowiedzialnosci. Jak funcja zwraca dane z bazy to ma zwracac dane z bazy i tyle.

Tak. I model nie powinieniec dziedziczyc po database. On powinien to miec wstrzykniete i jego nie powinno obchodzic startowanie i zamykanie polaczenia

W sumie faktycznie mógłbym umieścić kod css i js w oddzielnym katalogu np. public. Tylko że ostatnia linijka kodu w pliku .htaccess zabezpiecza przed dziurami.

https://github.com/webeeq/framework.eeq/blo...aster/.htaccess


Wzorowałem się na framework'u Symfony. Tam jest, dla każdej akcji, renderowany szablon w oparciu o plik twig. Jak to mogę uprościć lub zrobić jeszcze lepiej?

https://github.com/webeeq/symfony.eeq/blob/...mController.php
https://github.com/webeeq/symfony.eeq/blob/...FormService.php


Zabezpiecza przed atakami. Uniemożliwia wklejenie do zapytania SQL fragmentu niechcianego kodu. Bez metod klasy Param zmienne byłyby niezabezpieczone.


W Symfony jest dobrze. Tu nie chciało mi się usuwać. Miał to być przykładowy skrypt. Ustawiłem default'owo user i hasło root. Poprawię przy najbliższej okazji.


Tego nawiasu używam, bo coś takiego widziałem w dokumentacji. Ten skrypt powstawał, gdy nie było jeszcze php7. Nie chciało mi się wszystkiego poprawiać.

Do zabezpieczania zapytan uzywa sie BINDowania a nie jakis pseudu zabezpieczen sprzed 30 lat


W dokumetnacji tez widziales == zamiast ===
Zreszta, jak widziales po naszej rozmowie z vikingiem, nawet takie zbedne nawiasy maja swoich fanow.

Kiedys rozwiniesz sie jeszcze bardziej, przesiadziesz sie np. na NGINX i tyle z twojego zabezpieczenia. ROb porzadnie od razu. PUBLIc to public, PRIVATE to private

Jedna strona jeden kontroler. W różnych fw różnie się to zwie (handlers, actions, invokable controller) ale w takim razie zastosuj __invoke. Myślałeś o psr15?

Nasuwa mi się pytanie... Gdzie i jak powinienem startować i zamykać połączenie z bazą danych?


Będę szczery... Poczytałem i wiem, jak ten __invoke działa, ale nie mam pojęcia, jak go w moim projekcie użyć. Jeśli chodzi o PSR15, to muszę się wgryźć, bo na szybkiego średnio zrozumiałem. Mógłbym prosić o jakąś podpowiedź, jak tego mam użyć?

Na przykładzie tego https://github.com/webeeq/framework.eeq/blo...rController.php

[PHP] pobierz, plaintext 
class ActivateUserHandler implements RequestHandlerInterface
{
    /** @var TemplateRendererInterface */
    private $renderer;
 
    public function __construct(TemplateRendererInterface $renderer)
    {
        $this->renderer = $renderer;
    }
 
    public function handle(ServerRequestInterface $request) : ResponseInterface
    {
        $activateUserModel = new ActivateUserModel();
 
        $activateUserModel->dbConnect(); //głupota, serwis powinien mieć informację o interfejsie storage, połączenie z bazą i tak jest zamykane na koniec requestu
 
        $activateUserService = new ActivateUserService($activateUserModel);
        $array = $activateUserService->variableAction(
            $user,
            $code
        );
 
        return new HtmlResponse($this->renderer->render(
            'app::login',
            $array
        ));
    }
}
 
[PHP] pobierz, plaintext 

Wszystko zalezy jak rozwiazesz tworzenie modeli, serwisow itp, ale generalna idea:

gdzies tam, niech dla przykladu bedzie to index.php, tworzysz obiekt bazy danych:

[PHP] pobierz, plaintext 
$db = new Database();
$db->connect(); //choc to, powinno byc zrobiony jako lazy loading jak ci mowiono w innym temacie by bez potrzeby nie laczyc sie z baza.
 
//...
// i na koncu rozlaczenie
$db->disconnect();
[PHP] pobierz, plaintext 

Taki obiekt bazy danych jest potem wstrzykiwany wszedzie tam gdzie jest potrzebny

Dzięki serdeczne za trafne uwagi. Wyciągnę z nich wnioski na przyszłość.