Task/adding validation to the create campaign method

[Spiker1992]

• Added request file for campaign create
• Refactored create method
• Added spatie's dto package
• Added tests & related seeds for create campaign
• Had to make changes to one of the migrations due to SQLite issues:
  • SQLite didn't like that field was null
  • didn't like to perform of rename whilst changing other columns.

[thybag]

Do you mind expanding a little on what the advantages of adding in the DataTransferObject/Services abstraction? I'm generally a bit wary of adding in unnecessary abstractions, as the Request & Eloquent model objects appear to have the desired functionality built it - so unsure how much merit there is to converting 2 lines of code, to 2 additional classes etc?

Its not a pattern I've really used before (I generally just go the route of using the resource controller policy approach for auth & form request validated data, in to the models fillable for the rest). This is my first time using the Resources over something like fractal re: API output.

Open to having my mind changed, tho my default leaning is always towards keeping stuff as simple as possible.

[Spiker1992]

@thybag it's really down to the size of the application, if the application is smallish than existing Laraval's structure is more than suitable. However, if logic is to grow, in my experience, there are concerns of testability, flexibility, and maintenance.

I would put that extra abstraction in place to solve "where can I find X?" in larger codebases. By separating everything into domains/packages (or maybe even going further - microservices) a developer just has to know where the feature is located and as long as that feature follows a familiar structure (i.e. folder structures that Laravel comes with), an individual developer would have an easier time figuring their way around.

Re DataTransferObject, unlike arrays, when DTO is passed into a method you know exactly what data to expect.

When looking at the class similar to:

class PerformActionService
{
  public function handle(array $data): void 
  { ... }
}

I personally don't know what $data contains, I would have to run through the class and then go through a chain of calls where the $data array was passed & defined. Bypassing in a DTO (ValueObject, or Data Structure class - whatever individual teams prefer to use), all I need to do is look at the DTO class for all available attributes. Of cause, adding a PHPDoc block is another option, but you'd have to not forget to update a PHPDoc block whenever new data is added and you have to blindly trust that passed in array follows PHPDoc block definition... and there are some other issues around it.

Re service classes, it's all about having a reusable class that performs the same action again and again. There are two main pluses:

• a single location where actions are stored
• easier/quicker testability

By having a single class you can mock other dependencies to perform a unit test.

i.e.

class SaveActionService
{
  public function handle(MyDto $data): void 
  { 
    Relation::getMorphedModel('my-model')->save($data->toArray());

    event(MyEvent::class);
  }
}

To unit test my SaveActionService I can

• mock a model within Relation class
• test that correct data was passed into the save method
• verify that event was called using Event::faker()

Since I control DTO (I know what data is sent) and correct data was passed into save() method - I can be confident that SaveActionService will work no matter what. Of cause, this is just a simple example, but it allows you to quickly test individual parts of the system, whilst reducing the number of expensive tests such as database & endpoint tests (leaving them for critical scenarios.)

Re eloquent models, whilst eloquent is a great tool, eloquent models do too much in one place. Due to which models can bloat very quickly should the project grow, with which other issues would follow. This is why, should I anticipate that feature is to have a lot of functionality in most cases I would keep only absolute must in an eloquent model class, such as relations, and move the rests, such as local scopes, into separate classes.

Laravel itself would be an example of a similar logic separation. We have multiple features, Caching, Database, Mail and so onwards. In each feature we have separation of logic: builders, factories and so on... Another example would be Event classes within Laravel, they are DTOs in essence (of cause they do abit more - implement event pattern), you define what data event class should hold & then that data is retrieved within the listener.

I hope I answered your question.

[Spiker1992]

@thybag btw, do let me know how you'd want to proceed, I am more than happy to remove that extra abstraction.

[thybag]

Hi @Spiker1992, Thanks for taking the time to write up such a thorough reply. Def some food for thought in there, as isn't a pattern I've ever really used (At least not in PHP) and I def do err towards the fat models approach with Eloquent.

At least for now, my gut says to stay minimalist with the laravel basics, as given the current scale (~7 models total) the extra complexity in spitting everything out feels like overkill. (The one exception potentially being "Images", as given its working with the filesystem & db at once, potentially would benefit more from being wrapped up in a service).

So yea, sorry to ask 😬 - but yea, my pref would be to skip the abstraction for now + pull up to date with master. The factory/tests & seeders would all be great to get in.

[Spiker1992]

@thybag tests failed due to travis checking against PHP 7.3, which is not compatible with PHP 7.4 functionality that I have in my code.

[thybag]

Ah, in theory may be able to drop php 7.3 (Was mostly being lazy with updating my local dev env initially - is also main reason I've not bumped to Laravel 8). My weeks fairly busy, so if i don't find an evening, potentially may not have chance to review/merge till weekend.