Symfony - pytanie dotyczące projektowania aplikacji (tworzenie obiektów)

Question

Dzień dobry,

Chciałbym zapytać o rady, ludzi bardziej doświadczonych.
Tworze projekt w symfony i na etapie projektowania zrobiłem takie etap tworzenia i zapisywania obiektu do bazy.


Date wejściowe (kontroler) -> PrepareAndSave -> Prepare -> Creator

Działa to tak, że użytkownik wysyła jakieś dane na temat encji, do której odnosi się formularz na stronie.
Obiekt z danymi (właściwościami) przekazywana jest do metody prepareAndSave z serwisu PrepareAndSave.
W powyższej metodzie wykonywane są dwie akcje.

1. Dane przekazywane są dalej do serwisu Prepare, który obrabia sobie w jakiś sposób dane i przekazuje je do Creatora, który z kolei tworzy już konkretny obiekt wskazanej Encji. Po jego utworzeniu i uzupełnieniu danymi obiekt zwracany jest z powrotem do metody prepareAndSave.
2. Gotowy obiekt z punktu pierwszego przekazywany jest do entityManagera, który wrzuca go do bazy.

Czy takie rozwiązanie jest poprawne?

Answer 1

Widzę dużo problemów. 

1. Dlaczego używasz tablic? Phpowy array to kontener na wszystko. Używaj obiektów. Jeśli pochodzą one z formularza, to mogą to być dto z publicznymi polami.
2. Klasy mają niewłaściwe nazwy, nie powinny być czasownikami.
3. Przyjęta nomenklatura nic nie mówi. Nie wskazuje z jakimi obiektami ma do czynienia. 
4. PrepareAndSave z definicji ma więcej niż jedną odpowiedzialność. Albo się tam za dużo dzieje, albo dobierz odpowiednie nazewnictwo.

Comments

1. Mój błąd, źle napisałem używany jest obiekt.
   Nie mogą być to obiekty DTO z tego względu, że dane mogą przychodzić również z innych miejsc (takie mam założenie) np: end point api lub plik np. csv.
2. W sytuacji kiedy ktoś ma gotowy obiekt np z formularza droga do jego zapisu jest nieco inna.
3. Nie wiem za bardzo co mam tutaj odpisać, ja wiem z jakimi obiektami mam do czynienia. Mógłbyś coś podpowiedzieć?
4. PrepareAndSave to w zasadzie robocza nazwa. Nie wiem jak powinienem nazwa taką klase. A wydaje mi się, że wyciąganie logiki z tej klasy do kontrolera nie jest dobrym rozwiązaniem.

---

Nie mogą być to obiekty DTO z tego względu, że dane mogą przychodzić również z innych miejsc (takie mam założenie) np: end point api lub plik np. csv.

I gdzie jest to problem z korzystaniem z dto?

W sytuacji kiedy ktoś ma gotowy obiekt np z formularza droga do jego zapisu jest nieco inna.

To nie powinno mieć większego sensu. SYstem nie powinien ingerować w pochodzenie danych, tylko w to jak je obsługiwać.

Pokaż kod to pogadamy.

---

Właśnie przeczytałem, że FormType można używać również w api.
Zaraz postaram sie poprawić według tego co tutaj napisałeś i pokaże jak to wygląda po poprawkach.

---

@Ehlert, Tylko teraz jedno pytanie, pracując z obiektami za pomocą formularzy musze jakoś robić relacje w nich.

Chciałem skorzystac z CollectionType.

public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder
        ->add('hostname')
        ->add("serverIpAddresses", CollectionType::class, [
            'entry_type' => ServerIpAddressType::class
        ])
    ;
}

 

class ServerIpAddressType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('address')
        ;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'data_class' => ServerIpAddress::class,
        ]);
    }
}

W ten sposób w kontrolerze odbieram i przekazuje dane do formularzA:
 

            $data = json_decode($request->getContent());

            $form = $this->createForm(ServerType::class);
            $form->submit((array)$data);

Niestety kolekcja jest pusta:
 

["serverIpAddresses":"App\Entity\Server":private]=>
object(Doctrine\Common\Collections\ArrayCollection)#863 (1) {
["elements":"Doctrine\Common\Collections\ArrayCollection":private]=>
array(0) {
}
}

Dane przekazuje w ten sposób na endpoint za pomocą postmana:
 

{
    "hostname" : "hostname.com",
    "serverIpAddresses": [
        {
            "address" : "381.121.215.109"
        },
        {
            "address" : "481.151.55.9"
        }
    ]
}

W normalnym formularzu dokładnie wiem jak z tego skorzystać, natomiast tutaj nie do końca rozumiem ten mechanizm. Moge prosić o podpowiedź?

---

Po pierwsze, json_decode ma drugi argument. Należy z niego korzystać, a nie castować jawnie obiekt na arraya, bo nie wiadomo co tam zajdzie.

---

W porządku, a po drugie? :)

---

WebProfiler i sprawdzaj czy dane wpadły do formularza czy nie. Polecam dodać opcję allow_add dla ChoiceType.

---

Jak mam uruchomić webprofiler robić to na end pointach?
nie korzystam z api platfrom

---

Linki do webprofilera są zwracane w nagłówkach odpowiedzi o ile korzystasz z trybu dev + debug.

---

Jak rozumiem to o czym mówisz : https://symfony.com/blog/new-in-symfony-2-4-quicker-access-to-the-profiler-when-working-on-an-api

Chyba bedzie mały problem, zainstalowałem
composer require symfony/debug --dev ^4.2
ponieważ korzystam z S5 

uruchomiłem w akcji debuga.
 

    public function new(Request $request)
    {
        Debug::enable();

Niestety po wysłaniu post'a w postmanie nie otrzymuje nagłówka 
X-Debug-Token-Link

---

Nie wiem co Ty chciałeś instalować, ale mi chodziło o Symfony/profiler-pack, który prawdopodobnie masz zainstalowany.

---

No tak ale pisałeś o debugu, a jedyne informacje jakie o tym znalazłem dotyczace symfony to ten własnie pakiet.

Jedyny artykuł jaki znalazłem w sieci na ten temat to: https://symfony.com/blog/new-in-symfony-2-4-quicker-access-to-the-profiler-when-working-on-an-api
Informacje ssą strasznie ubogie, nie ma informacji co trzeba włączyć, aby ten nagłówek otrzymywac.

---

@Ehlert,
@edit

Moja wina, w momencie kiedy Respons nie jest zwracany przez Akcje w kontrolerze te nagłówki nie są przesyłane. Nie zauważyełem, że w kodzie mialem die. Wszystko działa poprawnie tak jak napisałes. Dziekuje za porade.

---

@Ehlert,

Czy użycie takiego kodu w akcji kontrolera jest prawidłowe cyz powinienem to wyodrębnić do innego serwisu w pojedynczej metodzie?
 

if ($form->isSubmitted() && $form->isValid()) {
    /** @var User $user */
    $user = $form->getData();
    try {
        $this->userManagerService
            ->setUserObject($user)
            ->setUserActive()
            ->encodePassword()
            ->save();

    } catch (\Exception $e) {
        dump($e->getMessage());
    }
}

1. setUserObject - ustawia obiekt użytkownika, na którym pracujemy. Może to być nowy z formularza (DTO) lub jakiś pozyskany z repozytorium z bazy danych.
2. setUserActive - ustawia mu pole active na 'Y'
3. encodePassword - koduje hasło ponieważ z formularza wpada zwykły string
4. save - zapisuje obiekt do bazy (persist + flush)

---

1. Nie wiem jak duży system robisz, ale dobrą praktyką przy nieco większych projektach jest odbieranie z warstwy użytkownika prostych obiektów typu dto, a nie encji do zapisu. 
2. Dump rozumiem tak roboczo? Exception handling nie powinien tak wyglądać. W Symfony są do tego odpowiednie listenery.
3. Nie zwykłem korzystać z serwisów które trzymają stan w taki sposób. Co jak wykonam save przed wywołaniem setUserObject?
4. Wszystko co robi ten manager ogarnąłbym jednym wywołaniem.
5. Manager, manager. Może UserRegisterer oraz metoda register? Od razu robi się czytelniej.

Polecam zacząć pisać testy jednostkowe. W tdd najpierw piszesz test więc niejako narzuca Ci on to co może się wykonać na jednym poziomie abstrakcji. Jeśli przegniesz pałę, to od razu widać, bo test robi się zagmatwany i nieczytelny.

https://youtu.be/DVRJNPnezIk

---

Ja chyba nie rozumiem czym są obiekty DTO. Sądziłem, że to obiekty wychodzace właśnie od użytkownika przetwarzane międyz innymi przez formularz i tu własnie coś takiego jest 

    $user = $form->getData();

Co do punktu 3, wywali wyjątek, ponieważ sam menadżer zakłada, że przed użyciem jakiejkolwiek metody musimy najpierw skorzystać z 

setUserObject

Ustawia ona odpowiednią właściwość i dalej można pracować.
Jeżeli właściwość user wewnątrz obiektu jest pusta, rzuca wyjątkiem.

 

---

https://en.m.wikipedia.org/wiki/Data_transfer_object

Jak mniemam to co wychodzi z formularza to zwykła encja zapisywana do bazy, więc nie dto ​​​​​​

Nie wiem po co ten manager tak działa, chyba tylko po to, żeby użyć tych fluent setterów. Wywołaj tam jedną metodę i cześć.

---

Rozumiem, czyli powinno to być mniej więcej tak:
 

1. Użytkownik wysyła formularz, w kontrolerze odbieram dane z formularza jako DTO.
2. Obiekt DTO przesyłam do metody "register" z serwisu UserRegisterService
3. Metoda register, wykonuje obiekt encji na podstawie danych z DTO i wrzuca do bazy

 

Czyli jak rozumiem, warto też zrobić osobną klase do edycji (EditUserService) oraz usuwania (RemoveUserService)?

Cały czas sądziłem, że kodu powinno być jak najmniej stad chciałem stworzyć UserManagerService, który jest w stanie zarządzać obiektem encji użytkownika i robić z nim różne rzeczy. Dodawać nowego, edytować czy usuwać itd.

Sądziłem, że lepsze będzie dodanie zależności (w tym przypadku EntityManager) w jednej klasie zamiast robić kilka osobnych klas i w nich wszystkich dodawać zależność EntityManagera.

---

Już w trochę lepszą stronę idziesz. Skorygujmy to jeszcze trochę 

1. Dodaj do dtosów walidację na poziomie formularza. Najwygodniej będzie poprzez adnotacje w klasie DTO.
2. UserRegisterService... Wsadź to do folderu Services i możesz wywalić suffix.
3. wykonuje obiekt encji... Do tego zrób oddzielny service UserFactory.
4. W mojej opinii zapis warto oddelegować do repozytorium i tam zrobić persist + flush.

---

Repo mocno WIP, bo to w ramach zaliczenia na studia ale możesz looknąć jak podszedłem do architektury w tym projekcie. 

https://github.com/mjavor/zpsb-cms

Oczywiście dużo rzeczy zasługuje na potępienie  brak testów, readme, fatalny frontend. Bardziej chodziło o przykład zastosowania takich a nie innych rozwiązań, nawet jak dla tak trywialnej logiki biznesowej.

---

Troche kodu na początek:

Dodałem za Twoją radą walidacje do DTO:

class UserRegistrationFormModel
{
    /**
     * @Assert\NotBlank(message="Please enter a password")
     * @Assert\Length(min=6, minMessage="Minimal length is 6.")
     */
    public $plainPassword;

    /**
     * @Assert\NotBlank(message="Please enter an email")
     * @Assert\Email()
     */
    public $email;
}

Utworzyłem katalog Service, w nim User, a w nim klase UserRegister, w której mam metodę register

class UserRegister
{
    private $userFactory;
    private $userRepository;

    public function __construct(
        UserFactory $userFactory,
        UserRepository $userRepository
    )
    {
        $this->userFactory = $userFactory;
        $this->userRepository = $userRepository;
    }

    public function register(UserRegistrationFormModel $userModel)
    {
        $user = $this->userFactory->createUser($userModel);

        $this->userRepository->save($user);
    }
}

Moja fabryka:

class UserFactory
{
    /**
     * @var UserPasswordEncoderInterface
     */
    private $passwordEncoder;

    public function __construct(UserPasswordEncoderInterface $passwordEncoder)
    {
        $this->passwordEncoder = $passwordEncoder;
    }

    public function createUser(UserRegistrationFormModel $userModel): User
    {
        $user = new User();

        $password = $this->passwordEncoder->encodePassword(
            $user,
            $userModel->plainPassword
        );

        $user->setEmail($userModel->email);
        $user->setPassword($password);

        return $user;
    }
}

Całość wywołuje w kontrolerze:

        if ($form->isSubmitted() && $form->isValid()) {
            /** @var UserRegistrationFormModel $user */
            $user = $form->getData();

            try {
                $this->userRegisterService->register($user);
                $this->addFlash("success", $this->translator->trans("New user has been add to database."));
            } catch (\Exception $e) {
                dump($e->getMessage());
            }
        }

Na koniec wpisu dwa pytania:

1. Czy warto jest zrobić dodatkową, abstrakcyjna klasę, np: AbstractUserService, która w konstruktorze wstrzykiwane będzie miała między innymi:

UserFactory $userFactory,
UserRepository $userRepository

I dziedziczyć po niej w każdym serwisie odnoszącym się do użytkownika, aby nie musieć każdorazowo wstrzykiwać tych zależności?

2.  Jak rozumiem praca na obiektach w repozytorium to dobra praktyka? Mówie tutaj o zapisie itd.
Sądziłem, że repozytoria służą tylko do wyciągania danych, a nie ich zapisu.

---

Dalej 

• UserRegister to kiepski pomysł na nazwę gdyż register to czasownik. UserRegisterer ma większy potencjał.
• UserRegistrationFormModel nazwij to po prostu UserRegistrationDto. Obecna nazwa sugeruje że dane muszę przychodzić z formularza. To Cię mocno ogranicza. Może w przyszłości będziesz chciał tworzyć usera przez cli, kolejkę, api. Nazwa zaproponowana przeze mnie jest bardziej uniwersalna.
• Co do repo i zapisu:

  <?php
  
  final class UserRepository extends....
  {
      public function save(User $user): void
      {
          $this->_em->persist($user);
          $this->_em->flush($user);
      }
  }
  
  

Co do ogarnięcia tego wszystkiego jednym flow. Masz serwis UserRegisterer który ma dwie zależności: Factory oraz repozytorium. Factory przyjmuje dto i tworzy encję, repo zapisuje encję zwracaną.

I dziedziczyć po niej w każdym serwisie odnoszącym się do użytkownika, aby nie musieć każdorazowo wstrzykiwać tych zależności?

Może to być dobry pomysł pod warunkiem dobrania odpowiedniej nomenklatury. Bazową nazwałbym UserManagmentAbstractService i np dziedziczy po niej wcześniej wspomniany UserRegisterer. Mimo wszystko na początku dałbym sobie z tym spokój. Nie twórzmy abstrakcji tam gdzie mamy 2 takie same linijki kodu/zależności. Poczekałbym aż zrobi Ci się więcej do wyodrębnienia.

Wywal try catch.

Mocno Ci marudzę o tej nomenklaturze, ale naprawdę ma ona znaczenie i przekłada się na prawidłowy podział odpowiedzialności oraz inne składowe SOLIDu.

---

Wdrożyłem zmiany, które podpowiedziałeś. Zrobie to samo dla pozostałych encji w systemie i postaram się wrzucić na zdalne repo.
Tak jak mówisz odpuszcze na razie abstrakcje.
Mógłbyś mi jeszcze podpowiedzieć co zastosować zamiast try catcha?

A co do marudzenia to bardzo za nie dziękuje, w mojej obecnej pracy nie ma nikogo kto mógłby tak marudzić. Dlatego jestem bardzo wdzięczny za Twoją pomoc :)

---

Kilka refleksji na temat tego co poradziłem: ogólnie takie podejście to kroki w stronę może nie czystej, ale trochę "czystszej" architektury. Metody i kontrolery zaczynają chudnąć, taki kod staje się czytelniejszy. Łatwiej się go rozwija oraz testuje: jednostkowo i nie tylko. Są również problemy: generuje się dość znaczna ilość boilerplate'u. To co np w Django / Larvie zrobiłbyś w 8 linijkach, tutaj zrobisz w 6 plikach.

Warto przemyśleć: jeśli robisz mały projekt, bez perspektyw na rozwój, kilka encji, crud dla nich oraz admin, to nie ma co filozofować. Django albo Larva, dwa tygodnie i projekt z testami masz gotowy. Co innego jeśli rozwijasz system w którym będziesz siedzieć przez najbliższe dwa lata a klient jest poważny. Warto zadbać o komfort swojej pracy, jak i jakość tego co dowozisz.

Co do handlingu: w symfony, jak i w innych fw phpowych masz podejście safe to fail. Tworzysz walidację i formujesz kod tak, aby nie poleciał wyjątek. Jeśli już się tak stanie to user powinien dostać 500 (chyba że biznesowo nie jest to dopuszczalne), a Ty i tak się o tym dowiesz z logów czy monitoringu np New Relic, albo Blackfire.

Poszukaj w dokumentacji Symfony eventów które lecą przy exceptionach.

---

Na pewno poczytam na temat tych eventów.

Chciałem jeszcze zahaczyć o to co było poruszone wcześniej mianowicie o metodę rejestracyjną. Posiadam w systemie jeszcze encje o nazwie server, która ma relacje do encji reprezentującej adres ip.

Dodałem, podobnie jak w przypadku uzytkownika, serwis serverRegisterer oraz metodę register, która rejestruje nowy server w bazie.

Server może (ale nie musi) posiadać adresy ip. Nie są one wymagane podczas dodawania go do bazy.

Rozwiązałem to w ten sposób:

    public function register(ServerRegistrationDto $registrationDto)
    {
        $ipAddresses = $registrationDto->serverIpAddresses;

        $server = $this->serverFactory->createServer($registrationDto);

        foreach ($ipAddresses as $address) {
            $address = $this->serverIpAddressFactory->createIpAddress($address);
            $server->addServerIpAddress($address);
        }

        $this->serverRepository->save($server);

        $this->readyServer = $server;
    }

Jest to poprawne? Czy powinien do tworzenia relacji stworzyć znowu, kolejną klasę i tam wyprowadzić ten proces?

---

Zadaj sobie pytanie czy ta relacja jest kluczowa dla logiki którą implementujesz. Jeśli tak to przekazywałbym utworzone już encje adresów do konstruktora serwera poprzez factory.

Zamykanie logiki w encjach jest czymś mega użytecznym. Jeśli jakiś obiekt, w Twoim przypadku np user nie może istnieć bez username i hasła, to podawaj je przez konstruktor. Dzięki temu masz pewność że nikt nie utworzy tego obiektu bez podania tych wartości.

W powyższym kodzie martwi mnie to pole readyServer. 

---

co do readyServer to planowałem tam wrzucać gotowy obiekt server, który następnie mógłbym pobierać metodą getServer właśnie z tego serwisu ale po Twojej reakcji widzę, że to chyba zły pomysł. Lepiej byłoby w metodzie register dodać return, który zwracał będzie ten obiekt?

dobrze czyli nie jest niczym złym użycie konstruktora w encji, sądziłem, że to po prostu reprezentacja tabeli z bazy danych w przełożeniu na kod.

Czyli w encji Server musiałbym zaimplementować konstruktor, który przyjmować będzie te adresy, następnie w pętli za pomocą metody addIpAddress będzie dodawać je do kolekcji ipAddresses

Dobrze kombinuje?

---

Dobrze. Robisz sobie lokalną tablicę którą zapełniasz stworzonymi ipkami i po pętli przekazujesz ją do utworzenia encji serwer.

ORM jest po to, aby wartości z tablic mapowane na konkretny byt miały swoje własne zachowania, stan, logikę. W przeciwnym wypadku można wszystko rozwiązać tablicami asocjacyjnymi. Vernon Vaugh takie zwykłe przekładanie pustych obiektów na tabele nazywał anemicznym modelem dziedziny. 

Lepiej byłoby w metodzie register dodać return, który zwracał będzie ten obiekt?

Oczywiście że tak. Skąd programista korzystający z tego serwisu ma wiedzieć że najpierw trzeba wykonać register a potem getServer.