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.

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

Bugfix Issue #190 #231

Closed
wants to merge 2 commits into from
Closed

Bugfix Issue #190 #231

wants to merge 2 commits into from

Conversation

Engerim
Copy link
Contributor

@Engerim Engerim commented Jan 12, 2012

As described in #190 by cbou. The problem occurs in the function "loadReferenceManyCollectionOwningSide" in DocumentPersister. Each ReferenceID will cast to a String also Increment Id's.

@iamjochem
Copy link

I wonder if this is the correct fix (I have been fighting with problems related to properties marked as $Id with a strategy of INCREMENT too) ... my problem was related to lazyloading of Documents via DocumentPersister::Load.

as far as I can tell the fix should occur in the convertToDatabaseValue() method of the relevant Doctrine\ODM\MongoDB\Mapping\Types\Type subclass ... i.e.:

Doctrine\ODM\MongoDB\Mapping\Types\CustomIdType

I make this assumption base on the fact that Document properties marked with annotations as follows always get the 'custom_id' data type:

class Test 
{
    /**
     * @MongoDB\Id(strategy="INCREMENT")
     */
    protected $id;

therefore the $value needs to be cast to an int when being converted for use in the database, e.g.

namespace Doctrine\ODM\MongoDB\Mapping\Types;

class CustomIdType extends Type
{
    public function convertToDatabaseValue($value)
    {
        return $value !== null ? (int)$value : null;
    }

@Engerim
Copy link
Contributor Author

Engerim commented Jan 13, 2012

The Problem with this is that every strategy which is not Auto will be CustomIdType. For example a uuid is cast to a int

@iamjochem
Copy link

Hi Engerim,

thanks for the info about CustomIdType, a simplistic solution would then be something like this:

namespace Doctrine\ODM\MongoDB\Mapping\Types;

class CustomIdType extends Type
{
    public function convertToDatabaseValue($value)
    {
        return $value !== null ? (is_numeric($value) ? (int)$value : (string)$value) : null;
    }

... but that seems somewhat wrong.

any idea how to tell ODM to use something other than CustomIdType for and @id field? looking further I think that the type should be Doctrine\ODM\MongoDB\Mapping\Types\IncrementType if strategy is set to "INCREMENT" ... what do you think?

either way it does not seem to be possible to control the type via the annotation/config of the field when using strategy "INCREMENT" - I would be happy to do something like the following (but it does not work):

class Test 
{
    /**
     * @MongoDB\Id(strategy="INCREMENT", type="increment")
     */
    protected $id;

needless to say the problem of auto-incremented mongo primary ids being casted to strings rather than int's occurs in more places than the function "loadReferenceManyCollectionOwningSide" ... so it would seem that the solution to the casting problem should occur somewhere else, no?

@Engerim
Copy link
Contributor Author

Engerim commented Jan 13, 2012

The ClassMetadataInfo decide which type is set. I do not know if this problem occurs in other places. I just had the same problem as CBOU. I'm home now and can take a look on it on Monday. Maybe it solves the problem, when in the ClassMetadataInfo the mapping type "increment" for auto-increment set.

namespace Doctrine\ODM\MongoDB\Mapping;

use Doctrine\ODM\MongoDB\MongoDBException,
    Doctrine\ODM\MongoDB\LockException,
    ReflectionClass;

class ClassMetadataInfo implements \Doctrine\Common\Persistence\Mapping\ClassMetadata
{
    /* The Id generator types. */
    /**
* AUTO means Doctrine will automatically create a new \MongoId instance for us.
*/
    const GENERATOR_TYPE_AUTO = 1;

    /**
* INCREMENT means a separate collection is used for maintaining and incrementing id generation.
* Offers full portability.
*/
    const GENERATOR_TYPE_INCREMENT = 2;

    /**
* Map a field.
*
* @param array $mapping The mapping information.
*/
    public function mapField(array $mapping)
    {

        if (isset($mapping['id']) && $mapping['id'] === true) {
            $mapping['name'] = '_id';
            $mapping['type'] = isset($mapping['type']) ? $mapping['type'] : 'id';
            $this->identifier = $mapping['fieldName'];
            if (isset($mapping['strategy'])) {
                $generatorType = constant('Doctrine\ODM\MongoDB\Mapping\ClassMetadata::GENERATOR_TYPE_' . strtoupper($mapping['strategy']));
                if ($generatorType !== self::GENERATOR_TYPE_AUTO) {
                    $mapping['type'] = 'custom_id';
                }
                $this->generatorType = $generatorType;
                $this->generatorOptions = isset($mapping['options']) ? $mapping['options'] : array();
            }
        }
    }
}

@jvega
Copy link
Contributor

jvega commented Jan 31, 2012

perhaps in increment strategy is better do the cast of the @id

$doc = $dm->find('\Collection', (int)$identifier);

@jwage
Copy link
Member

jwage commented Feb 4, 2012

We need to fix all the occurrences where we do $id = (string) $id and replace it with $id = Type::getType($mapping['type'])->convertToPHPValue($id). That should fix the problem.

@jwage
Copy link
Member

jwage commented Feb 4, 2012

But I would suggest writing a test for this first so we can write a patch against that test.

@jwage jwage closed this in befb8e2 Oct 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants