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.

Sign up for GitHub

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

Too eager validation - fails common uses. #9

Closed
wants to merge 3 commits into from
Closed

Too eager validation - fails common uses. #9

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79f1c09
Too eager validation, fails common uses.
stratease Aug 25, 2023
3c5ffab
Proposed castAttribute() method.
stratease Aug 25, 2023
3b1071b
Updates from CR, and new tests for new functionality. Adding casting,…
stratease May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions src/Models/Model.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ protected function getPropertyDefault( string $key ) {
protected function getPropertyDefaults() : array {
$defaults = [];
foreach ( array_keys( $this->properties ) as $property ) {
// @todo This could be an edge case bug, we shouldn't assume `null` is always a valid default value.
$defaults[ $property ] = $this->getPropertyDefault( $property );
Copy link
Contributor Author

@stratease stratease Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

Expand Down Expand Up @@ -326,10 +327,48 @@ public static function propertyKeys() : array {
return array_keys( ( new static() )->properties );
}

/**
* Will infer the type based on the property definition and
* cast the type on the value passed in.
*
* @since TBD
*
* @param string $key The property name.
* @param mixed $value The value to be cast.
*
* @return array|bool|int|string The $value after being cast to it's type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
protected function castAttribute( string $key, $value ) {
// Handle nullable fields as expected.
if ( is_null( $value ) ) {
return null;
}

$type = $this->getPropertyType( $key );

switch ( $type ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case 'int':
return (int) $value;
case 'string':
return (string) $value;
case 'bool':
return (bool) $value;
Copy link
Contributor

@nikolaystrikhar nikolaystrikhar Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );
}

case 'array':
return (array) $value;
case 'date':
return is_string( $value ) ? $value : $value->format( 'Y-m-d' );
Comment on lines +369 to +370
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Would like to start supporting date values. Thoughts?

case 'datetime':
return is_string( $value ) ? $value : $value->format( 'Y-m-d H:i:s' );
default:
return $value;
}
}

/**
* Sets an attribute on the model.
*
* @since 1.0.0
* @since TBD No longer throws Exceptions with a default type validator. Use custom validators instead.
*
* @param string $key Attribute name.
* @param mixed $value Attribute value.
Expand All @@ -338,14 +377,13 @@ public static function propertyKeys() : array {
*/
public function setAttribute( string $key, $value ) : ModelInterface {
$this->validatePropertyExists( $key );
$this->validatePropertyType( $key, $value );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';

Copy link
Contributor

@nikolaystrikhar nikolaystrikhar Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


$validation_method = 'validate_' . $key;
if ( method_exists( $this, $validation_method ) ) {
$this->$validation_method( $value );
}

$this->attributes[ $key ] = $value;
$this->attributes[ $key ] = $this->castAttribute( $key, $value );

return $this;
}
Expand Down