Rest api - funkcja dodawania - czy dobrze to rozumiem?

Question

Hej, prosta funkcja dodająca film do bazy.

Cały kod: https://gitlab.com/raptoor2/php-movies---rest-api

public function addMovie(){
    $post = (array)json_decode($this->f3->get('BODY')); // pobieramy całe ciało odpowiedzi curl
    $data = Array('name', 'year', 'category'); // lista pól, które muszą znaleźć się w ciele

    for($i=0;$i<count($data);$i++){
        if(!array_key_exists($data[$i], $post)){
            echo "ERR_NO_MOVIE_DATA_IN_POST";
            return false;
        }
        else if(ctype_space($post[$data[$i]])){
            echo "ERR_EMPTY_".mb_strtoupper($data[$i])."_MOVIE_DATA";
            return false;
        }
    }

    foreach($post as $key => $value){
        $$key = $value; // przypisanie pól z tablicy post do zmiennych na podstawie nazw kluczy
    }

    if(!is_int($category)){
        echo "ERR_CATEGORY_ISNT_NUMBER";
        return false;
    }
    else if(!is_int($year)){
        echo "ERR_YEAR_ISNT_NUMBER";
        return false;
    }


$result = $this->db->exec("insert into movies (name, year, category) values(?, ?, ?)", Array( 1=> $name, 2 =>$year, 3 => $category)); //bind i wykonanie zapytania do bazy

    if($result){
        echo "SUCC_MOVIE_ADDED";
    }
}

sposób nr 2:

    public function addMovie(){
        $post = (array)json_decode($this->f3->get('BODY')); // pobieramy całe ciało odpowiedzi curl
        $data = Array('name', 'year', 'category'); // lista pól, które muszą znaleźć się w ciele

        $err = NULL;
        for($i=0;$i<count($data);$i++){
            if(!array_key_exists($data[$i], $post)){
                $err = $err ?? "ERR_NO_MOVIE_DATA_IN_POST";
            }
            else if(ctype_space($post[$data[$i]])){
                $err = $err ?? "ERR_EMPTY_".mb_strtoupper($data[$i])."_MOVIE_DATA";
            }
        }
        if(!is_int($post['category'])){
            $err = $err ?? "ERR_CATEGORY_ISNT_NUMBER";
        }
        else if(!is_int($post['year'])){
            $err = $err ?? "ERR_YEAR_ISNT_NUMBER";
        }

        if(empty($err))
           $result = $this->db->exec("insert into movies (name, year, category) values(?, ?, ?)", Array( 1=> $post['name'], 2 =>$post['year'], 3 => $post['category'])); //bind i wykonanie zapytania do bazy
            if($result) echo "SUCC_MOVIE_ADDED";
        else{
            echo $err;
        }
    }

 

Czy ten sposób obrabiania i sprawdzania danych jest poprawny? 
Wypisywanie w ten sposób błędów jest zrobione dobrze czy powinienem robić to inaczej?
 

Który z przedstawionych sposobów jest lepszy? A może oba są błędne i powinienem inaczej podchodzić do problemu?

Answer 1

Na pewno masz błąd, jeżeli chodzi o zawartość metody w kontrolerze, jest tam cała logika. Warto, żebyś wymyślił sposób na walidację modelu, w każdym miejscu, gdzie będzie będą zapisywane dane z zewnątrz, np: jak to się dzieje w frameworku Symfony.

Wtedy nie musisz robić walidacji danych w każdym miejscu gdzie jest użyty model. Poczytaj o mechanizmie reflekcji.

Druga sprawa, warto żeby zbindował dane zanim je umieścisz w bazie danych. W innym razie, może Ci ktoś wstrzyknąć swoje zapytanie SQL. Biblioteka PDO, dostarcza prosty do użycia mechanizm bindowania wartości.

Jeszcze pytanie, jak masz zbudowane linki. Metoda dodająca nowy zasób, nie może być obsługiwane przez żądanie GET, musi być użyte żądanie typu POST. Masz tutaj artykuł o tym jak mniej więcej powinna wyglądać struktura Rest Api.

 

Comments

$f3->route('POST /movies', '\App\Controllers\MoviesController->addmovie');

Przechodzimy do kontrolera,  w którym wywołuje metodę addMovie(), czyli logika nie jest napisana w kontrolerze tylko w modelu.

Dodatkowo dane do zapytania są bindowane, własnie biblioteki PDO używam w tym projekcie, jest ona dostarczana wraz z frameworkiem Fat Free, z którego korzystam.

---

