-
Notifications
You must be signed in to change notification settings - Fork 2
2.0 — Feature: Improved Properties #22
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
Conversation
Co-authored-by: Jon Waldstein <[email protected]>
Co-authored-by: Matthew Batchelder <[email protected]>
I'm still chewing on a few ideas. Required properties Read-only properties Property hooks Property validation I'm open to the idea that maybe it's best to not start with these. But I wanted to at least record them as I'm thinking up ideas. |
A method that I skipped was |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is good and i'm all for the idea of defining a better structure for the model properties.
What i think is missing from this library is a static analysis integration. I think there are things we could improve if we had it in place.
Regardless, that should not block us from merging the PR.
$initialValues = []; | ||
|
||
foreach (static::$properties as $key => $type) { | ||
if ( ! array_key_exists( $key, $data ) ) { | ||
// Skip missing properties when BUILD_MODE_IGNORE_MISSING is set | ||
if ( $mode & self::BUILD_MODE_IGNORE_MISSING ) { | ||
continue; | ||
} | ||
Config::throwInvalidArgumentException( "Property '$key' does not exist." ); | ||
} | ||
|
||
// Remember not to use $type, as it may be an array that includes the default value. Safer to use getPropertyType(). | ||
$instance->setAttribute($key, static::castValueForProperty(static::getPropertyType($key), $data[$key], $key)); | ||
$initialValues[ $key ] = static::castValueForProperty( static::getPropertyDefinition( $key ), $data[ $key ], $key ); | ||
} | ||
|
||
return $instance; | ||
return new static( $initialValues ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe using model methods in an initialized Model Instance is a better way of cunstructing fromData.
The reason is that we purposely give the ability to developers to execute additional actions during the constructor. That could be an external integration of getting model properties dynamically from somewhere else (like from schema).
Why did you prefer to collect them in an array first and then set them all up during the model's construction ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see where you're coming from, but I think it means deviating from the standard flow — models are constructed with all of their initial data. So on the other hand, not doing that means the afterContstruct
method doesn't work as someone might think it would in this case.
I think if we're doing something more complex than what this method offers, then either the subclass should introduce its own method or overload this one.
The way this works is exactly as intended in the dozens of models I've made with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can stand behind that. I do think its fair, and thats what im doing already in the Schema integration ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to merge once the doc-block is added
Resolves #21
Depends on #19
Ok, I'm actually pretty excited about this PR. It's a work in progress and I'll continue to update the description as I make changes, but it's a major refactor of how Models work under the hood, which opens up some really cool possibilities.
New concepts
In 1.x, models properties was a simple property that supported a property type and default. We were limited to a single type and expressions couldn't be used as it was a property. Under the hood, the properties were stored as-is and referenced to check types, the values were stored as basic arrays.
This introduces three new classes to beef things up:
ModelPropertyDefinition
This class is responsible for defining the attributes of a property — it's type, default, whether it's required, how to cast it, and so forth. This provides much more capability to what a property is capable of, and can be subclassed to easily define multiple properties of the same sort.
ModelProperty
This class encapsulates a single, specific key-value pair property. It receives a key and definition. A lot of the logic that lived in the Model for tracking values, original values, whether they were dirty, and so forth now lives within the
ModelProperty
instance.ModelPropertyCollection
Rather than being stored in an array in the Model, properties are kept in a collection. This gives us a way of encapsulating how properties are stored and retrieved.
New possibilities
Union types
The
ModelPropertyDefinition
can have multiple types set:$definition->type('int', 'float')
. This allows the property to be one of a number of types.Castable properties
The
ModelPropertyDefinition
can receive a casting method so when data is turned into the model, the property can handle the casting itself.Nullable properties
Previously, properties were all nullable, and in fact a null value was sometimes considered not set. It wasn't consistent. In any case, there are times a null value should not be allowed, which is now possible.
Reusable properties
The
ModelPropertyDefinition
may be subclassed with things such as types and casting preset. For example, aTimeStampDefinition
could be used for common properties such ascreated_at
andupdated_at
.Required properties
The
ModelPropertyDefinition
class can be marked as required and "required on save". The difference between required and required on save is that required means a model cannot be created without that property, whereas required on saved means it can be, but it won't allow itself to be saved unless the property is set.Dynamic default values
The
ModelPropertyDefinition
can receive aClosure
as the default value. If this is the case, then the closure is called when theModel
is instantiated, allowing for dynamic values. This is useful for things like a timestamp wherein we may want the value to be set at the time of instantiation.Property reverting
A single property can be reverted to it's original state. So if you change a property and want to undo the change for that property, it can be easily reverted to it's original state, which will also mark it as clean.
Property unsetting
A property can be unset with it's value completely stripped. This isn' t the same as a null value and is equal to
unset($object->property)
, without actually removing the property altogether. This can be done on non-nullable properties, and results in$property->isSet()
returning false (which a null value won't do). Whether the property is dirty at this point has to do with whether it started unset or not.Fluent and expressive property definitions
The
Model::$properties
way of setting properties will continue to work with its shorthand structure. It's super simple and nice for basic models. But there's also a new (and both can be used) way of setting up a Model's properties:Backwards Compatibility
Model API
This is largely backwards compatible. The Model's public API is exactly the same as it was, with some new methods being introduced. It's the protected methods that changed dramatically, with a number being removed entirely.
Model Properties
I did managed to keep the
Model::$properties
property compatible, which arguably would've been the biggest headache. One difference between using the existing shorthand (e.g.['string', 'bob']
) versus a Definition is that the parsed shorthand Definition is made nullable for backwards compatibility.Model
fromData
changesThis does affect how casting data to a model works. Here's the new order: