Prośba o ocenę kodu PHP

pytanie zadane 28 stycznia 2018 w PHP przez User007 Bywalec (2,400 p.)

Witam.

Skrypt ma obsługiwać zapytania typu REST. Obecnie napisana jest obsługa zapytań typu GET.

Proszę o ocenę kodu pod każdym kątem. Szczególnie czy kod jest zgodny ze wzorcem MVC.

Uprzedzając możliwe pytania odpowiem na nie teraz :).

1. Tak wiem mogłem użyć polecanego przez wszystkich frameworka SLIM. Ale piszę ten kod w celu nauki i poznania od środka jak to się wszystko działa. Na framework przyjdzie czas później.

2. Wiem że plik config.ini powinien znaleźć się poza głównym katalogiem na serwerze i finalnie tam będzie.

Link do projektu na gitlab.

https://gitlab.com/DOOMinik/test

Z góry dziękuję za konstruktywną krytykę.

Pozdrawiam

1 odpowiedź

odpowiedź 28 stycznia 2018 przez Arkadiusz Waluk Ekspert (287,950 p.)

W pliku index.php są jakieś komentarze, raczej unikałbym zostawiania takich wykomentowanych rzeczy w kodzie - nie wiadomo czy to działa czy nie, może to potrzebne a może nie itd.
Konfiguracja nie powinna być możliwa do zacommitowania w repozytorium. Nawet jak plik config.ini gdzieś tam wyciągniesz, to i tak powinien on być tylko plikiem bazowy dla konfiguracji, a plik właściwy powinien być w gitignore, aby każdy mógł bez problemu mieć swoje dane czy przez przypadek ktoś nie zacommitował swoich.
No i wszystkie pliki (wraz z tym configiem) będą dostępne publicznie - root directory serwera będzie trzeba ustawić tak aby index.php był publiczny, będzie więc też reszta plików. To raczej niepotrzebne i umieściłbym je katalog wyżej (aby z serwera nie były widoczne). Jak dla plików PHP to nie powinien być problem bo nic specjalnie szkodliwego nie powinno się tam zadziać, tak nie wiem jak zachowa się serwer dla pliku .ini - czy przypadkiem po prostu go nie wyświetli ujawniając Twoje dane do bazy. Także dla bezpieczeństwa wszystko co niepotrzebne poza publicznym katalogiem serwera.
Mamy już od dłuższego czasu PHP 7 i sądzę, że pisanie pod nie powinno być standardem. Warto więc dodać typy argumentów do metod, czy typy zwracane.
Nieprzestrzegany standard wyglądu kodu PSR-2 - chociaż nie jest to standard oficjalny i nie musisz go przestrzegać, to jest w PHP tak popularny, że moim zdaniem warto pisać zgodnie z nim.
Masz kontrolery DeleteController, GetController, PostController... - dla każdej metody HTTP robisz nowy kontroler? Moim zdaniem to bez sensu, robiłbym jeden kontroler dla jednej grupy. Czyli np. mam UserController i tam wszystkie akcje jakie dotyczą użytkownika.
W widokach masz coś takiego jak GetRecords i tam pobierasz dane z bazy. Jak dla mnie widok nie ma tego robić, widok powinien tylko wyświetlać dane które dostanie, a nie ma prawa wiedzieć skąd się wzięły. Takie operacje jak pobranie danych przeniósłbym do modelu.
```
$getData = $this->db->query("SELECT * FROM $table WHERE users.id = $recordId");
```
Dobrze widzę, że tu $recordId wchodzi prosto z adresu strony? Jeśli tak to można wykonać atak sql injection, polecam użyć bindowania.

Na plus korzystanie z Gita oraz autoloadingu z Composera. Po komentarzu widzę PhpStorma, również plus.

komentarz 29 stycznia 2018 przez User007 Bywalec (2,400 p.)

Dziękuję bardzo za recenzję. Zastosuje się do Twoich zaleceń. Zastanowił mnie tylko ten fragment:

