doctrine / DoctrineModule

Doctrine Module for Laminas
http://www.doctrine-project.org/projects/doctrine-module.html
MIT License
398 stars 269 forks source link

Use Type manager for ORM and MongoDB ODM or generic handling for other #647

Closed TomHAnderson closed 5 years ago

Erikvv commented 6 years ago

This change means we treat values which originate from php (or html or json) as if they originate from the database.

I understand that it improves consistency between simple and complex types (enum, uuid, geo, etc). And I would definitely approve it on those grounds.

Logically the raw format from the database is not necessarily the same as the json/html format. Strategies would be the logical solution in thst case.

This does rely on convertToPhpValue to not do anything if you're already supplying the correct type. Which it usually does.

But any breakage would be because of convertToPhpValue implementations. I see for example that the JsonType::convertToPhpValue would cause a bug because it does not take into account the situation where the value is already decoded.

Should we add tests for all types?

TomHAnderson commented 6 years ago

Tests for all types seems like a function of the dbal or odm implementation because that is where those type classes exist. Here I'm only using what those already implement and IMHO this is the correct way to do it and the original, replaced, code was very wrong.

But this more correct implementation can cause incorrect implementations to fail. See https://github.com/API-Skeletons/zf-doctrine-module-zend-hydrator/issues/6

Erikvv commented 6 years ago

I think if the field type is json_array or sinple_array then for that property you should be able to pass an array to the hydrator.

And if the field type is object you should be able to provide an object.

You seem to disagree right?

My reasoning is that the deserialization of such properties will usually already have been done before it hits the hydrator. For example in a form post or json api.

Erikvv commented 6 years ago

Tests for all types seems like a function of the dbal or odm implementation because that is where those type classes exist.

They are tested with data guaranteed to be coming from a database but now we will use them for data coming from other sources.

I am curious what the maintainers of doctrine/dbal think about thrusting this new responsibility on the Type classes and adjusting them for this purpose. They have often been mentioned as performance-critical so I think they will disagree with this use case.

I have no doubt about your good intentions but I have my doubts about using the Type classes. It definitely needs more approval.

TomHAnderson commented 6 years ago

And if the field type is object you should be able to provide an object. You seem to disagree right?

I do not disagree but handling that case should be a function of the Type class: https://blog.tomhanderson.com/2018/09/datetime-with-microseconds-for-mysql-in.html

public function convertToPHPValue($value, AbstractPlatform $platform)
    {
        if ($value === null || $value instanceof DateTimeInterface) {
            return $value;
        }

In this case on my custom type I cover the case of the value already existing as a DateTimeInterface and I think that's what the other types should do too.