search

dbojdo / wFirma

wFirma SDK
MIT License
13 stars 15 forks source link

#10 PaymentCurrency -> PaymentAmount re-factorisation. Backward compa… #11

Closed dbojdo closed 3 years ago

dbojdo commented 3 years ago

…tibility fixes.

dbojdo commented 3 years ago

@jacekkow Trzeba jeszcze coś zmienić, czy jest ok?

jacekkow commented 3 years ago

Jeśli nie masz nic przeciwko, to zmieniłbym w wszędzie w polach DateTime na DateTimeImmutable (a w metodach przyjmujących parametr typu DateTime, zmieniłbym go na DateTimeInterface). Dodatkowo godzina i minuta byłaby ustawiana na zero. Wrzucę pull request wieczorem.

dbojdo commented 3 years ago

@jacekkow Nie zmieniać tego w tej wersji, bo to łamie kompatybilność wsteczną. Można pomyśleć o porządnej zmianie na PHP 7.1+ z silnym typowaniem / zwracanymi wartościami i wtedy możnaby zamienić wszystkie sygnatury na DateTimeInterface, a wewnątrz używać DateTimeImmutable. Ale to jest grubsza przeróbka na wersję 3.x Pierwsza wersja ten biblioteki była pod PHP 5, stąd DateTime, później zrobiłem wersję 2.0 pod PHP 7 (usunąłem rzeczy przestarzałe, pozwoliłem na nowsze wersji dependencji), ale nie miałem czasu na zmianę sygnatur, itd.

jacekkow commented 3 years ago

Dlaczego łamie?

dbojdo commented 3 years ago

Coś bardzo Cię uwiera ten DateTime. Jak rozumiem pracujesz na \DateTimeImmutable stąd chcesz zmiany na DateTimeInterface? \DateTimeImmutable w wersji PHP 7.3 ma fajną metodę \DateTime::createFromImmutable, którą łatwo możesz użyć przed przekazaniem argumentów do API.

Zgadzam się, że ta zmiana może nie łamie BC sensu stricto, ale zmienia sygnatury metod (typy), co (moim zdaniem) nie powinno mieć miejsca w "minor" release. Taką refaktoryzację widziałbym w całym projekcie, nie tylko w Payments, dodatkowo ze wsparciem dla typów prostych w sygnaturach. Możemy zrobić nowy issue i popracować nad tym.

jacekkow commented 3 years ago

O wiele bardziej niż brak DateTimeInterface boli mnie to, że za każdym pobraniem zmienia się czas, więc dwa DateTime zwrócone z metody date() będą równe (==) lub nie w zależności od "czynników zewnętrznych" (m.in. tego kiedy zostały wykonane zapytania). Tak samo może niestety nie mieć sensu porównywanie dwóch obiektów Payment (mimo że po stronie API zostanie zwrócony identyczny obiekt, to tutaj porównanie może nie zadziałać). Trochę szkoda, że PHP nie ma klasy na samą datę.

I tak, mogłoby mnie zaboleć gdyby przekazany DateTime został przez klasę zmieniony - tutaj DateTimeImmutable dawałoby gwarancję, że to się nie stanie.

dbojdo commented 3 years ago

Rozumiem problem. Niestety PHP nie daje dobrej metody na ogóle rozwiązanie tego problemu. Nawet jeśli chodzi o API, to używanie \DateTimeImmutable rozwiąże tylko częściowo problem. Zobacz poniższy przykład:

$date = new \DateTimeImmutable();
$payment = new Payment(..., $date, ...);
$payment->date() == $date; // (jeżeli pomijamy ->setTime(0,0), to wszystko ok w tym miejscu
$createdPayment = $paymentsApi->add($payment);
$createdPayment->date() == $date; // tu już mamy problem, ponieważ zwrotka z API została zdeserializowana, a więc obiekt daty został utworzony ponownie z 1 - 2 sekundowym opóźnieniem (w zależności jak długo trwa komunikacja z API).

Niestety, PHP to nie Java i nie ma jak ogólnie zaimplementować metody .equlas. Można taką metodę stworzyć, ale wtedy trzeba ją wywołał explicite (== nie tego łyknie). Może kiedyś doczekamy się jakiegoś ogólnego interfejsu Comparable czy coś w tym stylu.

Można w jakiś sposób ograniczyć problem, przez tworzenie dat zawsze z zerowym czasem (w trakcie rdeserializacji), ale żeby porównanie działało, data "z zewnątrz", z którą porównujesz, również musiałaby być tak utworzona. Więc, tak czy inaczej, gwarancji na poprawne działanie porównania nie ma :(

Chyba, że czegoś nie zrozumiałem?

jacekkow commented 3 years ago

Wszystko "w punkt".