class DateTimeType is buggy

[villalvilla]

Hello,

This is my first bug repport, so I will try to be as concrete as possible. I found a serious problem with doctrine and DateTime objects. If you create an M-N relationship with attributes inside the relationship, it's known that you have to implement it with two Many-To-One relationships in doctrine and another Entity in the middle. The thing is that, as \DateTime class in php doesn't implement __toString method, there are a couple of points in file doctrine\orm\lib\Doctrine\ORM\UnitOfWork.php that will crash:

1. Line 2437: $idHash = implode(' ', $id);

This will fail because in the M-N relationship scenario that I've reported, $id will be an object instead an string, so it will create an exception of the type "Object of class DateTime could not be converted to string"

1. Line 1337: $idHash = implode(' ', $this->entityIdentifiers[spl_object_hash($entity)]);

This will fail because of the same reason, but it will fail due to the persist action in the inbetween entity.

I've managed to patch this situation, and I think it's pretty simple to do it:

1st step: You should create a class named, for example, DateTime2 that inherits form \DateTime and just implements method __toString:

class DateTime2 extends \DateTime{
    public function __toString() {
        return $this->format('Y-m-d H:i:s');
    }
}

2nd step: Modifying DateTimeType and DateType classes to adapt to this new implementation. The code I've created is not the best one and could be factorized, but gives you the idea (and also works):

class DateTimeType extends Type
{
    public function getName()
    {
        return Type::DATETIME;
    }

    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
    {
        return $platform->getDateTimeTypeDeclarationSQL($fieldDeclaration);
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform)
    {
        return ($value !== null)
            ? $value->format($platform->getDateTimeFormatString()) : null;
    }

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

        $val = \DateTime::createFromFormat($platform->getDateTimeFormatString(), $value);
        $val1 = new DateTime2();
        $val1->setTimestamp($val->getTimestamp());
        $val=$val1;
        if ( ! $val) {
            throw ConversionException::conversionFailedFormat($value, $this->getName(), $platform->getDateTimeFormatString());
        }
        return $val;
    }
}

3rd step: Using new DataTime2 class everywhere in the entities. I send you a piece of code for a sample getter and setter functions:

    /**
     * Set the value of fechamodificacion.
     *
     * @param DateTime2 $fechamodificacion
     * @return UsuarioModificaExercise
     */
    public function setFechamodificacion(\Gestion\CoreBundle\Classes\DateTime2 $fechamodificacion)
    {
        $this->fechamodificacion = $fechamodificacion;

        return $this;
    }

    /**
     * Get the value of fechamodificacion.
     *
     * @return DateTime2
     */
    public function getFechamodificacion()
    {
        return $this->fechamodificacion;
    }

Hope this helps, Miguel Amez

[backbone87]

You should always be "as concrete as possible" in your bug reports, be it your first or 100th ;)

But one major thing you probably forgot: You use a datetime as your PK? Using datetimes as PKs is generally not "best practice" and makes only sense in very edgy cases. Because of that, its perfectly fine to use your own mapping type and overwrite the concrete PHP DateTime implementation to use to fit your needs. But also because the valid use cases are so edgy, it makes no sense to support datetime PKs inside doctrine out-of-the-box.

Related article: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/changelog/migration_2_5.html#support-for-objects-as-identifiers

I dont think, your problem will or should be covered by doctrine out-of-the-box. Your current approach is good for your project.

btw: A better name for your DateTime2 class would be "StringifyableDateTime" because it exactly describes what your subclass does.

PS for the devs: I searched the doctrine docs for a list of explicitly supported PK data types, but couldnt find one.

[villalvilla]

Hi Backbone,

I really appreciate your feedback and humor sense. It's allways positive anywhere ;-) You talk about a "not-best-practice" using datetimes as PKs, but then how could someone, for example, create a database-guided log with any single change in a document?

Come on! This is a common issue, I could give you tons of links to http://stackoverflow.com that really point to this issue! Of course you have to dig a lot inside Doctrine's core, but in the end it's allways more or less the same issue.

In general, Doctrine has a great problem with ManyToMany relationships with extra attributes. To be as purist as possible, Doctrine should allow this kind of ManyToMany relationships, because they are the "formal" implementation of Boyce-Codd Normal Form.

From the point of view of the naming, I called it "DateTime2" just to respect something like a "try" that your team created in this ticket file: doctrine\orm\tests\Doctrine\Tests\ORM\Functional\Ticket\DDC1209Test.php

It seems like someone was thinking about this issue, doesn't it?

Thanks again for your feedback, Miguel

[backbone87]

You talk about a "not-best-practice" using datetimes as PKs, but then how could someone, for example, create a database-guided log with any single change in a document?

Especially in this use case, using a datetime as PK is problematic. Using an artifical integer PK to denote version logs with an indexed "created_at" column, is much more reliable. Datetimes have resolutions. if 2 changes happen wihin the same timeslice, you cant track them, beause of duplicate PK. Its basically the same problem described for the optimitic locking at http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/transactions-and-concurrency.html#optimistic-locking

