Walidacja i wysyłanie maila z wiadomością z formularza w laravelu. SOLID i odpowiednie wzorce, czysty kod.

0

Zrobiłem to co opisałem w temacie. Kod działa. Ale chcę napisać czysty kod zgodny z SOLID taki aby nie można było się do niczego przyczepić i nie jestem pewny czy mi wyszło dlatego proszę o ocenę
Kontroler

<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;
use App\Interfaces\NotificationValidator;
use App\Interfaces\Notifier;
use App\Notifications\SendEmail;
use App\Mail\SimpleEmail;


class SendMessageFromUserController extends Controller
{
    
    public function sendMessageFromUser(Request $request)
    {
       $data = $request->validate([
          'email' => 'max:30',
          'subject' => 'max:30',
          'message' => 'required|min:3|max:1000'
       ]);

       $notification = new SendEmail(new SimpleEmail($data['message'], $data['subject'], $data['email']), 
       	config('mail.management_email'));
      
       return $notification->notify();
    }
}


Klasa budująca maila


namespace App\Mail;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Mail\Mailable;
use Illuminate\Queue\SerializesModels;

class SimpleEmail extends Mailable
{
    use Queueable, SerializesModels;

    private $sender;
    private $msg;

    /**
     * Create a new message instance.
     *
     * @return void
     */
    public function __construct($msg, $subject = "Brak tematu",$sender = "Brak adresu")
    {
        $this->subject = empty($subject) ? "Brak tematu" : $subject;
        $this->sender = empty($sender) ? "Brak adresu" : $sender;
        $this->msg = $msg;
    }

    /**
     * Build the message.
     *
     * @return $this
     */
    public function build()
    {

        return $this->from($this->sender)
                    ->view('mails.simple_email')
                    ->subject($this->subject)
                    ->with(['messageContent' => $this->msg]);
    }
}

Klasa wysyłająca maila

<?php

namespace App\Notifications;


use App\Interfaces\Notifier;
use App\Mail\MessageFromUser;
use Mail;
use Illuminate\Mail\Mailable;

class SendEmail implements Notifier
{
   private $mail;
   private $receipient;

   public function __construct(Mailable $mail,$receipient)
   {
     $this->mail = $mail;
     $this->receipient = $receipient;
   }

   public function notify()
   {
      Mail::to($this->receipient)->send($this->mail);

      return Mail::failures() ? back()->withErrors(['Nieoczekiwany błąd podczas wysyłania wiadomości']) : back()->with('success','Pomyślnie wysłano wiadomość');
 
   }
}

Czy nie wieje tutaj amatorszczyzną?

2

Nie biorąc pod uwagę tego, że to jest Laravel, to wydaje mi się, że jest OK :) Jedynie co bym zrobił to tylko poprzenosił wszystko do serwisów, ale to moje naleciałości z Symfony tylko. Czy napisałeś sobie do tego testy jednostkowe? Jeśli uda Ci się napisać je bez zmiany w kodzie to na 99% masz dobrze :)

Dodaj jeszcze tylko typowanie parametrów w klasie SimpleEmail :)

Ogólnie niedawno spłodziłem takie coś, jest tak kilka punktów które mogły by Cię zainteresować jeśli chodzi o produkowanie dobrego kodu: https://phpcenter.pl/jak-rozpoznac-aplikacje-legacy/

PS. nie dopatrzyłem wcześniej, klasa SendEmail nie może zwracać redirect'a - musi zwrócić true/false albo rzucić wyjątek, a w kontrolerze dopiero to obsługujesz i robisz przekierowanie.

2

Siedzę w laravelu od 5 lat więc uwagi ode mnie:

  • validator do osobnej klasy, używaj tego https://laravel.com/docs/5.8/validation#form-request-validation ;
  • validacje rób na arrayach a nie na stringach połączonych |, tak jest czytelniej;
  • jeśli już chcesz mieć taką klase jak send mail (ja nie robię takich) to zmień to tak by wstrzykiwać ją przez DI, a nie używać new.
  • $sender = "Brak adresu" ??? pomijając już fakt użycia polskich słów, wrzucaj po prostu nulla i jeśli null to używasz defowych wartości a ajk nie null no to tego co przesłane

To tak na początek.

0

Wydaje się, że email też mógłby mieć required i znaków tak do 40, subject również trochę więcej więcej, fajnym pomysłem jest też honeypot: https://github.com/msurguy/Honeypot

0

@czysteskarpety: z mailem racja tylko nie 40 a 320, przynajmniej jeśli się chce przyjąć każdy email spełniający standard :) https://stackoverflow.com/questions/386294/what-is-the-maximum-length-of-a-valid-email-address osobiście daje zawsze limit na 255, by nie wylatywać poza varchara w bazie.

3

To ja od siebie dodam, że jak walidujesz maila to możesz użyć po prostu email zamiast max

0

@mr_jaro

validator do osobnej klasy, używaj tego https://laravel.com/docs/5.8/validation#form-request-validation ;

Zgadzam się w 100% już poprawione.

validacje rób na arrayach a nie na stringach połączonych |, tak jest czytelniej;

Nie jestem pewny czy rozumiem, chodzi o to aby były dwie tablice - w jednej wartości a w drugiej zasady?

jeśli już chcesz mieć taką klase jak send mail (ja nie robię takich) to zmień to tak by wstrzykiwać ją przez DI, a nie używać new

Tego też do końca nie rozumiem. Aktualnie jako parametr konstruktora podaję instancję klasy budującej maila i i przechowuję ją jako właściwość obiektu wysyłającego. To nie jest dependency inject? Jak to powinno według Ciebie wyglądać? Ponadto jeżeli nie robić klasy wysyłającej maila, to napisać tylko

Mail::to()

w kontrolerze? Czy może gdzieś to upchnąć jeszcze gdzie indziej?

$sender = "Brak adresu" ??? pomijając już fakt użycia polskich słów, wrzucaj po prostu nulla i jeśli null to używasz defowych wartości a ajk nie null no to tego co przesłane

Wiadomość z formularza będzie wysyłana do mnie, czy to źle, że użyłem języka Polskiego? Chodzi o to, że użytkownik nie ma obowiązku podawania swojego emaila dlatego wydaje mi się, że w tej sytuacji dobrze by było zamiast nulla coś bardziej opisowego. Czy się mylę? Jak rozumiem masz na myśli to aby parametr domyślnie przyjmował wartość null a gdy parametr z takową wartością się pojawi to w konstruktorze podmienić na defową? Czemu tak jest lepiej? Weź pod uwagę, że nie wiemy czy użytkownik podał maila a jeżeli takowa sytuacja się pojawi to przekazany zostanie pusty string który nie jest nullem i wtedy prześlizgnie się i defowa wartośc nie zostanie przypisana.

@czysteskarpety

Wydaje się, że email też mógłby mieć required i znaków tak do 40, subject również trochę więcej więcej

Chodzi o to, że nie chcę na użytkowniku wymuszać aby podawał swój email. A z subject to mógłbym się zgodzić.

@jurek1980

To ja od siebie dodam, że jak walidujesz maila to możesz użyć po prostu email zamiast max

W sumie tak, musiałbym tylko zaznaczyć, że ma to sprawdzać w sytuacji gdy jakiekolwiek dane w tym polu się pojawią.

1

@Mostek87: odpowiadając na komentarz, tu łap fragment jak ja robię przykładowe wysyłanie maila, jest to na lumenie więc bez systemu notyfikacji, ale w laravelu też często się go nie zaprzęga do roboty.

namespace App\Http\Controllers;

use Illuminate\Mail\Mailer;
use App\Helpers\ApiResponse;
use App\Mail\GroupUserChangeStatus;
use App\Http\Requests\GroupUserSetStatusRequest;
(...)

