2.0 — Feature: Built-in system for converting from query data

[JasonTheAdams]

Resolves #15

As noted in the Issue, models didn't have a first-class way of building from query data, which was a problem since relationships relied on this.

This PR introduces the ability to build from query data right into the Model class. For models that only have primitive properties, no extra work is needed. If the model has class properties, then the way to convert them is by overloading the castValueForProperty static method. It will probably look something like this:

protected static function castValueForProperty( string $type, $value, string $property ) {
	switch($type) {
		case: DateTimeInterface:
			return new ImmutableDateTime($value);
		default:
			 // let the parent handle the primitives
			return parent::castValueForProperty($type, $value, $property);
	}
}

I decided to leave (although rename) the ModelBuildsFromQueryData interface. I'm really tempted to get rid of it. But I wanted to defer to @borkweb, who first added it, as to whether he thinks it's still necessary. I can't personally think of a use-case where we'd want to pass an interface whose only purpose is to be able to create models, when the underlying model is what's actually concretely passed. That sounds more like a factory.

[JasonTheAdams]

I'm really on the fence about the ModelBuildsFromQueryData interface. The interfaces we have are admittedly a bit more robust than I'd likely go with, and this interface is in keeping with that path. It doesn't hurt anything, and is used by the ModelQueryBuilder — it just feels perhaps a bit overly abstracted.

[jonwaldstein]

I'm going to flip this on it's head for a moment...

When we first built models in Give the QueryBuilder was also in progress. We didn't have a ModelQueryBuilder back then as it came later when I realized I wanted to return model instances from the QueryBuilder. So that's why some of this feels a bit weird as it was very much iterative to what we needed along the way.

Looking at this from a framework perspective I don't actually think models should care about how they are being converted from a SQL query. In my humble opinion, that responsibility should be something the ModelQueryBuilder cares about. So what i've thought about is introducing a new callable property on the ModelQueryBuilder called $modelBuilder who's sole job is to convert a Query to Model.

Then in the ModelQueryBuilder we would have something like this:

Using the ModelQueryBuilder by itself would look something like this:

$builder = new ModelQueryBuilder(Donation::class, ConvertQueryToDonation::class);

However, we could add something to the model to make the process a little easier:

class Donation {
 public static function queryBuilder()
    {
        new ModelQueryBuilder(self::class, ConvertQueryToDonation::class);
    }
}

Donation::queryBuilder()->from('posts')....

[borkweb]

I like the additions you are suggestion, @jonwaldstein, however, the guts of this PR is still valid and needed.

What I like about the approach in the PR is that it allows for more flexibility by not being overly opinionated about how a Model gets its data - which greatly enhances its ability to be used in scenarios where the DB library is and isn't in play.

While the name fromQueryData() might be too specific (I think fromData() would be better), the presence of that method enhances the usability of the code by putting data assignment into the model as part of the model itself. A dev doesn't have to go hunting for the answers on how to do it, nor jump through any hoops if they have a non-standard usecase.

Again, I think what you are proposing is a great addition where we want to be more explicit.

[jonwaldstein]

I feel like this could happen up the chain before we get into casting, doing something like $this->isPropertyTypeValid($key, $value)

[JasonTheAdams]

I purposely want to do this here so all someone has to do is overload this function to support additional casting of types.

[jonwaldstein]

@borkweb I do like the idea of simply using the model definition to convert a SQL object to a model. Of course that means the alias's need to match the model attributes. I agree, this is less stuff for a dev to have to think about which i'm all for. However, i'm still just questioning what the model's responsibility is when it comes to converting itself from some object.
Alternatively, maybe the guts of what @JasonTheAdams has here could actually live inside the ModelQueryBuilder somehow because it already has access to the model 🤔

[borkweb]

The ModelQueryBuilder is centered around queries, whereas this implementation is a bit more base level and generic than that. I think a model knowing how to construct itself is perfectly reasonable - at the end of the day, we get to dictate what the Model's responsibility is.

Supporting a baseline ingestion of an array or object in the Model is a great value add for simplicity. If we want a more complicated or nuanced approach for a model or set of models, the method can be overridden and deferred to other objects.

[jonwaldstein]

@borkweb yeah true, it's a nice default to have and you can always do your own thing if need be.

I was poking around Laravel, which obviously was huge inspiration for me (although I originally had to go back to one of the earliest iterations because of php support 😏 and of course simplify a lot)

Some noteworthy concepts that are quite similar to us but still good inspiration:

• Models have a concept of a newInstance that supports an array of $casts (which I really like) and newFromBuilder which is similar to what we're doing here with fromQueryData.
• Query Builder has newModelInstance that just uses the newInstance method from the model.

I like the way they connect Models and QueryBuilder with a naming convention that's easy to understand.

That whole concept of custom $casts in Laravel is something I forgot about. We originally did not add those because we were validating on the way in and doing these query object conversions to cast individually but would really help clean up the redundancy now that the framework is taking more shape.

[borkweb]

@jonwaldstein - yeah, I agree. All three of those look like implementations about what we're talking about above! I had to look up what the $casts thing was as I hadn't used that before 😁

You are spot on about the newFromBuilder() looking like what fromQueryData() is trying to do (minus the casting). I think newFromBuilder() (like fromQueryData()) is a bit too opinionated of a name and something more generic is warranted as it may not be coming from a Builder or a Query.

But the point about supporting new Model creation in a couple of different ways lands us in a nicely flexible and usable spot. 🥳

[JasonTheAdams]

Alright! A couple of topics to hit on here! I'm loving this discussion!

Properties & Casting
One of the most fundamental concepts of Laravel that I disagreed with when we were architecting these models is the idea of casting but not validating. The Laravel models are very loose. You can assign any property to any model in Laravel; it will just cast the value if it knows to and ignore it otherwise. $model->someInteger = "banana" is no problem in Laravel! What makes it really weird in Laravel is this makes it hard to know what exactly you have when you have a model... because it could literally be anything.

I'm a "break early" kind of thinker. So these models validate both the existence of properties as well as their type. As such, I merged $casts, $fillable, and $guarded into the $properties property. I bring that up now to explain why adding $casts now is unnecessary — we already have what we need in $properties to tell us what to cast values to. By definition, it must the same as if it were being otherwise assigned — in fact to do otherwise would risk breaking the casting when it is finally assigned to the model.

Model vs ModelQueryBuilder Shaping
I like the two directions we're coming at on this. On the one hand, I agree with Matt that it's quite sensible for the Model to know how to take an array or object and conform it to itself. It's really simple and doesn't care where the data comes from.

On the other hand, Jon, I agree that requiring that the query returns the exact shape could be an unnecessary requirement. I like the idea that it will assume this to be the case, but it can be overridden. My plan, in a subsequent PR, is to add support for ModelQueryBuilder subclasses. I think what I'll do is add a protected ModelQueryBuilder::getModelFromRow() that, by default, simply calls Model::fromData() (I like that simple name, Matt). This way a ModelQueryBuilder subclass can override that method and reshape the data as it needs to from the incoming row.

[jonwaldstein]

@JasonTheAdams one more thought on the whole $casts concept. I really like how we originally simplified everything into $properties that can be used for both type validation and casting. I'm just curious what your thinking for how to cast custom objects. Of course we could assume they can all be instantiated but there's some objects that are a bit more nuanced and could benefit from a little more validation before instantiating. I think Laravel does this with a castable interface to setup some getters and setters.

[JasonTheAdams]

@borkweb This is the mode I was talking about. I'm curious what you think the default mode should be.