Add CustomType class to let use convertToPHPValue method instead of closureToPHP (no reflection) #1513
Conversation
|
I'm personally fine with such addition (also introducing this method in a
Anyways thanks @mgonzalezbaile for throwing the idea :) |
|
I guess I should wait for confirmation before starting working on the task list given by @malarzm, right? |
|
Pinged folks on IRC, please bear with us a little longer @mgonzalezbaile |
| $fqcn = self::class; | ||
|
|
||
| return sprintf(' | ||
| $type = \%s::getType($typeIdentifier); |
There was a problem hiding this comment.
Would it make sense to call Doctrine\ODM\MongoDB\Types\Type::getType() directly here, or do we expect that it might be overridden (the method isn't final)?
There was a problem hiding this comment.
I wouldn't expect anyone to override the main method but we can't slap final
There was a problem hiding this comment.
TBH, we're always using Doctrine\ODM\MongoDB\Types\Type::getType() directly to fetch a type, so I'd do the same here.
| /** | ||
| * @return string Redirects to the method convertToPHPValue from child class | ||
| */ | ||
| final public function closureToPHP() |
There was a problem hiding this comment.
(also introducing this method in a trait wanders through my mind)
@malarzm: A trait may allow users more flexibility than forcing them to extend a new CustomType base class.
|
|
||
| return $value; | ||
| return array_map( | ||
| function ($v) { |
There was a problem hiding this comment.
Is there a particular reason to not rely on Doctrine\ODM\MongoDB\Types::convertToPHPValue() here, as the original code was doing with $converter->convertToPHPValue()?
|
I'm 👍 on moving this to a trait and 👍 in general as a fix to basically writing the logic twice for every type. Since we've already talked about changing the types a bit for 2.0 (parameters for types come to mind, e.g. immutable=true/false for DateType), and I would like to add the closure handling to that list. Currently, a type closure can access all document data as well as the finished object during hydration and modify it at will. I know performance is a key issue here, but we also need to limit the scope a bit here. As it is, a type can do a lot of things during document hydration. |
|
I think I applied all your comments, let me know if something is missing :) |
| @@ -0,0 +1,18 @@ | |||
| <?php | |||
|
|
|||
There was a problem hiding this comment.
Please add licensing, you can copy it from any other file
|
|
||
| return sprintf(' | ||
| $type = \%s::getType($typeIdentifier); | ||
| $return = $type->convertToPHPValue($value);', $fqcn); |
There was a problem hiding this comment.
Could you make this return sprintf('$return = \%s::getType($typeIdentifier)->convertToPHPValue($value);', Type::class); ?
|
@malarzm do you think we can merge? |
|
Ah crap @mgonzalezbaile I'm sorry, this fell through the cracks due to intensive season in my normal work... Anyways when I was about to hit "Merge" button last time I realized that we need to change Also we'll need to add a note in the docs so people will know such feature is in place, but this can be done by us in a follow up PR to speed things up. |
|
@malarzm just renamed the trait :-) |
|
@mgonzalezbaile thanks! I'll merge this prob later tonight |
|
Merged manually in 44e1b92 - thanks @mgonzalezbaile! |
Issue #1509
Notes: Per suggested by @malarzm here it is another approach from PR #1510 avoiding reflection to instantiate a
Type.