Dobrze widzę, że tu $recordId wchodzi prosto z adresu strony? Jeśli tak to można wykonać atak sql injection, polecam użyć bindowania.

$recordId nie wchodzi prosto z adresu. Najpierw cały URI jest validowany przez regex, więc wydawało mi się że jeśli zapytanie nie będzie zgodne ze wzorcem to validacja nie przepuści dalej zapytania. Dlatego finalnie zrezygnowałem z bindowania.

Czy się myliłem?

Mam jeszcze pytanie odnośnie composera.

Dlaczego u mnie na kompie dla autoloadingu starczała taka instrukcja:

{
  "autoload" : {
    "psr-4"  :{
      "Api\\":"api/"
    }
  }
}

A po przeniesieniu na serwer powyższe nie działało i musiałem dopisać:

    "classmap": [
      "api/"
    ]

Dziękuję

komentarz 29 stycznia 2018 przez Arkadiusz Waluk Ekspert (287,950 p.)

Ach tak, jeśli była tam walidacja np. tylko na cyfry to teoretycznie jest w porządku. Jednak dla bezpieczeństwa radziłbym wszędzie, gdzie podstawiasz jakieś dane wchodzące od użytkownika, używać bindowania.

A co do autoloadingu to dziwne, powinno działać po samej deklaracja z PSR-4. Co dokładnie się działo? Zrobiłeś na pewno na serwerze composer install, aby wszystko się ładnie przygotowało?

komentarz 29 stycznia 2018 przez User007 Bywalec (2,400 p.)

A to muszę coś jeszcze na serwerze robić? Jeśli tak to nie, nic takiego nie robiłem. To moje pierwsze spotkanie z composerem i nic jeszcze nie wiem (jak widać).

Jak mam composera na serwerze uruchomić?

komentarz 29 stycznia 2018 przez Arkadiusz Waluk Ekspert (287,950 p.)

Jeśli na tym serwerze sklonowałeś repozytorium to trzeba po prostu zainstalować ewentualne biblioteki i przygotować autoloading, wystarczy do tego composer install.

A jeśli po prostu przesłałeś wszystkie pliki, włącznie z vendorem, to teoretycznie powinno działać.

Co rozumiesz przez to, że nie działało? Jakie były błędy?

komentarz 29 stycznia 2018 przez User007 Bywalec (2,400 p.)

Tak przesłałem wszystko z vendorem. A błąd była taki że nie widziało plików z klasami. Jak dopisałem w/w część to problem znikł.

Dobra, nie ważne. Teraz na próbę znowu przesłałem pliki na serwer i działa pomimo braku classmap. Więc nie wiem dlaczego wcześnie nie działało.

komentarz 29 stycznia 2018 przez Arkadiusz Waluk Ekspert (287,950 p.)

Dziwne, nigdy nie używałem tego classmap i bez problemu działało. Gdy sytuacja się powtórzy spróbowałbym jednak usunąć vendor i zainstalować to bezpośrednio na serwerze. Albo spróbować composer dump-autoload. Coś, co odświeży mu to wszystko.

O. ok, fajnie że działa :)

komentarz 5 lutego 2018 przez User007 Bywalec (2,400 p.)

@Arkadiusz Waluk,

Dokonałem zmian w kodzie. Czy teraz wygląda to lepiej?

komentarz 5 lutego 2018 przez Arkadiusz Waluk Ekspert (287,950 p.)

Tak, wygląda znacznie lepiej. Dobrze, że wydzielony jest kod z katalogu publicznego serwera. Fajnie, że dodałeś bindowanie przy bazie danych. Pojawiły się typy zwracane i typy argumentów, super.

Chcesz żebym coś dalej sugerował? Podrzucić trochę mogę:

