[Code Review] Formularz kontaktowy

Question

Stworzyłem formularz kontaktowy (w sumie poprawiłem stary) dla mojej strony jaki-jezyk-programowania.pl i napisałem na moim blogu konkursowym DSP2017 serię postów opisujących ten proces krok po kroku.  Są 4 części. TU - pierwsza z nich, gdzie opisuję założenia. Zwracam się do was z prośbą o review. 
Uwzględniam:

• HTML5
• Javascript
• JQuery
• AJAX
• PHP
• Swiftmailer
• Dostępność

Więc każdy znajdzie coś dla siebie ;) W postach starałem się w miarę wszystko uzasadniać, więc jeśli macie czas to przeczytajcie i ewentualnie mnie naprostujcie gdzie się mylę. Postaram się dopracować co trzeba według waszych sugestii, żeby te wpisy stały się rzetelne i żeby inni początkujący na tym korzystali ;)

Jeśli chcecie rzucić okiem tylko na kod to efekty końcowe są na końcu każdego postu w jsfiddle. Posty z ostatecznym kodem: HTML-JS i PHP

Dzięki sugestiom od  event15 (PHP) oraz Ivan (dostępność) zagłębiłem się w temat i oto tego efekt ;). Większość rzeczy musiałem się douczyć od podstaw, mam nadzieję, że nie trafiłem na jakieś stare źródła. Od Ivana dostałem również PR (dzięki wielkie jeszcze raz) w którym poprawiał dostępność strony i zastosował strukturę z role="document" dla formularza. Tu linki do githuba:HTML i JS. Ja nieco zmodyfikowałem formularz i pozbyłem się tego (bo wydawało mi się zbędne) pozostając przy samym role="dialog" + dopracowałem kwestię jego dostępności. Gryzło się to też z niektórymi założeniami W3C. Dajcie znać czy tak też jest ok ;)

Comments

Github: https://github.com/C0deboy/jaki-jezyk-programowania

Answer 1

<?php

require_once 'swiftmailer/lib/swift_required.php'; //najlepiej uzyc composera
$config    = require_once 'emailconfig.php'; //brakuje __DIR__
$secretKey = $config['secretKey'];

$userEmail = $_POST['userEmail'];
$subject   = filter_var($_POST['subject'], FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES);
$message   = filter_var($_POST['message'], FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES);

$checkIfBot = file_get_contents('https://www.google.com/recaptcha/api/siteverify?secret=' . $secretKey . '&response=' . $_POST['g-recaptcha-response']);
$recaptcha  = json_decode($checkIfBot);

if (!filter_var($userEmail, FILTER_VALIDATE_EMAIL)) {
    $alert = 'Podaj poprawny email!'; //powinien byc arrayem
} else if (empty($subject)) {
    $alert = 'Wpisz jakiś temat!'; //powinien byc arrayem
} else if (empty($message)) {
    $alert = 'Pusta wiadomość? Napisz coś!'; //powinien byc arrayem
} else if ($recaptcha->success === false) {
    $alert = 'Potwierdź, że nie jesteś robotem!'; //powinien byc arrayem
} else {
    $transport = Swift_SmtpTransport::newInstance($config['mailServer'], $config['port'])
                                    ->setUsername($config['username'])
                                    ->setPassword($config['password']);

    $mailer = Swift_Mailer::newInstance($transport);

    $message = Swift_Message::newInstance($subject)
                            ->setFrom($userEmail)
                            ->setReplyTo($userEmail)
                            ->setTo($config['myEmail'])
                            ->setBody($message);//ze niby body, to nasz message instance? wywali error.. nadpisujesz zmienna

    $emailSent = $mailer->send($message);

    if ($emailSent) {
        $alert = 'Wysłano. Dzięki za wiadomość!';
    } else {
        $alert = 'Coś poszło nie tak.';
    }
}

$response = json_encode(array( //shortcode?
    'text' => $alert
));

exit($response);

?> <- niepotrzebne

Taki strukturalny kodzik. Nic specjalnego... prócz użycia swiftmailera : )

• Ogólnie nie tworzysz własnych funkcji, co utrudnia późniejsze wykorzystanie kodu.
• W konfiguracji użyłeś [], a w programie używasz array(). Bądź konsekwentny.
• Masz tam błąd, który nie pozwala na normalne działanie programu. (Nadpisujesz message instancją)
• Jeśli ktoś popełni wiele błędów w formularzu i tak zobaczy ten jeden.
• Jest możliwość wysłania recaptchy bez jsa: http://stackoverflow.com/questions/18853007

Oto moja odpowiedź na twój kod: (robiona na kolanie w 30min)

https://gist.github.com/xandros15/3c17a9c5961cca901bbfea3c226eca3b

Comments

No strukturalny, bo na razie nie zagłębiałem się w backend (głównie z braku czasu). Nie miałem jeszcze styczności z Symfony, composer itd, a widzę, że z tego korzystasz. 

Alę dzięki za gotowe rozwiązanie, na pewno do tego wrócę. Wracając do mojego rozwiązania, to w tak prostym formularzu nie jest dopuszczalny taki strukturalny kod? Nie spełnia swojej roli?

• Masz tam błąd, który nie pozwala na normalne działanie programu. (Nadpisujesz message instancją)

O jakim błędzie mówisz? Sprecyzujesz? Co to znaczy, że nie pozwala na normalne działanie programu? Maile się wysyłają i wszystko przebiega zgodnie z założeniami ;)

Jest możliwość wysłania recaptchy bez jsa

Czytałem o tym, ale jak podają na oficjalnej stronie, tylko z JS-em mamy największe bezpieczeństwo. Gdy JS jest wyłączony to też jest wyłączona część zabezpieczeń. A z invisibleRECAPTCHA w ogóle nie jest możliwe korzystanie w takim wypadku.

Tak szczerze, to w dzisiejszych czasach nadal wspiera się przeglądarki bez JS-a? ;)

---

> O jakim błędzie mówisz? 

  $message = Swift_Message::newInstance($subject)
                            ->setFrom($userEmail)
                            ->setReplyTo($userEmail)
                            ->setTo($config['myEmail'])
                            ->setBody($message);//ze niby body, to nasz message instance? wywali error.. nadpisujesz zmienna

 

 > Symfony, composer itd, a widzę, że z tego korzystasz.  

Nie ma tam ani jednej rzeczy z symfony.

Composer, a dokladniej zewnetrzne biblioteki, ułatwiaja nam znaczonco prace. Jakbym mial budować samemu te walidatory, to byś się pogubił w moim kodzie.

>  w tak prostym formularzu nie jest dopuszczalny taki strukturalny kod?

Zależy, co chcesz osiągnąć i ile masz na to czasu. Bo jeśli na to zagadnienie możesz poświęcić 30min. To robisz to najszybciej, jak się da. 

Dlaczego zamykanie w funkcjach jest takie ważne? 

• Czytelność kodu
• DRY
• KISS
• Możesz wyciągnąć funkcje, ktorę cię interesują i użyć w innym projekcie

Jak zapewne zauważyłeś (albo i nie), zrobiłem 2 wersje obsługi wysłania maila:

1. Zapytanie RESTowe, po ajaxie.
2. Zapytanie standardowe, z przetrzymywaniem stanu w sesji.

Zamiast sesji, mógłbym użyć formularza/kontenera, ale to już wyższa szkoła jazdy. (sprawdzenie, czy request był postem, czy getem etc.)

> Tak szczerze, to w dzisiejszych czasach nadal wspiera się przeglądarki bez JS-a? ;)

Chyba mój kindle nie obsluguje JSa.

---

Whoa, z tym $message to rzeczywiście się pospieszyłem, aż dziwne że to normalnie działało :O

"symfony/validator": "Use Symfony validator through Respect\\Validation",

Nie korzystasz z walidatora Symfony? Tak wywnioskowałem z tej linijki.

Chyba mój kindle nie obsluguje JSa.

Mój (Paperwhite 3) obsługuje, ale to i tak jest wersja eksperymentalna przeglądarki.

Przysiądę do tego PHP, ale raczej nie będzie to 30 min tak jak mówisz. W 30 min to zrobiłeś to Ty, a nie kompletnie raczkująca osoba ;)

Dzięki jeszcze raz ;)

---

> Nie korzystasz z walidatora Symfony?

Nie, użyłem respecta, którego używa symfony we własnym walidatorze.

> Tak wywnioskowałem z tej linijki.

1. To jest plik wygenerowany automatycznie, by instalacja przebiegła łatwiej
2. ta paczka jest w `"require-dev"` więc nie jest instalowana normalnie
3. to tylko komponent/moduł symfony, a nie cały framework.

> aż dziwne że to normalnie działało :O

Bo zadziała, ale jest to bardzo mylące. :P

Zanim nadpiszesz zmienna, jest ona przekazywana do funkcji/metody i tam przerabiana. Po wykonaniu kodu jest nadpisywana.

---

rzeczywiście ;)

Answer 2

Do reszty nikt się nie przyczepi? ;)