-
Notifications
You must be signed in to change notification settings - Fork 99
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
ReflectionDescriptor: deduce database internal type based on parent #582
ReflectionDescriptor: deduce database internal type based on parent #582
Conversation
} catch (DescriptorNotRegisteredException $e) { | ||
continue; | ||
} | ||
} |
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 also have another idea how to implement this without the requirement of known parent. Then, you can extend even abstract Doctrine\DBAL\Types\Type
and still have proper type (which is not working in the implementation here).
We use it our codebase, but it may feel a bit more hacky:
public function getDatabaseInternalType(): Type {
/** @var DbalType $dbalType */
$dbalType = new $this->type();
$schemaColumn = new SchemaColumn('dummy', $dbalType, []);
$fullSqlType = strtolower($dbalType->getSQLDeclaration($schemaColumn->toArray(), new MySQL80Platform()));
$typeName = Strings::replace($fullSqlType, '~^([a-z ]+).*?$~', '$1');
return match ($typeName) {
'tinyint',
'smallint',
'mediumint',
'int',
'bigint' => new IntegerType(),
'decimal',
'numeric' => new IntersectionType([new StringType(), new AccessoryNumericStringType()]),
'double precision' => new FloatType(),
'varchar',
'char',
'binary',
'tinytext',
'text',
'longtext',
'json',
'date',
'datetime',
'time' => new StringType(),
default => throw new LogicException("Unexpected type: $fullSqlType"),
};
}
If you feel that this more generic approach is suitable even for phpstan-doctrine, I can try implementing it in #506 (as ReflectionDescriptor actually need to implement DoctrineTypeDriverAwareDescriptor
).
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.
What about combining both, to cover more use-cases?
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.
If both, I think we can merge this and add that extra layer using getSQLDeclaration
on top of this later.
Thank you! |
Separated from #506.
This is crucial for every codebase that utilizes custom types a lot. Previously any
AVG(t.customTypedField)
was mixed. Now, it can produce proper type.