Jako główny katalog nazw dałbym src/. Nie jest to żaden wymóg i żaden standard tego chyba nie określa, po prostu taki nawyk i takie też najczęściej spotykam - to jest dla każdego od razu oczywiste, /api/core mniej.
Nazywałbym podprzestrzenie nazw rozpoczynając od wielkich liter. Czyli przykładowe ścieżki:
src/Controller/UserController - to przestrzeń nazw Api\Controller\UserController. Nie pamiętam czy PSR-2 to sztywno określa, ale tak pokazuje przykład: https://www.php-fig.org/psr/psr-2/#11-example
Zgodnie z PSR-4 klasa powinna nazywać się tak samo jak plik, w którym się znajduje i tutaj: https://gitlab.com/DOOMinik/test/blob/master/api/core/controllers/Controler.php (chyba przez literówkę) tak nie jest - plik Controler.php klasa Controller.
PSR-2 nie jest nadal w niektórych miejscach przestrzegany (choć jak mówiłem nie jest to obowiązek, to nieoficjalny standard).
array() można wyrzucić i użyć [], zapis krótszy i moim zdaniem ładniejszy. Dodany w PHP chyba 5.3, można więc śmiało używać.
Średnio jestem za używaniem die() - nawet jeśli aplikacja nic dalej nie robi, pozwoliłbym jej się po prostu samej zakończyć, a nie nagle wszystko przerywać.
```
catch (PDOException $e) {
            $e->getMessage();
        }
```
Jest kilka kawałków w takim stylu i nie za bardzo wiem co to ma zrobić. Ten komunikat nawet nie zostanie wyświetlony, nie ma echo ani nic. Zbędny kod, nic nie robi. W tej samej kwestii: zaraz potem jest normalne zwracanie pustej tablicy, nie wiem czy to pożądane działanie jeśli wystąpi jakiś bliżej nieznany błąd bazy.
```
$strArr = explode('/',$path);
```
Parsowanie adresu pojawia się często, przygotowałbym więc pod to jakąś klasę/metodę/cokolwiek, co by to ułatwiało.
```
use Api\Core\Models\{Level,LevelQueries};
```
Taki zapis (wiele klas w jednym use) jest dozwolony od PHP 7, ale zabrania go standard PSR-2 mówiąc: "There MUST be one use keyword per declaration."
https://gitlab.com/DOOMinik/test/blob/master/api/core/views/LevelView.php#L13 - potencjalny wyjątek bazy danych jest łapany już wyżej, w samym modelu, więc tak teoretycznie to try catch nie ma tu co złapać.
https://gitlab.com/DOOMinik/test/blob/master/api/core/Router.php#L22 taki zapis routingu może i na razie ładnie wygląda, ale docelowo będzie bardzo długi i kiepsko skalowalny. Pomyślałbym jak to zrobić inaczej.

Chyba tyle na razie, jakby się jeszcze postarać to można więcej sugerować. To nie są żadne poważne rażące błędy, bardziej rzeczy które bym zasugerował z doświadczenia i dobrych praktyk. Ogólnie całość wygląda nieźle, kierunek nauki i rozwoju moim zdaniem jest dobry.

komentarz 6 lutego 2018 przez User007 Bywalec (2,400 p.)

Dzięki wielkie za tak obszerną recenzję. Postawił bym Ci duże PIWO gdyby nie było tylko wirtualne :). A tak to muszę się niestety ograniczyć do dużego Dziękuję.

A teraz do tematu.

