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

[DoctrineBridge] Fix: Add type detection. Needed by pdo_dblib #9747

Closed

Conversation

Projects
None yet
4 participants
@iamluc
Copy link
Contributor

commented Dec 12, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR
$entities[$fromClause->getAlias()] = $fromClause->getFrom();
}
$meta = $qb->getEntityManager()->getClassMetadata($entities[$alias]);
$type = ('integer' === $meta->fieldMappings[$identifier]['type']) ? Connection::PARAM_INT_ARRAY : Connection::PARAM_STR_ARRAY;

This comment has been minimized.

Copy link
@webmozart

webmozart Dec 29, 2013

Contributor

You can simplify this whole snippet to:

$entity = current($qb->getRootEntities());
$metadata = $qb->getEntityManager()->getClassMetadata($entity);
$parameterType = 'integer' === $metadata->getTypeOfField($identifier) ? Connection::PARAM_INT_ARRAY : Connection::PARAM_STR_ARRAY;

This comment has been minimized.

Copy link
@iamluc

iamluc Jan 2, 2014

Author Contributor

Thanks. Commit updated.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 2, 2014

Is it possible to add some unit tests to cover this change?

// Guess type
$entity = current($qb->getRootEntities());
$metadata = $qb->getEntityManager()->getClassMetadata($entity);
$parameterType = 'integer' === $metadata->getTypeOfField($identifier) ? Connection::PARAM_INT_ARRAY : Connection::PARAM_STR_ARRAY;

This comment has been minimized.

Copy link
@piotrpasich

piotrpasich Jan 2, 2014

There might occur some problem in this line because $identifier may also be a bigint, not only the integer value. I have read a PR base on the same problem today. Unfortunatelly I cannot find this one.

Actually in doctrine repo you can find e.g.

if (in_array($mapping['type'], array('integer', 'bigint', 'smallint')))

This comment has been minimized.

Copy link
@iamluc

iamluc Jan 3, 2014

Author Contributor

Updated to manage the 3 types

$query = $this->getMockBuilder('Symfony\Bridge\Doctrine\Tests\Mock\QueryMock')
->setConstructorArgs(array($em))
->setMethods(array('setParameter', 'getResult'))

This comment has been minimized.

Copy link
@webmozart

webmozart Jan 7, 2014

Contributor

Can you add getSql() and _doExecute() to this array and remove the QueryMock class?

This comment has been minimized.

Copy link
@iamluc

iamluc Jan 7, 2014

Author Contributor

Done. I did that because Doctrine\ORM\Query is a final class, but you were right it was not needed.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 7, 2014

Thanks for fixing this bug @iamluc.

fabpot added a commit that referenced this pull request Jan 7, 2014

bug #9747 [DoctrineBridge] Fix: Add type detection. Needed by pdo_dbl…
…ib (iamluc)

This PR was submitted for the 2.3-dev branch but it was merged into the 2.3 branch instead (closes #9747).

Discussion
----------

[DoctrineBridge] Fix: Add type detection. Needed by pdo_dblib

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Commits
-------

75f5cc3 Add type detection. Needed by pdo_dblib

@fabpot fabpot closed this Jan 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.