Open joelbutcher opened 1 year ago
@jbrooksuk I've deliberately opened this as a draft PR as I didn't want to spend hours on this PoC if it's not going to be accepted. Above, I've outlined my approach and why I think this is a strong case for using data objects in cachet/core again.
@joelbutcher I can definitely see the upside of this change, not just the IDE completion (which is a big win for development).
How do we handle a situation where we may accept a payload value like notifications
, but actually need to store that as notify
?
@jbrooksuk If the column on the model is not named the same as the validated array key, then that will always cause issues. But that problem is pre-existing, because we're passing the result of $request->validated()
to Model::create(...)
and Model::update(...)
.
The easiest way is to resolve this is to map properties via an array key / value pairing, though this could lead to obscurity within the data objects themselves (when do you map from / to for e.g.).
What would you want to be able to achieve in that scenario? Is it hiding the database table structure by using different parameter names in the request validation versus column names in the database?
One approach could be to have some form of "interceptor" on the model that maps the attributes from Data::toArray()
to values that are accepted by the model itself. You wouldn't need to do this for all attributes, just those that differ.
@jbrooksuk What would you want to be able to achieve in that scenario? Is it hiding the database table structure by using different parameter names in the request validation versus column names in the database?
Yeah, basically. It's more for a nicer API than what the DB was giving. We could rename the columns if it came to it.
This raises the discussion around the appropriateness of
PUT
versusPATCH
requests for the API... @jbrooksuk correct me if I'm wrong, but aPUT
request should send all attributes, even if they've not changed andPATCH
only sends the ones we want to update?
You're right. We've only ever support PUT
endpoints though. We could support PATCH
too, I guess. Though it does add another endpoint to support.
@jbrooksuk Not had a lot of time to dedicate to this. Hoping to pick this back up this week!
This PR serves as a proof of concept to add data objects back into Cachet core. Data objects provide developers with a better type system than arrays, making it clearer what data should be used where.
Initialisation
Data objects may be initialised four ways. Firstly, you may use the traditional
new MyClass
syntax:Secondly, from arrays via the
fromArray
builder method:Thirdly, from a JSON string, via the
fromJson
builder method:Finally, from a form request using the
fromRequest
builder method:Casting
As an array
Data objects can be cast to an array. To do this, the abstract
Data
class uses PHP's reflection API to retrieve all public readonly properties and traverse them into an array of key / value pairs. Array keys are converted tosnake_case
keys using LaravelsStr
helper.As a string / JSON
The base
Data
class implements both the\Stringable
and\JsonSerializable
contracts. As such, all data objects may be cast to JSON string either by traditional string casting(string) $myData
, viastrval
, or by callingjsonSerialize
.Architecture
Where possible, I've tried to abstract most of the functionality into a single class that data objects extend. This is checked in a new
DataTest.php
architecture test:Summary
In summary, data objects provide:
fromArray
,fromJson
, andfromRequest
builder methods