"Nazywałbym podprzestrzenie nazw rozpoczynając od wielkich liter. Czyli przykładowe ścieżki...". Nie bardzo mogę wywnioskować gdzie błąd zrobiłem. Przejrzałem raz jeszcze dokumentację PSR i postąpiłem zgodnie z wytyczną. Czy może Ty gdzieś dopatrzyłeś się u mnie małych liter.
" PSR-2 nie jest nadal w niektórych miejscach przestrzegany (choć jak mówiłem nie jest to obowiązek, to nieoficjalny standard)". Staram się pisać zgodnie ze standardem widocznie coś mi umknęło. Przejrzę jeszcze raz od A do Z.
"Średnio jestem za używaniem die() - nawet jeśli aplikacja nic dalej nie robi, pozwoliłbym jej się po prostu samej zakończyć, a nie nagle wszystko przerywać.". A możesz podać jakiś "techniczny" powód dlaczego lepiej nie używać "die()". Czy może jest jakaś zagorzała dyskusja pośród starych wyjadaczy "phpowców" i przerzucają się argumentami za i przeciw :).
```
catch (PDOException $e) {
            $e->getMessage();
        }
```
No tak trochę to bez sensu. Finalnie mam zamiar zwracać zapisywać takie błędy do loga. A co do zwracania pustej tablicy to sens był taki że: jeśli wyskoczy błąd, to do widoku zwrócę pustą tablicę i tam funkcja odpowie nagłówkiem np. 404. Jeśli błędu by nie było to tablica nie jest pusta i skrypt wykonuje się dalej. Czy moje rozumowanie jest błędne?
"Parsowanie adresu pojawia się często, przygotowałbym więc pod to jakąś klasę/metodę/cokolwiek, co by to ułatwiało." Tak zamierzam coś takiego zrobić.
"https://gitlab.com/DOOMinik/test/blob/master/api/core/views/LevelView.php#L13 - potencjalny wyjątek bazy danych jest łapany już wyżej, w samym modelu, więc tak teoretycznie to try catch nie ma tu co złapać." Nie do końca jest tak jak mówisz. W modelu jest łapany wyjątek PDOExeption, natomiast wcześniej jest wyrzucany zwykły wyjątek odnośnie pliku konfiguracyjnego bazy danych. I to on jest łapany w widoku. Ale może to trochę bez sensu, może w modelu już powinienem obsłużyć ewentualny wyjątek pliku.
"https://gitlab.com/DOOMinik/test/blob/master/api/core/Router.php#L22 taki zapis routingu może i na razie ładnie wygląda, ale docelowo będzie bardzo długi i kiepsko skalowalny. Pomyślałbym jak to zrobić inaczej."

Przerobiłem routing i teraz wygląda tak:

class Router
{
    private $controllers = ['user'=>'UserController','level'=>'LevelController','stats'=>'StatsController'];

    public function __construct()
    {
       $this->loadModuleController();
    }

    private function loadModuleController(): void
    {
        $path = trim($_SERVER['REQUEST_URI'],'/');
        $uriArray = explode('/',$path);
        $module = $this->searchModule($uriArray[0]);
        if ($module!="") {
            $className = "Api\\Core\\Controllers\\" . $module;
            $controller = new $className($path);;
        } else {
            header('HTTP/1.1 404 Not Found');
        }
    }

    private function searchModule(string $uriModule): string
    {
        foreach ($this->controllers as $key => $val) {
            if ($key == $uriModule) {
                return $val;
            }
        }
       return "";
    }
}

Czy to właściwy sposób? Chyba teraz będzie bardziej skalowalny. Jedynie co mi tu nie wygląda dobrze (oprócz parsowania adresu, ale to będzie i tak zmienione) to tworzenie nowej klasy.

Nie wiem czy to jedyna opcja aby w ten sposób stworzyć instancję klasy, której nazwa jest w zmiennej. Czy trzeba podawać całą ścieżkę przestrzeni nazw? Szukałem w odmętach internetu jak by to można było inaczej wywołać ale nie znalazłem. Czy może Ty masz na to inny sposób?

Pozdrawiam.

komentarz 6 lutego 2018 przez Arkadiusz Waluk Ekspert (287,950 p.)

"Nazywałbym podprzestrzenie nazw rozpoczynając od wielkich liter. Czyli przykładowe ścieżki...". Nie bardzo mogę wywnioskować gdzie błąd zrobiłem. Przejrzałem raz jeszcze dokumentację PSR i postąpiłem zgodnie z wytyczną. Czy może Ty gdzieś dopatrzyłeś się u mnie małych liter.

