Too eager validation - fails common uses.

[stratease]

This is to update our handling of type definitions in the Model. Some questions below.

[stratease]

An issue I can see happening here is a conflict between things that are explicitly being set to NULL versus ones that have not been set at all. This muddies that water, and makes it hard to know which situation is happening.

We ought to embrace MySQL and their DEFAULT and NULL features, and a PHP null would translate to a MySQL NULL. And a non-defined value (e.g. not set to NULL or any other value) should allow MySQL to leverage it's DEFAULT definitions.

And of course if we define a PHP default, that should take precedence and happen here, but I don't believe null in all other cases is safe.

https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html

[stratease]

Scenarios this would fail that we ought to support:

class Fish extends Model {
	protected $properties = [
		'eggs' => 'int',
	];
...

And in use...

// This would fatal.
$fish = new Fish();
$fish->eggs = '10';

[nikolaystrikhar]

While we don't want Fatal errors, it does not feel like deleting is a good solution. I wonder what @borkweb thinks about this approach.

[stratease]

			case 'date':
				return is_string( $value ) ? $value : $value->format( 'Y-m-d' );

Would like to start supporting date values. Thoughts?

[JasonTheAdams]

I'll weigh in simply because this was originally taken from the GiveWP codebase, which I helped build. This doesn't mean this can't deviate from that, but I felt it worth explaining our rationale at the time.

We intentionally didn't want the models to work the way the Eloquent models do, wherein types are cast at the time of retrieval. The reason being we wanted to catch errors early in the process. Using the example you gave:

$fish = new Fish();
$fish->eggs = '10';

If this were a public int $eggs property with strict_types enabled, then yes, this would throw a type error. It raises the question: Why are we trying to assign a string to that property? What's happening at that point in time? The model, like a property, assumes that any prior validation/sanitization/casting has already occurred and it's now safe data. Consider this scenario:

$fish = new Fish();
$fish->eggs = 'buffalo';

That wouldn't be caught unless we now introduce some kind of is_numeric validation... but now we're validating in the model, too. Or we ignore it, in which case it's cast to a number and the problem is harder to follow.

This all comes down to the philosophy that errors should happen as soon as possible.

Hope this helps! 😄

[JasonTheAdams]

As far as the nullable behavior is concerned, I wouldn't include that in this PR, as that's a separate concept than JIT casting. I can certainly see value in declaring whether a given property is nullable. 👍

[nikolaystrikhar]

Maybe changing it with settype( $value, $type ); for multiple cases will be nicer, so you don't need to read all cases really as they are the same.

[nikolaystrikhar]

In the default case on line 363 we can return mixed still.

[nikolaystrikhar]

Regarding booleans, there is a tricky moment in PHP. I'm not sure if we want to support it here, but we had such case in LearnDash, so want to make sure you are aware.

if ( 'bool' === $type ) {
    // We want to convert "false" string to false.
    $value = filter_var( $value, FILTER_VALIDATE_BOOLEAN );
}

[stratease]

I'll weigh in simply because this was originally taken from the GiveWP codebase, which I helped build. This doesn't mean this can't deviate from that, but I felt it worth explaining our rationale at the time.

We intentionally didn't want the models to work the way the Eloquent models do, wherein types are cast at the time of retrieval. The reason being we wanted to catch errors early in the process. Using the example you gave:

$fish = new Fish();
$fish->eggs = '10';

If this were a public int $eggs property with strict_types enabled, then yes, this would throw a type error. It raises the question: Why are we trying to assign a string to that property? What's happening at that point in time? The model, like a property, assumes that any prior validation/sanitization/casting has already occurred and it's now safe data. Consider this scenario:

$fish = new Fish();
$fish->eggs = 'buffalo';

That wouldn't be caught unless we now introduce some kind of is_numeric validation... but now we're validating in the model, too. Or we ignore it, in which case it's cast to a number and the problem is harder to follow.

This all comes down to the philosophy that errors should happen as soon as possible.

Hope this helps! 😄

Thanks, I don't disagree with that mentality at all. I prefer the explicit definitions. Although the side effect would be we need to have some more formal type casting into a DTO or other request input which we are missing at the moment. We need to avoid a lot of the $model->eggs = (int)$_GET['cnt'] mess that would come without that layer interacting with these models.

[JasonTheAdams]

I think extending this to support object casting via interface would be awesome!