class GroupUserController extends Controller
{
(...)
    public function setStatus(GroupUserSetStatusRequest $request, Mailer $mailer)
    {
            (...)
            $mailer->to(['email' => $group_user->user->email])->send(new GroupUserChangeStatus($group_user));

        return ApiResponse::responseSuccess([]);
    }
(...)
}

namespace App\Mail;

use App\Models\GroupUser;
use Illuminate\Bus\Queueable;
use Illuminate\Mail\Mailable;
use Illuminate\Queue\SerializesModels;

class GroupUserChangeStatus extends Mailable
{
    use Queueable, SerializesModels;

    public $group_user;

    /**
     * GroupUserChangeStatus constructor.
     * @param GroupUser $group_user
     */
    public function __construct(GroupUser $group_user)
    {
        $this->group_user = $group_user;
    }

    /**
     * Build the message.
     *
     * @return $this
     */
    public function build()
    {
        return $this->view('emails.group.change_status');
    }
}
0

Kod po poprawkach.
Kontroller

namespace App\Http\Controllers;

use App\Http\Requests\SendMessageToManagementRequest;
use App\Services\SendMessageToManagement;


class ContactFormController extends Controller
{
    
    public function sendMessageFromUser(SendMessageToManagementRequest $request, SendMessageToManagement $message)
    {
       
       return $message->send($request->all())['success'] ? back()->with('success','Pomyślnie wysłano wiadomość') : back()->withErrors(['Nieoczekiwany błąd podczas wysyłania wiadomości']);
    }

     public function showContactForm()
    {
    	return view('contact');
    }
}

Serwis

<?php

namespace App\Services;

use App\Notifications\SendEmail;
use App\Mail\SimpleEmail;

class SendMessageToManagement
{
	public function send(array $data)
	{
       $email = new SimpleEmail($data['message'], $data['subject'], $data['email']);
       $receipient = config('mail.management_email');
       $notification = new SendEmail($email,[$receipient]);
       return $notification->notify();
	}
}

Wysyłanie emaila


namespace App\Notifications;

use App\Mail\MessageFromUser;
use Mail;
use Illuminate\Mail\Mailable;

class SendEmail
{
   private $mail;
   private $receipient;

   public function __construct(Mailable $mail, array $receipients)
   {
     $this->mail = $mail;
     $this->receipients = $receipients;
   }

   public function notify()
   {
      $response = ['success' => true, 'failed_emails' => []];

         foreach($this->receipients as $receipient)
         {
            try
            {
               Mail::to($receipient)->send($this->mail);
            }
            catch(\Exception $exception)
            {
                $response['success'] = false;
                array_push($response['failed_emails'], $receipient);
            }
         }
         
      return $response;
      }
   
}

Budowanie emaila

<?php

namespace App\Mail;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Mail\Mailable;
use Illuminate\Queue\SerializesModels;

class SimpleEmail extends Mailable
{
    use Queueable, SerializesModels;

    private $sender;
    private $msg;

    /**
     * Create a new message instance.
     *
     * @return void
     */
    public function __construct(String $msg, String $subject = null, String $sender = null)
    {
        $this->subject = $subject ?? "Brak tematu";
        $this->sender = $sender ?? "Brak adresu";
        $this->msg = $msg;
    }

    /**
     * Build the message.
     *
     * @return $this
     */
    public function build()
    {

        return $this->from($this->sender)
                    ->view('mails.simple_email')
                    ->subject($this->subject)
                    ->with(['messageContent' => $this->msg]);
    }
}

Walidacja i autoryzacja

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

class SendMessageToManagementRequest extends FormRequest
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        return true;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
          'email' => ['nullable','email'],
          'subject' => ['nullable','max:40'],
          'message' => ['required','min:3','max:1000']
        ];
    }
}


1 użytkowników online, w tym zalogowanych: 0, gości: 1