Version numbers (not timestamps) should however be preferred as they can not potentially conflict in a highly concurrent environment, unlike timestamps where this is a possibility, depending on the resolution of the timestamp on the particular database platform.

Regarding n:m

In general, Doctrine has a great problem with ManyToMany relationships with extra attributes. To be as purist as possible, Doctrine should allow this kind of ManyToMany relationships, because they are the "formal" implementation of Boyce-Codd Normal Form.

But from doctrines point of view, you dont have 2 entities with an attributed n:m relation. You have 3 entities with 2x 1:n relations. I dont know any relational DBMS that can model a ER-diagram 1:1. You always use join tables and extra tables. I dont even know how you would model an attributed n:m relation in OOD that is easier to understand and better to use, than the doctrine way (3 entities, 2x 1:n relation). All RDBMS schemas and OO patterns rely on (higher) abstractions of the (lower) diagram abstractions, because you dont have rectangles and diamonds in real life software ;)

[Ocramius]

Fixed OP syntax

[Ocramius]

This is a known limitation that simply needs documenting in http://doctrine-orm.readthedocs.org/projects/doctrine-orm/en/latest/reference/limitations-and-known-issues.html

Sorry, we can't fix this atm. Yes, identifier fields must be stringable due to how the ORM hashes identifiers internally.

[Ocramius]

Please check https://github.com/doctrine/doctrine2/blob/a4d84e0cd8cc4ea1b99f4fd7d1d560f3fbe27c30/docs/en/reference/limitations-and-known-issues.rst if you think that you can help us document the limitation :-)

[villalvilla]

Especially in this use case, using a datetime as PK is problematic. Using an artifical integer PK to denote version logs with an indexed "created_at" column, is much more reliable. Datetimes have resolutions. if 2 changes happen wihin the same timeslice, you cant track them, beause of duplicate PK. Its basically the same problem described for the optimitic locking at http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/transactions-and-concurrency.html#optimistic-locking

Very interesting point. Really didn't know about that article, so I will work on changing this. Although this is true, Wouldn't be possitive to fix that bug? Maybe the symfony-way could be richer than directly allowing, I mean, if there is a manual of better practices, then giving the advice of changing that entity abstraction the way symfony does could re-conduct the coder better than documenting it more propertly in the public documentation. For example, instead of:

$idHash        = implode(' ', $this->entityIdentifiers[spl_object_hash($entity)]);

It could be coded something like this:

try {
$idHash        = implode(' ', $this->entityIdentifiers[spl_object_hash($entity)]);
} catch (Exception $e) {
    if (is_object($this->entityIdentifiers[spl_object_hash($entity)]) && get_class($this->entityIdentifiers[spl_object_hash($entity)])=="DateTime") {
       echo "It's not a very good practice to use datetime database type as a PK...";
    }
}

What do you think? It's just a pillgrim idea ;-)

But from doctrines point of view, you dont have 2 entities with an attributed n:m relation. You have 3 entities with 2x 1:n relations. I dont know any relational DBMS that can model a ER-diagram 1:1. You always use join tables and extra tables. I dont even know how you would model an attributed n:m relation in OOD that is easier to understand and better to use, than the doctrine way (3 entities, 2x 1:n relation). All RDBMS schemas and OO patterns rely on (higher) abstractions of the (lower) diagram abstractions, because you dont have rectangles and diamonds in real life software ;)

Loooool!!! Of course I know this is a hard work and an abstraction, but you know... Allways trying to look on the bright side of life XXD.

[DHager]

As a side-note, I think in many cases you'll want a DateTimeImmutable instead, especially if you're using it for things like keys.

[villalvilla]

As a side-note, I think in many cases you'll want a DateTimeImmutable instead, especially if you're using it for things like keys.

Sorry, DHager, I don't see the point... The problem in Doctrine's code is with DateTime, because if you use anotations, then Doctrine will care about everything behind the scenes... Could you please tell me how could DateTimeInmutable would help here? Maybe I'm facing this issue from the wrong perspective.

[Ocramius]

We already rejected previous hacks around DateTime by the way: don't put hardcoded class names in the ORM logic.

[DHager]

@villavilla I'm not saying it would solve your particular problem, I'm just pointing out that primary keys are usually immutable, and that DateTime doesn't guarantee that, so someone could potentially go $entity->getId()->setDate(3000,1,1) and change the PK.

[villalvilla]

I'm Re-coding my approach the way backbone87 pointed, but I found another problem with that implementation: Doctrine doesn't allow auto-increment keys in composite keys, so I should do it inside entity code. I see what you mean, Ocramius, because if I do that, then any composer update will replace my code and it won't be scalable.

Don't you think it is a little bit difficult to manage M-N entities with attributes?

[Ocramius]

I see what you mean, Ocramius, because if I do that, then any composer update will replace my code and it won't be scalable.

What I mean is that such a change will not happen in ORM core either, as it is a symptom fix, not fixing the actual cause ;-)

Don't you think it is a little bit difficult to manage M-N entities with attributes?

Not really: http://stackoverflow.com/questions/15616157/doctrine-2-and-many-to-many-link-table-with-an-extra-field