Fakt, zasugerowałem się tylko nazwami katalogów - sama przestrzeń jest np. Controllers, lecz już katalog jest controllers. Ujednoliciłbym to wszystko osobiście do wielkich liter, choć nie pamiętam czy któryś PSR to definiuje.

" PSR-2 nie jest nadal w niektórych miejscach przestrzegany (choć jak mówiłem nie jest to obowiązek, to nieoficjalny standard)". Staram się pisać zgodnie ze standardem widocznie coś mi umknęło. Przejrzę jeszcze raz od A do Z.

Takich małych "umknięć" akurat jest sporo:

- https://gitlab.com/DOOMinik/test/blob/master/api/core/controllers/Controler.php#L23 nie switch( a switch (.
- https://gitlab.com/DOOMinik/test/blob/master/api/core/controllers/Controler.php#L45 klamra powinna być w tej samej linii co deklaracja foreach.
- https://gitlab.com/DOOMinik/test/blob/master/api/core/controllers/LevelController.php#L12 spacja przed klamrą.
- https://gitlab.com/DOOMinik/test/blob/master/api/core/controllers/LevelController.php#L27 } else { w jednej linii.
Niby drobiazgi, ale PSR-2 akurat sztywno to definiuje.

"Średnio jestem za używaniem die() - nawet jeśli aplikacja nic dalej nie robi, pozwoliłbym jej się po prostu samej zakończyć, a nie nagle wszystko przerywać.". A możesz podać jakiś "techniczny" powód dlaczego lepiej nie używać "die()".

Nie wiem czy takowy jest, żadnych specjalnych dyskusji raczej też nie spotkałem. Tak po prostu, wydaje mi się naturalniejszym gdy aplikacja sama się ładnie zakończy, bez die czy exitów.

No tak trochę to bez sensu. Finalnie mam zamiar zwracać zapisywać takie błędy do loga.

Ok, logi to spoko sprawa. Na razie jednak nic to nie robi :P

A co do zwracania pustej tablicy to sens był taki że: jeśli wyskoczy błąd, to do widoku zwrócę pustą tablicę i tam funkcja odpowie nagłówkiem np. 404. Jeśli błędu by nie było to tablica nie jest pusta i skrypt wykonuje się dalej. Czy moje rozumowanie jest błędne?

Hmm... jak dla mnie błąd odpowiedzi z bazy danych to nie 404 a np. 500. 404 mówi że zasób nie istnieje, nie wiem jak to się ma do błędu bazy. No i wtedy też nie wiem po co zwracać pustą tablicę, można po prostu walnąć wyjątek i łapać go gdzieś, że tak to określę, na samym dole, ustawiać 500 i/lub dawać jakiś komunikat błędu.

W modelu jest łapany wyjątek PDOExeption, natomiast wcześniej jest wyrzucany zwykły wyjątek odnośnie pliku konfiguracyjnego bazy danych. I to on jest łapany w widoku. Ale może to trochę bez sensu, może w modelu już powinienem obsłużyć ewentualny wyjątek pliku.

Rozumiem, nie zauważyłem tego tak dokładnie. Wydaje mi się, że obsługa konfiguracji to w ogóle powinna być osobna klasa. Łapanie wyjątku to też kwestia tego co potrzeba - jeśli chcesz do tego tak podejść, to będzie trzeba to łapać w każdej akcji. A równie dobrze tak samo można to łapać raz, na samym "dole" i podejmować jedną akcję.

Routing wygląda już z pewnością lepiej. Ścieżki do kontrolerów umieszczałbym po prostu w tablicy $controllers na początku. Tyle że ta tablica też szybko się rozrośnie, pytanie na ile to będzie czytelne. We frameworkach najczęściej definiuje się routing wywołując jakieś metody na klasie czy np. definiując wszystko w pliku tekstowym YAML. Jednego poprawnego sposobu na pewno nie ma, jak to rozwiążesz zależy od Ciebie i potrzeb :)