Powiedź mi czy to nie jest logika?

    $post = (array)json_decode($this->f3->get('BODY')); // pobieramy całe ciało odpowiedzi curl
    $data = Array('name', 'year', 'category'); // lista pól, które muszą znaleźć się w ciele
 
    $err = NULL;
    for($i=0;$i<count($data);$i++){
        if(!array_key_exists($data[$i], $post)){
            $err = $err ?? "ERR_NO_MOVIE_DATA_IN_POST";
        }
        else if(ctype_space($post[$data[$i]])){
            $err = $err ?? "ERR_EMPTY_".mb_strtoupper($data[$i])."_MOVIE_DATA";
        }
    }
    if(!is_int($post['category'])){
        $err = $err ?? "ERR_CATEGORY_ISNT_NUMBER";
    }
    else if(!is_int($post['year'])){
        $err = $err ?? "ERR_YEAR_ISNT_NUMBER";
    }

Kontroler ma tylko pobierać dane i przekazywać go modelu a później tylko wyświetla wynik z logiki biznesowej.  A ten kontroler sprawdza czy pola istnieją i czy zmienna jest typu całkowitego itd. To wszystko co napisałeś jest logiką biznesową

 

---

to jest mój kontroler:

public function addmovie(){
    $this->api->addMovie();
}

Wywołanie metody w kontrolerze to także zawieranie przez niego logiki?

juz sie pogubilem

---

Wiesz, co najlepiej daj linka do kodu źródłowego na GitHubie. Bo masz tak samo nazwane metody w innych klasach jak i w kontrolerach. To ciężko mi się domyśleć, jak wygląda Twój kontroler a jaki wygląda warstwa modelu.

---

Bardzo prosze :)

https://gitlab.com/raptoor2/php-movies---rest-api

---

Napisałeś, że nie masz logiki w kontrolerze :D  Umieściłem samą logikę kontrolerze, tylko ubrałeś dodatkowo to w klasę i w metodę, równie dobrze, ciało tej metody mogłoby być w metodzie kontrolera i na to samo by wyszło.

Poczytaj trochę o zasadach SOLID, przede wszystkim musisz podzielić klasy na odpowiedzialności. Np. jedna klasa odpowiada za walidację modelu. Jedna za zapisywanie do bazy danych. A w tej metodzie i masz walidację i masz zapisywanie do bazy.

Dodatkowo taką metodę bardzo ciężko będzie przetestować jednostkowo albo będzie to w ogóle nie możliwe.

Jeszcze opinie od siebie. Na Twoim miejscu, bym zaczął się uczyć frameworka Symfony. Ten framework jest pełen dobrych praktyk programowania ale za to ma duży próg wejścia.

Masz w linku potęgę narzędzia jakim jest mechanizm reflekcji, Oczywiście to jest prosty przykład do zaobrazowania phpsandbox

---

Spróbuje jutro usiąść i poprawic kod tak aby spełniał wymagania przedstawione przez Ciebie, dziękuje za porady :)

Jeszcze wczoraj, o rest api slyszalem tylko pogloski, teraz dotoczyles tu refleksje, zaczynam sie gubic w tym wszystkim szczerze mowiąc :D

chyba tak jak mowisz, sprobuje symfony, moze troche poukłada

---

Trzymaj:

https://github.com/HaKIMus/smartphones/blob/feature/smartphones-api/src/Controller/Api/Smartphones/SmartphonesApi.php

Zrobione na Symfony 4. Ostrzegam, że nadal nad tym pracuję, ale Controller już wygląda niczego sobie. ^^

---

Widzę, że chcesz zaimplementować CQRS, ale brakuje Ci tam czegoś takie jakoś command bus, które ma na celu kolejkować i wykonywać zarejestrowane command handlery. Staraj się nie korzystaj ze zmiennych statycznych. Aplikacja Rest powinna być bez stanowa, a druga sprawa ciężko znaleźć błąd jak jakaś zmienna jest statyczna i ktoś gdzieś indziej ją nadpisał i całość źle wpływa na działanie aplikacji.

---

Widzę, że chcesz zaimplementować CQRS, ale brakuje Ci tam czegoś takie jakoś command bus, które ma na celu kolejkować i wykonywać zarejestrowane command handlery.

Command bus nie jest żadnym wymogiem w CQRS a fajnym dodatkiem. B. możliwe, że wkrótce go zaimplementuję, choć nie obiecuję.

Staraj się nie korzystaj ze zmiennych statycznych.

 Chodzi Ci o SmartphonesApiHandler::readExceptionsHandler isn't it?

W sumie to mógłbym wpakować SmartphonesApiHandler jako serwis.

Aplikacja Rest powinna być bez stanowa [...]

Czy mógłbyś rozwinąć?

a druga sprawa ciężko znaleźć błąd jak jakaś zmienna jest statyczna i ktoś gdzieś indziej ją nadpisał i całość źle wpływa na działanie aplikacji. 

To również. Dość chaotycznie to napisałeś. No i ja nie mam statycznych zmiennych. :p