Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat] Use data objects (via Laravel Data) #123

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

joelbutcher
Copy link
Contributor

Background

This app currently passes around various arrays for creating and updating eloquent models. This poses three problems:

  1. Arrays are untyped – there's no knowing what's in them. Data objects are strictly typed, so you know exactly what's being passed around (better for logging and stack traces)
  2. Poor DX – unless your project has strict doc blocks for all arrays, IDE's that support auto-completion don't work.
  3. Bugs – using arrays to carry around commonly used data is bug-prone, all it takes is one spelling mistake and you end up with an Undefined Index error.

Why Laravel Data?

When I was thinking about implementing data objects into Cachet Core, I was considering two separate approaches:

  1. Use POPO's (Plain old PHP objects)
  2. Use a library (spatie/laravel-data)

I opted to use Laravel Data, as the package comes with a bunch of useful tools with it, such as; custom casts, a magic from builder method (that defers to other strongly typed "builder" methods if they exist) and input/output name mapping (e.g. map thisProperty from a snake_case variant).


Implementation

For now, I've just used a vary simple implementation of data objects using Laravel Data; replacing the HTTP FormRequest objects used in the codebase.

Entity Create Update Delete
Component
ComponentGroup
Incident
IncidentTemplate
IncidentUpdate
Metric
MetricPoint N/A
Schedule
Subscriber

Current example – create component

Controller

public function store(CreateComponentData $data, CreateComponent $createComponentAction)
{
    $component = $createComponentAction->handle(
        $data,
    );

    return ComponentResource::make($component);
}

Action

class CreateComponent
{
    /**
     * Handle the action.
     */
    public function handle(CreateComponentData $component): Component
    {
        return Component::create($component->toArray());
    }
}

Next steps

  1. Use Resources from Laravel Data to pass around strongly typed data objects for actual entities (not just the data used to create or update them).

For example: create component action returns ComponentData

  1. Use the repositories to separate storage responsibilities from actions for varying storage mediums (e.g. eloquent or "cached")
  • EloquentComponentStore - interacts with Eloquent query builder and specifically the model being mutated
  • CachedComponentStore – uses an array store (this can be used to speed up tests and remove reliance on the database for unit tests for example)
  1. Add static analysis for strongly typed models (adding @property docs to all models for all their properties and relations for example).

@joelbutcher joelbutcher marked this pull request as ready for review October 27, 2024 11:16
@jbrooksuk
Copy link
Member

@joelbutcher this is great! I didn't know that laravel-data could do validation as well, that's a great benefit.

In terms of your next steps, are these things you want to do in a separate PR? Should I merge this as-is?

By the way, I'm not a fan of the repository pattern. Please don't spend time implementing that 😅

@joelbutcher
Copy link
Contributor Author

@jbrooksuk I would merge this as is and I'll do everything else in follow up PR's 👍

@joelbutcher
Copy link
Contributor Author

@jbrooksuk is there anything else you'd like from this PR or is this good enough for you?

@jbrooksuk
Copy link
Member

@joelbutcher I think this is fine. Would you mind resolving the conflicts and we can get this merged? Schedules just received an update.

@joelbutcher
Copy link
Contributor Author

@jbrooksuk done ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants